Implement PicturePasswordGrid component#14546
Implement PicturePasswordGrid component#14546AlexVelezLl wants to merge 3 commits intolearningequality:developfrom
Conversation
github-actions
bot
added
APP: User
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Two acceptance criteria from #14409 aren't met. CI passing. Visual inspection: reviewed 4 frames from the PR video — selected, default, disabled, and hover states all render correctly against the spec. Badge in upper-left, grayscale+opacity for disabled, blue border+tint for selected.
- blocking:
selectevent emits no payload — see inline - blocking: no separate event for disabled activation — see inline
- nitpick: badge SCSS comment says "top-right" but code/spec places it top-left — see inline
- praise: native checkbox +
aria-disabledaccessibility design — see inline
@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
|
|
||
| const onSelect = () => { | ||
| if (!props.disabled) { | ||
| emit('select'); |
There was a problem hiding this comment.
blocking: The select event emits no payload, but #14409's acceptance criteria state "An event is emitted with the icon token when clicked/activated and not disabled." The parent grid needs the token to record which icon was chosen.
Change to:
emit('select', props.icon);|
|
||
| await userEvent.click(screen.getByRole('checkbox', { name: picturePasswordStrings.bird$() })); | ||
|
|
||
| expect(emitted()['select']).toBeTruthy(); |
There was a problem hiding this comment.
blocking: This test only checks that the event was emitted, not its payload. Once the payload is added, add an assertion like:
expect(emitted()['select'][0]).toEqual(['birdColorful']);| if (!props.disabled) { | ||
| emit('select'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
blocking: #14409 requires "A separate event fires when a disabled option receives a click or Space/Enter keypress (this will be needed by the parent grid for the screen reader live region announcement in the following issue)." Currently onSelect() silently returns when disabled — no event is emitted.
Add an else branch:
const onSelect = () => {
if (props.disabled) {
emit('disabledSelect');
} else {
emit('select', props.icon);
}
};And add a corresponding unit test verifying that clicking a disabled option emits disabledSelect.
|
|
||
| .badge { | ||
| position: absolute; | ||
| // Position the badge centered on the top-right corner of the option, just outside the border. |
There was a problem hiding this comment.
nitpick: Comment says "top-right corner" but left: negative positions the badge at the top-left corner (which matches the spec and the screenshots). Should be "top-left corner".
There was a problem hiding this comment.
Top-right corner for RTL languages? hahaa okay, no. Fixed.
| We do not set the `disabled` HTML attribute so the input keeps focus. | ||
| `aria-disabled` communicates the disabled state to assistive technology. | ||
| --> | ||
| <input |
There was a problem hiding this comment.
praise: Using a native <input type="checkbox"> with aria-disabled instead of the HTML disabled attribute is the right call — it keeps the element in the tab order and focusable by keyboard users, while still communicating the disabled state to assistive technology. The comment explaining the rationale is helpful.
Build Artifacts
Smoke test screenshot |
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
bdddeea to
af8e151
Compare
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
af8e151 to
f89363e
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All three blocking issues from the prior review are resolved.
Prior findings
Resolved:
selectevent emitted with no payload (blocking) — now emitsprops.iconat line 121- Test didn't assert payload (blocking) —
toEqual(['birdColorful'])assertion added at line 25 - No
disabledSelectevent when disabled (blocking) — event emitted in else branch; test added - Comment said "top-right corner" but badge is top-left (nitpick) — comment corrected
4/4 prior findings resolved. 0 re-raised.
CI passing (build assets job still in progress at review time, all other checks green). Visual: 4 video frames reviewed — selected, hover, disabled, and default states all render correctly.
praise: The test suite now covers the full contract — select payload, disabled suppression, disabledSelect emission, badge visibility, and both aria-disabled branches. That's thorough for a single-component PR.
@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
AlexVelezLl
changed the title
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Good implementation of the Grid component — all acceptance criteria from #14410 are met. CI passing. Visual inspection: reviewed 7 frames from the PR video — selected/disabled/complete states all render correctly, badge positioning and progress bar fill correctly, submit button enables at 3 selections.
Prior findings
Resolved:
selectevent emitted with no payload (blocking) — now emitsprops.iconat line 127- Test didn't assert payload (blocking) —
toEqual(['birdColorful'])assertion added at line 25 - No
disabledSelectevent when disabled (blocking) — else branch added; test added - Comment said "top-right corner" but badge is top-left (nitpick) — comment corrected
4/4 prior findings resolved. 0 re-raised.
New findings below.
- suggestion: Live region re-triggering approach — see inline
- suggestion: Submit button is missing
type="button"— see inline - nitpick:
ICON_ENTRIESdefined insidesetup()— see inline - praise: Deselect logic using
slice— see inline - praise:
useKLiveRegiondelegation — see inline
@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
| sendPoliteMessage(ORDINAL_STRING_MAP[position]({ icon: iconLabelFor(id) })); | ||
| }; | ||
|
|
||
| const handleDisabledSelect = () => { |
There was a problem hiding this comment.
suggestion: The issue spec required a nextTick clear-before-set pattern to guarantee re-announcement when the same disabled-icon message is sent repeatedly: clear the live region, await a tick, then set the new content. useKLiveRegion.sendPoliteMessage does the opposite — it sets immediately and clears after 350ms. If a user taps the same disabled icon twice within that 350ms window, the DOM text hasn't changed between calls, and some screen readers won't re-announce.
The test mocks sendPoliteMessage so it only verifies the function was called twice — it doesn't verify the DOM actually re-triggers. This is worth checking against NVDA/VoiceOver: does rapid repeated tapping of the same disabled icon re-announce? If screen reader testing confirms it works, a comment explaining the delegation to KDS is sufficient.
There was a problem hiding this comment.
I imagine this may be the desired behavior, given that useKLiveRegion is implemented this way.
There was a problem hiding this comment.
Fair point — if useKLiveRegion handles the timing internally, delegating to it is the right call. Withdrawing this suggestion.
|
|
||
| <!-- Submit button: shows only a forward-arrow icon; the aria-label | ||
| cycles through four instructional states as the sequence is built. --> | ||
| <button |
There was a problem hiding this comment.
suggestion: Missing type="button". Without it, the button defaults to type="submit" inside any ancestor <form> element. The sign-in page likely has a form wrapping the username/password fields; if this grid is placed inside that form, clicking the arrow would both emit submit and trigger a native form submission. Add type="button" to be safe.
| signInWithSequence$, | ||
| } = picturePasswordStrings; | ||
|
|
||
| const ICON_ENTRIES = Object.entries(PICTURE_PASSWORD_SET).map(([idStr, data]) => ({ |
There was a problem hiding this comment.
nitpick: ICON_ENTRIES is derived entirely from the static JSON import PICTURE_PASSWORD_SET — it doesn't depend on props, reactive state, or anything that changes at runtime. Defined inside setup(), it's recomputed on every component mount. Move it to module scope (above export default) to compute it once.
| const existingIndex = sequence.value.indexOf(id); | ||
| if (existingIndex >= 0) { | ||
| // Deselect: drop this icon and everything after it. | ||
| sequence.value = sequence.value.slice(0, existingIndex); |
There was a problem hiding this comment.
praise: sequence.value.slice(0, existingIndex) is exactly the right tool here — one expression drops the tapped icon and everything after it, enabling mid-sequence correction without a loop or splice. Clean.
| setup(props, { emit }) { | ||
| const $themeTokens = themeTokens(); | ||
| const $themePalette = themePalette(); | ||
| const { sendPoliteMessage } = useKLiveRegion(); |
There was a problem hiding this comment.
praise: Using useKLiveRegion from KDS rather than wiring a raw aria-live element directly into the component's markup is the right architectural call — it keeps the live-region DOM singleton in one place and lets KDS manage its lifetime. Consistent with how other components in the codebase handle screen-reader announcements.
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
cdb6696 to
6e9d2d4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Both suggestions from the prior round are addressed.
Prior findings
2 prior findings unchanged (1 resolved previously).
Newly resolved:
- Submit button missing
type="button"(suggestion) ← was UNADDRESSED —type="button"added at line 43 ICON_ENTRIESdefined insidesetup()(nitpick) ← was UNADDRESSED — moved to module scope at lines 83–86 with an explanatory comment
Acknowledged (not re-raised):
- Live region re-triggering via
useKLiveRegion(suggestion) — author replied: delegating to KDS is intentional
3/3 prior findings resolved or acknowledged. 0 re-raised.
CI: one check still in progress at review time (Build Assets). Visual inspection: reviewed 7 frames from the PR video — colorful and standard icon modes, badge sequence, disabled state, progress bar fill, and submit button all render correctly. No new findings.
@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
AlexVelezLl
force-pushed
the
picture-password-option
branch
2 times, most recently
from
f370217 to
e442c90
Compare
LianaHarris360
left a comment
LianaHarris360
left a comment
There was a problem hiding this comment.
I reviewed and tested the changes locally, the updates look good. I only had one visual and one accessibility concern to note. So that Radina can more thoroughly test the updates related to the keyboard and screen reader, can you temporarily keep PicturePasswordGrid in the SignInPage?
kolibri/plugins/user_auth/frontend/views/SignInPage/PictureLogin/PicturePasswordGrid.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/user_auth/frontend/views/SignInPage/PictureLogin/PicturePasswordOption.vue
Show resolved
Hide resolved
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
e442c90 to
ef56db4
Compare
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
ef56db4 to
e57b32d
Compare
|
Thanks @LianaHarris360, I have invoked the grid on the signing page, I have used the actual facility settings values so that we can test all combinations, and have hardcoded that the "correct" sequence is: bee, star, moon for testing purposes. Grabacion.de.pantalla.2026-04-09.a.la.s.10.18.54.a.m.mov |
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
e57b32d to
ea6b851
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
All 7 prior findings remain resolved or acknowledged — no changes since last review.
CI build-assets job is queued (no test failures). One new blocking finding on the index.vue QA commit.
- blocking:
index.vueQA scaffolding should be removed before merging — see inline
| Correct! Signed in successfully. | ||
| </p> | ||
| <!-- eslint-enable vue/no-bare-strings-in-template --> | ||
| <PicturePasswordGrid |
There was a problem hiding this comment.
blocking: This entire block (lines 48–69 + the handlePicturePasswordSubmit method) is QA-only scaffolding that shouldn't land on develop as-is:
- Unconditional rendering —
PicturePasswordGridis shown to every user on the sign-in page, with nov-ifguard checking whether picture password is enabled for the facility. Every learner will see a 12-icon grid they can't use. - Bare strings with
eslint-disable— "Incorrect sequence. Try again." and "Correct! Signed in successfully." are hard-coded English, bypassing i18n enforcement. - Inline styles —
style="color: red"/style="margin: 16px 0"can't be auto-flipped by RTLCSS; directional styles must live in<style>blocks. - Hardcoded stub auth —
handlePicturePasswordSubmittreats'1.2.3'as a successful login; the TODO comment confirms this is not real.
The component implementations (PicturePasswordGrid.vue, PicturePasswordOption.vue) are complete and ready. Please remove this scaffolding before merging — the SignInPage integration belongs in a separate PR once the backend authentication endpoint is wired up.
|
Points for improvement after testing with the screen reader:
|
|
Thanks @radinamatic! Will be checking each point and let you know any questions 👐. |
kolibri/plugins/user_auth/frontend/views/SignInPage/PictureLogin/PicturePasswordOption.vue
Show resolved
Hide resolved
| .picture-password-option { | ||
| display: inline-flex; | ||
| min-width: 44px; | ||
| min-height: 44px; |
There was a problem hiding this comment.
Just wanted to flag that the Figma displays the selections with a fixed width and height of 84px, even when the label is shown, which I believe was an intentional design. The issue did specify that the minimum 44x44 touch target should be met, so this is definitely correct, although it would meet those touch targets at 84x84 as well.
There was a problem hiding this comment.
But if we set a fixed size, this won't be responsive, then?
There was a problem hiding this comment.
That's true, and the size is 84x84 when the label isn't shown. I’m wondering if it's possible to keep the selection container the same size even when the label is added, while keeping it responsive? Even if the icons are a little smaller when the label is there.
There was a problem hiding this comment.
Hmm, I'd say we can play with the padding a bit to make it look more square, but I wouldn't feel that making it always perfectly square would be an option, because what happens if there is a language with a long word? We would want the icon to be smaller or prefer to break the word in a new line, even if that means the height will be larger.
There was a problem hiding this comment.
That's a good point. I agree, it's not worth adjusting the container to always be square in this case, ignore this suggestion!
There was a problem hiding this comment.
Playing with paddings, I got an 84px x 83.3px with label.
It can still grow vertically in the rare case that a translated word, or an accessibility modified font-size doesn't fit on a single line:
While keeping the 84px x 84px when the label is hidden.
Let me know your thoughts!
There was a problem hiding this comment.
I think this solution is the best combination for allowing the container to be square when the label is included and allowing it to adjust vertically for future cases, thank you for taking the time to look into this!
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
ea6b851 to
d52fb55
Compare
|
Hi @radinamatic! I have fixed points number 1 and 5. 2, 3, and 4 are not in the scope of this PR, since we are just temporarily presenting the grid on the sign-in page as a playground for you to be able to test it, but this will be removed before merging the PR. |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
7 prior findings unchanged (all resolved or acknowledged).
Still unaddressed (re-raised below):
index.vueQA scaffolding (blocking) — no reply, no code change since last review
7/8 prior findings resolved or acknowledged. 1 re-raised below.
CI mostly passing; Morango integration tests and platform builds still pending (same state as prior round, unrelated to frontend changes). Visual inspection unchanged — no new commits since last review.
kolibri/plugins/user_auth/frontend/views/SignInPage/index.vue:48–69 — blocking: QA scaffolding still present. Three issues remain:
PicturePasswordGridis rendered unconditionally for all users — nov-ifguard on the facility's picture-password feature flag. Every learner sees a 12-icon grid regardless of their facility settings.- Error/success messages (
"Incorrect sequence. Try again."/"Correct! Signed in successfully.") are bare English strings suppressed byeslint-disable. They bypass i18n enforcement entirely. - Inline styles (
style="color: red; text-align: center",style="margin: 16px 0").
This entire block and the corresponding handlePicturePasswordSubmit method in the script section need to be removed before merging to develop. The grid will be properly integrated with a facility guard in a follow-up issue.
@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
AlexVelezLl
force-pushed
the
picture-password-option
branch
from
d52fb55 to
3a5d840
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
7 prior findings unchanged (all resolved or acknowledged).
Still unaddressed (re-raised below):
index.vueQA scaffolding (blocking) — no reply, no code change since last review
7/8 prior findings resolved or acknowledged. 1 re-raised below.
CI passing. Visual inspection: reviewed 2 frames from the PR video — selection and progress-bar states still render correctly.
- blocking:
index.vueQA scaffolding re-raised (see inline)
@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
| Correct! Signed in successfully. | ||
| </p> | ||
| <!-- eslint-enable vue/no-bare-strings-in-template --> | ||
| <PicturePasswordGrid |
There was a problem hiding this comment.
blocking: (re-raised, 3rd round) This entire block (lines 48–69 + handlePicturePasswordSubmit at line 371) is QA-only scaffolding that must be removed before this lands on develop:
- Unconditional rendering —
PicturePasswordGridis shown to every user with nov-ifchecking whether picture password is enabled for the facility. - Bare strings with
eslint-disable— "Incorrect sequence. Try again." and "Correct! Signed in successfully." bypass i18n. - Inline styles —
style="color: red"/style="color: green"bypass theme tokens. - Hardcoded sequence check —
handlePicturePasswordSubmitaccepts only'1.2.3'as correct; it's not real authentication.
The picturePasswordIconStyle and picturePasswordShowIconText computed properties and the wrongSequence data field are legitimate and can stay. Remove everything else added to this file and leave a <!-- TODO: integrate PicturePasswordGrid once backend auth is wired --> comment if the integration point needs marking.
There was a problem hiding this comment.
This is for QA only, it will be removed before merging the PR.
LianaHarris360
dismissed
their stale review
All requested changes have been addressed on my end, this will be ready to approve & merge after QA passes.
4 participants
Summary
picturePasswordStrings.jsfile.PicturePasswordOptionandPicturePasswordGrid.Grabacion.de.pantalla.2026-04-07.a.la.s.3.44.07.p.m.mov
For now, hadn't actually implemented anything specific for specific screen breakpoints, given that it will depend on the parent's available space, but I have implemented the grid so that icons are responsive to the available width, and if we ever consider changing the grid layout for smaller screens, we can do it easily:
Grabacion.de.pantalla.2026-04-07.a.la.s.3.45.37.p.m.mov
References
Closes #14409.
Closes #14410.
Reviewer guidance
AI usage
Asked Claude to know what the most accessible DOM node layout was for this, and asked him to write the unit tests, which I then tweaked to use the most recent test guidelines.