Update AnswersEditor layout and answer option structure#5949
Update AnswersEditor layout and answer option structure#5949Abhishek-Punhani wants to merge 4 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. |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Two blocking issues need to be resolved before merging — see inline comments.
CI: All 10 CI runs on a15eaa4 completed successfully.
Visual inspection: Screenshots and design mockups were present and reviewed. All five acceptance criteria are visually confirmed: correct rows show green border and green background; incorrect rows show a neutral fine-line border with transparent background; checkboxes are absent for Numeric Input; the section label is dynamic and correct for each question type; the '+ Add option' button renders with dashed purple border and plus icon matching the design spec. Minor note: the checkbox fill remains the default design-system purple/indigo (not green) — this matches the mockup and KDS conventions. All numeric-input answer rows appear green (all are implicitly correct: true), which matches the mockup but is undocumented in code.
Blocking
toggleCorrectAnswer(line 327): when the user unchecks an already-correct answer, theelsebranch is skipped entirely — the action is a silent no-op. For single-selection this is arguably intentional (radio semantics), but for true-false it is a regression: the previousVRadioGroupallowed deselecting the current choice. At minimum, add a comment explaining the no-op is deliberate; if deselection should be supported for true-false, add that branch. (See inline comment.)KCheckboxis used in the template without being imported or registered as a component (line 46). The priorCheckboxwrapper was removed along with its import andcomponentsregistration, butKCheckboxwas never added. It works because KDS registers it globally in production, but this is fragile: it will silently fail to render in test environments that do not bootstrap the full KDS plugin, and static analysis tools will flag it. Addimport KCheckbox from 'kolibri-design-system/lib/KCheckbox'and register it incomponents. (See inline comment.)
Suggestions
- Line 32: Two nested
divelements serve only class/style binding —.answer-border(outer, carries border/background inline style) and the inner div fromanswerClasses()(carries hover class via$computedClass). The hoverbackgroundColorfrom$computedClasson the inner div will be overridden by the outer div's explicitbackgroundColorfor correct rows, so hover on correct rows is already broken. Merging them and applying hover only when the answer is incorrect would fix the visual inconsistency and reduce nesting. - Line 523:
.answer-card-text.is-closedsetspadding-top: 0; padding-bottom: 0, but the KCheckbox container addsmargin-top: 8px; margin-bottom: 8pxby default, so closed rows still have 16 px of vertical spacing from the checkbox. Consider adding::v-deep .k-checkbox-container { margin-top: 0; margin-bottom: 0; }inside the.answer-card-text.is-closedrule. - Line 532: Closed rows have zero vertical padding but
.answer-view-textprovidesmin-height: 42pxand left/right padding of 7.5px is still applied, producing slight asymmetric spacing. Consider setting uniform vertical padding (e.g.padding: 0 7.5px) or a dedicated closed-row height rule.
Nitpicks
- Line 639:
.answer-editor-buttonhasmargin-top: 10pxin scoped CSS, but the element also has themt-3utility class in the template (line 142) adding anothermargin-top: 12px(Vuetify). The CSS wins on specificity, making the intent ambiguous. Removemt-3from the template or removemargin-topfrom the CSS. - Line 486: The
answersLabelfallback string'Answers'is unreachable —answersLabelcomputed covers all four enum values and the only fallback path would require an unknown question type that would already fail the prop validator. The dead string can be removed to reduce translation surface. - Line 27 (numeric input): All numeric-input answer rows always show green background because
answer.correctistruefor all of them (set inaddNewAnswer). This matches the mockup but may surprise contributors. A short comment explaining that all numeric answers are implicitly correct would improve readability. - Line 27 (checkbox fill): The acceptance criteria mentions "green checkbox background" — the row background is green, but the KCheckbox checked fill remains the default purple/indigo. This matches the design mockup, so no change needed, but worth noting if the intent was to colour the checkbox fill green.
- Line 117: The closing
>of theanswer-actionsdiv is misaligned by one space compared to surrounding template indentation.
Scope
HintsEditor.vuechanges (button colour updated to primary theme tokens,EDIT_ITEMaction removed, hover style via$computedClass) are out of scope for issue #5913, which exclusively targetsAnswersEditor. These changes have no corresponding test updates forHintsEditor. Consider splitting them into a separate issue/PR or explicitly noting the rationale in the PR description.
Displaced findings (not in diff)
- AnswersEditor.vue (original line ~265) — suggestion:
screenSizeLevelcallsuseKResponsiveWindow()on every computed access, invoking the composable inside a computed getter. The composable is designed to be called once at setup/created time and returns reactive refs. Calling it in a computed works but is misleading and allocates a new call on every invalidation. Initialise it once:this._windowBreakpoint = useKResponsiveWindow().windowBreakpointincreated(), then referencethis._windowBreakpoint.valuein the computed. - AnswersEditor.spec.js (original line ~399) — suggestion: The
on answer text updatetest emits oneditors.at(1), relying on DOM ordering to identify the edit-mode editor. Consider filtering explicitly:editors.filter(e => e.props('mode') === 'edit').at(0).vm.$emit('update', ...)to make the intent resilient to ordering changes.
Praise
- Splitting
TipTapEditorinto a static view-mode instance and a<keep-alive>-wrapped edit-mode instance is a meaningful improvement — it avoids re-mounting the heavy editor on every open/close cycle and keeps the view-mode renderer in a stable read-only state. - The click guard for checkboxes (
event.target.tagName === 'input'+event.target.closest('.answer-selection')) is well-considered and correctly prevents the answer from opening when the user intends only to toggle correctness. - Correct-answer row styling via
$themeTokens.correct(border) and$themePalette.green.v_100(background) is clean and theme-aware. - The dynamic
answersLabelcomputed and all four label strings correctly match the design mockup text for every question type. v-if="!isInputQuestion"on the checkbox correctly suppresses it for Numeric Input, confirmed by screenshot 2.- For input questions, the toolbar is correctly reduced to delete-only with no move controls, confirmed by screenshot 2.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
|
|
||
| if (answer.correct) { | ||
| classes.push('correct'); | ||
| toggleCorrectAnswer(answerIdx, isChecked) { |
There was a problem hiding this comment.
blocking: Single-selection / true-false: unchecking the currently correct answer is silently swallowed. toggleCorrectAnswer only calls onCorrectAnswersIndicesUpdate when isChecked is true; when the user unchecks an already-correct answer the else branch is skipped entirely. For a true-false question this means the correct answer can never be cleared once set. For single-selection it means once an answer is marked correct the user can only change it by checking a different answer — unchecking is a no-op. This may be intentional for single-choice (radio-button semantics), but for true-false it is a regression: the old code using VRadioGroup allowed deselecting the current choice when clicking another radio, and there is no alternative action here. At minimum add a code comment explaining the no-op is deliberate; if deselection should be supported for true-false, add that branch.
There was a problem hiding this comment.
@AlexVelezLl Should we allow unselection of correct response, as in single-coice / True-false , clicking on other answer will update the correct ans?
There was a problem hiding this comment.
To clarify the specific concern: clicking a different answer to change the correct selection already works fine — that path goes through the isChecked === true branch and is not the issue.
The gap is the case where you want to clear the correct answer entirely (no correct answer set). In the old VRadioGroup implementation, clicking the already-selected radio toggled it off. With the current KCheckbox + toggleCorrectAnswer approach, unchecking the already-correct box hits the else branch, which does nothing — so once a true-false question has a correct answer set it cannot be cleared.
Whether "no correct answer" is a valid state for true-false is a UX/product question for @AlexVelezLl. If it is not a valid state (i.e. true-false always requires exactly one correct answer) then the no-op is fine, but a comment would prevent future confusion.
There was a problem hiding this comment.
Well, I'm trying the current implementation on prod, and it seems we don't have the uncheck behavior on radio buttons, so the new implementation should keep that behavior :).
There was a problem hiding this comment.
You're right — if prod already doesn't support unchecking radio buttons, then the new implementation is consistent with the existing behavior, not a regression. Withdrawing this finding.
| v-if="!isInputQuestion" | ||
| class="answer-selection" | ||
| > | ||
| <KCheckbox |
There was a problem hiding this comment.
blocking: KCheckbox is used directly in the template without being imported or registered as a component. The existing Checkbox wrapper component was removed (import Checkbox deleted, components: { Checkbox } removed), but KCheckbox is never imported or added to components. It works only if KCheckbox is registered globally by the KDS plugin — which it is in production, but this is fragile: the component will silently not render in any test environment that does not bootstrap the full KDS plugin, and static analysis tools will flag it. Register it explicitly: import KCheckbox from 'kolibri-design-system/lib/KCheckbox'; components: { ..., KCheckbox }.
There was a problem hiding this comment.
We register KThemePlugin in tests too. Using the global components registry is the standard way to reference KDS components.
There was a problem hiding this comment.
You're right — I missed that KThemePlugin registers KDS components globally and is bootstrapped in tests as well. Using the global component registry is clearly the standard practice here. Withdrawing this finding.
| v-if="shouldHaveOneCorrectAnswer" | ||
| :value="correctAnswersIndices" | ||
| @change="onCorrectAnswersIndicesUpdate" | ||
| <div :class="answerClasses(answerIdx)"> |
There was a problem hiding this comment.
suggestion: There are now two nested div elements whose only purpose is class/style binding — the outer div.answer-border (carries border/background style and data-test) and the immediately inner div from answerClasses() (carries the answer / closed / computed hover class). These could be merged into a single element. The hover backgroundColor from $computedClass on the inner div will be overridden by the outer div's explicit backgroundColor inline style for correct rows anyway, so the hover visual for correct rows is already broken. Merging and applying the hover only when the answer is incorrect would fix the visual inconsistency and reduce the nesting.
| padding: 16px; | ||
| } | ||
|
|
||
| .answer-card-text { |
There was a problem hiding this comment.
suggestion: .answer-card-text.is-closed sets padding-top: 0; padding-bottom: 0. The KCheckbox container (from KDS) adds margin-top: 8px; margin-bottom: 8px by default, so closed rows still have 16 px of vertical spacing contributed by the checkbox. The comment in the CSS suggests the intent is a compact closed view; consider adding ::v-deep .k-checkbox-container { margin-top: 0; margin-bottom: 0; } inside the .answer-card-text.is-closed rule, or wrapping with a zero-margin container.
|
|
||
| .v-input--selection-controls { | ||
| margin-top: 6px; | ||
| .answer-editor-button { |
There was a problem hiding this comment.
nitpick: .answer-editor-button has margin-top: 10px in CSS but the element also has the utility class mt-3 applied in the template (line 142), which adds another margin-top: 12px (Vuetify spacing). The CSS rule will win due to specificity but the intent is ambiguous. Remove mt-3 from the template since the scoped CSS already controls the margin, or remove the margin-top from the CSS and rely on the utility class.
| class="card-border-light" | ||
| class="answer-border" | ||
| :style="{ | ||
| borderColor: answer.correct ? $themeTokens.correct : $themeTokens.fineLine, |
There was a problem hiding this comment.
praise: Correct answer row styling is well-implemented — green border via $themeTokens.correct and green background via $themePalette.green.v_100 both apply cleanly to the outer .answer-border wrapper, giving a consistent correct-answer highlight. Incorrect rows correctly fall back to $themeTokens.fineLine border and transparent background.
| }, | ||
| $trs: { | ||
| answersLabel: 'Answers', | ||
| answersLabelSingleChoice: 'Answer options — select one correct answer', |
There was a problem hiding this comment.
praise: The dynamic answersLabel computed property and all four label strings are correctly defined and match the design mockup text exactly. Both screenshots confirm the correct label is rendered for each question type.
| noAnswersPlaceholder: 'Question has no answer options', | ||
| newAnswerBtnLabel: 'New answer', | ||
| numberFieldErrorLabel: 'Answer must be a number', | ||
| newAnswerBtnLabel: 'Add option', |
There was a problem hiding this comment.
praise: The add-option button is correctly updated to 'Add option' (rendered as + Add option via the KIcon plus icon), displayed with a dashed purple border and primary color, matching the design spec precisely.
| > | ||
| <!-- Selection controls --> | ||
| <div | ||
| v-if="!isInputQuestion" |
There was a problem hiding this comment.
praise: The v-if="!isInputQuestion" guard on the checkbox selection control correctly suppresses checkboxes for Numeric Input question type. Screenshot 2 confirms no checkboxes appear on the numeric-input rows, matching the design mockup.
| ]; | ||
| } | ||
|
|
||
| if (this.isInputQuestion) { |
There was a problem hiding this comment.
praise: For input questions the toolbar is correctly reduced to only the delete action ([deleteAction]), with no move-up/move-down buttons. Screenshot 2 shows only X buttons on numeric-input rows, matching the design spec.
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Good progress — commit 5d490e56 resolved four prior findings.
CI passing. Screenshots verified against both question types (single-choice, numeric input); all five acceptance criteria met visually.
Prior findings
Withdrawn (by me, based on author's rebuttal):
toggleCorrectAnswerno-op for true-false (blocking) — AlexVelezLl confirmed prod already doesn't support unchecking; new behavior is consistentKCheckboxnot registered as component (blocking) — AlexVelezLl confirmed global KDS registration via KThemePlugin is the project-standard approach
Resolved in 5d490e56:
.is-closedpadding / KCheckbox margin (suggestion) — KCheckbox margins now zeroed via conditional inline:stylebindingmt-3+ CSSmargin-topduplication (nitpick) —mt-3class removed from KButtonanswersLabelfallback string unreachable (nitpick) — fallback call and string key both removedanswer-actionsindentation (nitpick) — layout refactored and formatting is now consistent
Still open (raised below):
- Hover background for incorrect closed rows may be suppressed by the inline
backgroundColorstyle (suggestion, continuation of prior finding)
2/2 prior blocking findings withdrawn. 4/4 prior non-blocking findings resolved. 1 re-raised below.
@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 deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| :key="answerIdx" | ||
| class="card-border-light" | ||
| class="answer-border" | ||
| :class="answerClasses(answerIdx)" |
There was a problem hiding this comment.
suggestion: $computedClass applies a :hover { backgroundColor: grey.v_100 } rule for incorrect closed rows (see answerClasses, lines 321–324), but this element also carries an inline style backgroundColor: 'transparent' (line 28). In CSS, inline styles take precedence over class-selector rules — including :hover — so the hover background change may have no visible effect unless $computedClass generates rules with !important. Removing backgroundColor: 'transparent' from the inline :style binding for incorrect rows (omitting it is equivalent, since transparent is the default) would let the $computedClass hover rule apply cleanly. Carry-over from prior suggestion — 5d490e56 fixed the correct-row hover by making the hover conditional, but this narrower incorrect-row case remains.
There was a problem hiding this comment.
Seems like $computedClass does add !important to all its rules. However, this is something we may potentially change in the future. Just to make sure, let's just add a null on the else branch of the conditional
There was a problem hiding this comment.
You're right — $computedClass adding !important to its rules means the hover override would apply regardless of the inline style. My concern was unfounded. Withdrawing this finding.
There was a problem hiding this comment.
Understood — thanks for confirming. Since $computedClass adds !important, the hover does work currently. The null on the else branch is a sensible defensive measure for if that changes. Withdrawing the 'may have no visible effect' claim.
| <KCheckbox | ||
| :checked="answer.correct" | ||
| :style="{ | ||
| marginTop: !isAnswerOpen(answerIdx) ? 0 : '', |
There was a problem hiding this comment.
praise: Zeroing marginTop/marginBottom on KCheckbox via a conditional inline :style is a clean targeted fix — it avoids needing a ::v-deep CSS override keyed to KDS internal class names, and keeps the intent co-located with the component that needs it.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Abhishek-Punhani, you've almost replicated the spec at face value! Just some comments to align it a bit more (also given that a couple of things changed on the Figma 😅), but generally this looks great!
| return windowBreakpoint.value ?? 0; | ||
| }, | ||
| isSmallScreen() { | ||
| return this.screenSizeLevel <= 1; |
There was a problem hiding this comment.
Just curious if there is a reason why this screenSizeLevel <= 1 value is used instead of our windowIsSmall property that exposes useKResponsiveWindow? I think I saw this condition also on another component, and thought it was because of the previous implementation, but I'm not seeing this here 😅.
| :key="answerIdx" | ||
| class="card-border-light" | ||
| class="answer-border" | ||
| :class="answerClasses(answerIdx)" |
There was a problem hiding this comment.
Seems like $computedClass does add !important to all its rules. However, this is something we may potentially change in the future. Just to make sure, let's just add a null on the else branch of the conditional
| :class="answerClasses(answerIdx)" | ||
| :style="{ | ||
| borderColor: answer.correct ? $themeTokens.correct : $themeTokens.fineLine, | ||
| backgroundColor: answer.correct ? $themePalette.green.v_100 : 'transparent', |
There was a problem hiding this comment.
Could we use v_50 instead to match the specs?
There was a problem hiding this comment.
I tried but it wasnt working for me! so I added v_100
There was a problem hiding this comment.
Hmmm, it's weird, we do have a v_50 on the design system. Could you do pnpm install in case you haven't done so recently?
There was a problem hiding this comment.
okk I will see if it works for me after pnpm install
| class="answer-border" | ||
| :class="answerClasses(answerIdx)" | ||
| :style="{ | ||
| borderColor: answer.correct ? $themeTokens.correct : $themeTokens.fineLine, |
There was a problem hiding this comment.
Could we use $themePalette.green.v_500 here instead of .correct to match the specs?
|
|
||
| &.small-screen { | ||
| &.is-closed { | ||
| font-size: 10px; |
There was a problem hiding this comment.
This font size doesn't seem to be applied anywhere. Is there a reason for this?
| <div class="answer-view-editor"> | ||
| <span | ||
| v-if="!answer.answer" | ||
| :style="{ color: $themePalette.grey.v_500 }" |
There was a problem hiding this comment.
Just noting that v_500 does not exist in our gray palette https://design-system.learningequality.org/colors#palette-grey-v_400 😅.
| :style="{ backgroundColor: $themePalette.white }" | ||
| mode="edit" | ||
| minHeight="80px" | ||
| :autofocus="true" |
There was a problem hiding this comment.
pretty much nitpick, but when we have a boolean prop on a component, we can just pass it without any value, e.g.
<TipTapEditor
v-model="answer.answer"
autofocus
...
>| icon="plus" | ||
| :color="$themeTokens.primary" | ||
| /> | ||
| <span>{{ $tr('newAnswerBtnLabel') }}</span> |
There was a problem hiding this comment.
Could we have a "new answer" label only for input questions? If not, call it "add option" to match the specs? Thanks!
| <div class="add-answer-btn-content"> | ||
| <KIcon | ||
| icon="plus" | ||
| :color="$themeTokens.primary" |
There was a problem hiding this comment.
Could we use the themePalette blue v_500 instead? https://design-system.learningequality.org/colors#palette-blue-v_500. I know it's the same blue, but if we ever have a themed system, the color here should keep being blue.
| backgroundColor: this.$themeBrand.primary.v_50, | ||
| border: `1px dashed ${this.$themeBrand.primary.v_200}`, | ||
| color: `${this.$themeTokens.primary} !important`, | ||
| fontSize: '14px', | ||
| fontWeight: '600', | ||
| textTransform: 'none', | ||
| ':hover': { | ||
| backgroundColor: this.$themeBrand.primary.v_100, | ||
| }, |
There was a problem hiding this comment.
Idem, lets use the v_500, v_100 and v_50 shades of $themePalette.blue
…wer row styling and layout logic Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…ve Edit action functionality Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…d label strings Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
…nsive layout logic Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
Abhishek-Punhani
force-pushed
the
Issue5913
branch
from
5d490e5 to
1864120
Compare
3 participants
Summary
Updated AnswersEditor layout and answer option structure and also made some changes in hintEditor
References
Fixes #5913
Reviewer guidance
AI usage
Used Antigravity for final review and nitpicks.