feat: add AnswerSettings and QuestionSettingsHeader components - #6059
feat: add AnswerSettings and QuestionSettingsHeader components #6059Abhishek-Punhani wants to merge 3 commits into
Conversation
|
👋 Hi @Abhishek-Punhani, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
@AlexVelezLl, Vue 2.7 doesn't support the built-in Teleport component, so I've added a custom component to mirror its behaviour. Let me know if you have a better implementation in mind! |
|
Oh, apologies @Abhishek-Punhani 😅, I just knew we had already used it in our ecosystem and didn't recall it was a dependency package. In KDS, we use vue2-teleport. Could you install that same version on Studio, please? I read it has some memory optimizations that'd be good to have! |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Abhishek-Punhani! I think there is a better way to do this to not remove the type selector from the DOM when we change the question type. Please let us know if there is any questions!
There was a problem hiding this comment.
Oh, given that this will only be used in the editor, could we move this component to the choice folder instead?
| settings: { | ||
| type: Array, | ||
| required: true, | ||
| validator: arr => arr.every(setting => ['shuffle', 'showAnswerCount'].includes(setting)), | ||
| }, |
There was a problem hiding this comment.
Given that this will only be rendered for choice interactions, I think it's fine to let it infer when to display each based on the questionType instead of this settings prop.
| :title="showAnswerCountInfoTitle$()" | ||
| @cancel="showAnswerCountModal = false" | ||
| > | ||
| <p :style="{ color: $themeTokens.annotation }"> |
There was a problem hiding this comment.
I think we can leave the normal text color here, instead of this annotation.
| <template #actions> | ||
| <KButton | ||
| :text="closeBtnLabel$()" | ||
| @click="showAnswerCountModal = false" | ||
| /> | ||
| </template> |
There was a problem hiding this comment.
We can also just use the cancelText prop instead of this actions slot. Usually the actions slot is used for more complex button layouts.
| :title="shuffleAnswersInfoTitle$()" | ||
| @cancel="showShuffleModal = false" | ||
| > | ||
| <p :style="{ color: $themeTokens.annotation }"> |
There was a problem hiding this comment.
Actually, the most important responsibility of this component is the type selector, so could we reference "type selector" in the name instead of "SettingsHeader"? (Similarly for class names like question-settings-header, etc)
| <template> | ||
|
|
||
| <div | ||
| v-if="mode === 'edit'" |
There was a problem hiding this comment.
We sometimes have issues because of rendering conditions on the root element of a component, and a lint rule will soon be added to avoid this, so could we move forward with this condition and leave this responsibility to the parent component instead? i.e. let the parent do <QuestionTypeSelector v-if="mode==='edit'" instead
| <div class="select-display-row"> | ||
| <KIcon | ||
| icon="language" | ||
| class="select-globe-icon" | ||
| :style="{ color: $themePalette.grey.v_700 }" | ||
| /> | ||
| <span class="select-value-text">{{ selectedOption.label }}</span> | ||
| </div> |
| @update:showAnswerCount="setShowAnswerCount" | ||
| /> | ||
| </template> | ||
| </QuestionSettingsHeader> |
There was a problem hiding this comment.
Oh, some comments: the interaction editors should not be the ones responsible for rendering this "settings header," which is actually a type selector, mainly because this will cause the DOM to remove these nodes when the component is unmounted. The type selector (and therefore, the whole header row) should be shared across all interaction editors and rendered independently of them, so that if we change the question type, the type selector component is not removed from the DOM (which would cause some accessibility issues).
So, what we can do instead is:
- Have the
InteractionSectioncomponent be the one that renders theQuestionTypeSelectorcomponent. This way, each interaction has its own type selector independent of the editor being rendered. - Let the
QuestionTypeSelectorbe the one responsible for rendering the div with the proper ID so that editor components can target it (instead of using a slot). - We can take advantage of the fact that we can only have one item being edited at a time, and because of this, there will always only be one question type selector rendered at a time, and use a constant as
idso that we don't have to keep track of any identifiers yet.
With this, the idea would be:
// QuestionTypeSelector
<div class="type-selector>
...
<div id="qti-interaction-settings" />
...// InteractionSection
<QuestionTypeSelector ... />
<component :is="descriptor.editorComponent" ... />// ChoiceInteractionEditor
<div class="choice-editor">
<Teleport to="qti-interaction-settings">
<AnswerSettings ... />
</Teleport>
....
</div>This way only the interaction editors that actually wants to add settings are the only ones that needs to teleport anything.
| const questionTypeOptions = computed(() => [ | ||
| { | ||
| value: QuestionType.SINGLE_SELECT, | ||
| label: singleSelectLabel$(), | ||
| description: singleChoiceDescription$(), | ||
| }, | ||
| { | ||
| value: QuestionType.MULTI_SELECT, | ||
| label: multiSelectLabel$(), | ||
| description: multipleSelectionDescription$(), | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
Another little problem with this is that the questionType selector should render all question types for all interactions, not only the ones they are related to.
…iceInteractionEditor Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…eaders Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
0260771 to
9a4d043
Compare
…tegrate question type selection into QTI editor sections Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
9a4d043 to
b385729
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🔵 Review postedLast updated: 2026-08-01 14:37 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6059 — the Answer settings half of #6033 is clean: Composition API throughout, theme tokens only, useKResponsiveWindow instead of media queries, every string through qtiEditorStrings. The type-selector half has three correctness problems.
CI passing. Manual QA was required for this PR but did not run (dev server failed to start), so nothing here asserts visual correctness — the rendered layout, the globe adornment, and small-screen behaviour are unverified.
blocking
- Type dropdown is populated from the whole interaction registry, so a choice question can be switched to a text-entry type and its XML is silently overwritten (
useInteractionDescriptor.js:70). KRadioButtonGroupwraps a list that renders checkboxes for multiple choice — verified TypeError in Firefox (ChoiceInteractionEditor.vue:80).max-choices/min-choicesnow derive from the correct-answer count for single choice too, against the issue's explicitmax-choicesis always1(useChoiceInteraction.js:39).
suggestion / nitpick — see inline: Teleport unregistered in Jest so the new Answer-settings tests never exercise the teleport, dead teleport-target defaults, duplicated buildXML pipeline, showAnswerCount stored inside parsed state, untested update:questionType and cardinality acceptance criteria, missing group names on the two new control clusters.
Two smaller points that don't map to changed lines: the deleted comment <!-- Per-choice validation messages sit INSIDE the bordered card --> (ChoiceInteractionEditor.vue, ~line 165) describes code that is still there — AGENTS.md asks for existing comments to be preserved. And each question now installs two document-body MutationObservers via vue2-teleport; on a long assessment with TipTap running, worth watching during QA.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| ); | ||
|
|
||
| return { descriptor, questionType, parseError }; | ||
| const typeOptions = computed(() => { |
There was a problem hiding this comment.
blocking: descriptors is the full registry (interactions/index.js:15), so a choice question's KSelect offers all five types, including the text-entry ones. Selecting one is a data-loss path: descriptor recomputes to the text-entry descriptor (line 64), <component :is> mounts TextEntryEditor with :interaction still holding the choice block, and its { immediate: true } watcher on workingInteraction (TextEntryEditor.vue:340) emits update:interaction on mount — which QTIItemEditor.onUpdateInteraction writes straight into currentBodyXml. Both descriptors declare convertsFrom = [], so there is no conversion step; the author's choice question is just replaced.
This is also outside #6033's scope ("The modal lists only the types that the current interaction plugin supports") and fails the criterion "Selector shows 'Single choice' and 'Multiple choice' for choice interactions".
Source the options from the resolved descriptor instead:
const typeOptions = computed(() => descriptor.value.getTypeOptions?.(qtiEditorStrings) ?? []);That also makes :disabled="questionTypeOptions.length <= 1" meaningful, and lets TextEntryInteractionDescriptor.getTypeOptions stay for when text-entry switching is actually built. Note selectedOption in QuestionTypeSelector would then need a guard — questionTypeOptions[0] is undefined for a descriptor without getTypeOptions, and the template dereferences selectedOption.label.
| </div> | ||
|
|
||
| <div class="choices-list"> | ||
| <KRadioButtonGroup class="choices-list"> |
There was a problem hiding this comment.
blocking: the list only contains KRadioButtons when isSingleSelect is true — multi-select renders KCheckbox (lines ~120-140). KDS KRadioButtonGroup.vue mounted() (verified in node_modules, lines 41-56) does:
this.lastRadioIdx = this.radioButtons.length - 1;
const firstRadioButton = this.radioButtons[this.focusedRadioIdx]; // undefined
firstRadioButton.setTabIndex(0); // TypeErrorwith no empty guard. The whole block is behind if (!this.isFirefox) return;, so this won't show up in Jest or Chrome QA, but it breaks the editor for every multiple-choice question in Firefox. Same crash for a single-choice question where every choice has a validation error, since the error KIcon replaces the radio button.
Separately, KRadioButtonGroup hard-codes role="radiogroup", so wrapping checkboxes misreports the control type and applies roving arrow-key navigation where Tab-per-checkbox is correct.
channelEdit/components/AnswersEditor/AnswersEditor.vue:11 already has the pattern:
<component :is="shouldHaveOneCorrectAnswer ? 'KRadioButtonGroup' : 'div'">This change isn't called for by #6033 at all, so reverting it is equally valid. If the wrapper stays for single-select, give it aria-labelledby pointing at the "Answers" heading.
| return state.value.choices.filter(c => c.correct).length; | ||
| }); | ||
|
|
||
| const stateForXml = computed(() => ({ |
There was a problem hiding this comment.
blocking: there is no questionType branch here, so the multi-select rule is applied to single choice as well, and buildChoiceInteractionXML writes state.maxChoices verbatim (interactions/choice/parse.js:126). Three concrete regressions:
- Single choice with no correct answer marked yet →
max-choices="0"(unlimited) instead of1. - Switching multiple → single with 3 answers marked correct →
max-choices="3"alongsidecardinality="single";toggleCorrectChoicedoesn't prune on the type switch. - Every single-choice question with a correct answer now also emits
min-choices="1"(previously dropped, since parsedminChoiceswas0), silently making it mandatory to answer.
#6033 is explicit: "For single choice this checkbox is hidden — max-choices is always 1." Gating stateForXml on questionType.value === QuestionType.SINGLE_SELECT, and leaving minChoices at the parsed value in that case, covers all three.
| Vue.component('BaseMenu', BaseMenu); | ||
| Vue.component('Divider', Divider); | ||
| Vue.component('Icon', Icon); | ||
| Vue.component('Teleport', Teleport); |
There was a problem hiding this comment.
suggestion: this global registration is the only one — jest_config/setup.js registers only ActionLink, so in tests <Teleport> renders as an unknown element with its children in place, and Vue.config.silent = true suppresses the warning. That means the whole describe('Answer settings') block in ChoiceInteractionEditor.spec.js passes because the label rendered inline, not at the teleport target; target resolution, the settingsTargetId string surgery, and the ordering of the two teleported blocks are all untested.
It also makes these components silently depend on shared/app.js having run, which is a bad fit for code living in shared/views/. Importing Teleport from 'vue2-teleport' locally in InteractionSection and ChoiceInteractionEditor (and dropping the global registration) fixes both, and avoids squatting on a Vue 3 built-in name. Worth one integration assertion in QTIItemEditor.spec.js that the settings land inside #qti-question-settings-0.
| }, | ||
| teleportTarget: { | ||
| type: String, | ||
| default: '#qti-question-settings', |
There was a problem hiding this comment.
suggestion: #qti-question-settings exists nowhere in the app — only the -${index} variant does. Same for InteractionSection's 'qti-interaction-settings' fallback (index.vue:75). They also interact badly: if InteractionSection renders without teleportTarget, v-if="teleportTarget" skips QuestionTypeSelector, so the #qti-interaction-settings div is never created — yet line 36 still passes that truthy string down, so AnswerSettings teleports into a querySelector that resolves to null and the settings silently vanish.
Make both ids required: true with no default, and pass one id consistently instead of stripping and re-adding the # across three components. Better still: if the descriptor exposed a settingsComponent alongside editorComponent, InteractionSection could render it into a QuestionTypeSelector slot directly — no ids, no teleport, no test-environment divergence.
| expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('disables selector when only one option available', () => { |
There was a problem hiding this comment.
suggestion: neither assertion touches :disabled — the hidden input exists with two options too, and the "Type" label is static. This test passes with :disabled deleted entirely. Assert on the control: expect(screen.getByRole('combobox')).toBeDisabled().
More importantly, the component's one behaviour — @update:questionType (index.vue:23) — has no test, and neither do the two acceptance criteria with real logic behind them: "changing the type updates questionType and re-renders ChoiceInteractionEditor", and "cardinality in the response declaration XML updates to match the new type". The second is the one that has to survive any refactor of the buildXML plumbing; a useChoiceInteraction spec that flips questionTypeRef and parses responseDeclarations.value[0] covers it cheaply.
| <template> | ||
|
|
||
| <div class="answer-settings"> | ||
| <div |
There was a problem hiding this comment.
suggestion: this group heading (and "Type" in QuestionTypeSelector/index.vue:9) is an unassociated <div>, so to a screen reader it's a loose text node — the checkbox set has no group name and "Type" isn't connected to the control it labels. role="group" + aria-labelledby on the label element keeps the current visuals and satisfies WCAG 2.2 AA:
<div role="group" :aria-labelledby="labelId" class="answer-settings">
<div :id="labelId" class="answer-settings-label">{{ answerSettingsLabel$() }}</div>That also gives the tests a semantic handle for within(), replacing the document.querySelector('.choices-list') and .select-display-row class queries in the new specs — those assert styling hooks rather than behaviour and break on a rename. The if (!list) return [] guard in ChoiceInteractionEditor.spec.js:93 is worse than a failure: a markup change turns those assertions vacuous instead of red.
| :questionType="questionType" | ||
| :questionTypeOptions="typeOptions" | ||
| :settingsTargetId="settingsTargetId" | ||
| @update:questionType=" |
There was a problem hiding this comment.
nitpick: a two-statement handler that writes to a setup-owned ref from a template expression. It works in Vue 2.7 (proxyWithRefUnwrap forwards the set), but it hides the mutation from the script block — and the watch(questionType, ..., { immediate: true }) at line ~55 already emits update:questionType, so each selection emits twice. A named onQuestionTypeChange(newType) in setup that only assigns the ref, letting the watcher emit, is clearer and emits once.
Related: currentQuestionType is stored in QTIItemEditor but never fed back as a prop, so this ref is the real source of truth. Fine functionally — worth a comment so the duplicated state isn't read as a wiring bug later.
| </div> | ||
| </div> | ||
|
|
||
| <div :id="`qti-question-settings-${index}`"></div> |
There was a problem hiding this comment.
nitpick: unique across questions in one QTIEditor, but vue2-teleport resolves targets with document.querySelector, which takes the first document-wide match. Two QTIEditor instances on a page would teleport both selectors into the first card. Suffixing with _uid makes it collision-proof for one interpolation.
| <div | ||
| class="question-type-selector" | ||
| :class="{ 'small-screen': windowIsSmall }" | ||
| :style="{ borderBottom: `1px solid ${$themeTokens.fineLine}` }" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: the conventions that usually slip are all clean here — no @media (useKResponsiveWindow drives .small-screen), no hard-coded colours, and the only inline bindings are color and borderBottom, both direction-agnostic so RTLCSS has nothing to flip.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6059 — 12 findings open, 1 new. CI passing; QA did not run.
Prior-finding status
RESOLVED AnswerSettings/
RESOLVED QuestionTypeSelector/index.vue:1
RESOLVED InteractionSection/index.vue:15
RESOLVED package.json:1
RESOLVED InteractionSection/index.vue:2
RESOLVED AnswerSettings/index.vue:29
RESOLVED AnswerSettings/index.vue:51
UNADDRESSED useInteractionDescriptor.js:70
UNADDRESSED ChoiceInteractionEditor.vue:80
UNADDRESSED useChoiceInteraction.js:39
UNADDRESSED app.js:263
UNADDRESSED ChoiceInteractionEditor.vue:547
UNADDRESSED useChoiceInteraction.js:45
UNADDRESSED useChoiceInteraction.js:21
UNADDRESSED QuestionTypeSelector.spec.js:77
UNADDRESSED AnswerSettings/index.vue:4
UNADDRESSED InteractionSection/index.vue:20
UNADDRESSED QTIItemEditor/index.vue:29
UNADDRESSED ChoiceInteractionEditor.vue:168
ACKNOWLEDGED QuestionTypeSelector/index.vue:6
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| <KIconButton | ||
| icon="infoOutline" | ||
| :tooltip="shuffleAnswersInfoTitle$()" | ||
| :ariaLabel="shuffleAnswersInfoTitle$()" |
There was a problem hiding this comment.
suggestion: ariaLabel duplicates the checkbox label.
3 participants
Summary
Added
QuestionSettingsHeaderandAnswerSettingscomponents to the QTI editor using portals.This PR adds:
QuestionSettingsHeader/index.vue— UI component for selecting interaction types.AnswerSettings/index.vue— UI component for interaction-specific configurations.References
Closes #6033
Reviewer guidance
AI usage
Used Antigravity for a final review and minor code/style nitpicks. I reviewed all suggested changes, kept only the relevant improvements, and verified that the implementation worked as intended after applying them.