Responsive picture password sign in#14682
Responsive picture password sign in#14682AlexVelezLl wants to merge 3 commits intolearningequality:developfrom
Conversation
github-actions
Bot
added
DEV: backend
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
Build Artifacts
Smoke test screenshot |
AlexVelezLl
force-pushed
the
responsive-picture-login
branch
from
47caf67 to
d70fa39
Compare
AlexVelezLl
force-pushed
the
responsive-picture-login
branch
from
d70fa39 to
4a7361d
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All acceptance criteria from #14662 are met. CI passing (WHL build still in progress, no test/lint failures).
Visual inspection: compared against the Figma-based mockups in the issue. Landscape layout matches — facility name + back arrow left, Kolibri logo+title top-right, responsive icon columns, inline footer links with separator divider. Portrait layout is unchanged. The 4-column result on Surface Duo (720px landscape) and 6-column result on a full-size tablet are both consistent with the column formula.
Findings:
- praise — see inline comment on
useKResponsiveElementusage - suggestion — redundant
v-ifon slot content (see inline,PictureSignInPage.vueline 9) - suggestion —
isTouchDeviceon hybrid devices (see inline, line 94) - suggestion — no test coverage for
columnslogic (see inline,PicturePasswordGrid.vueline 134) - nitpick — inline
style="margin-top: 0"(line 10) - nitpick — generic CSS-property class names in
AuthBase.vue(line 552)
@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 $themeTokens = themeTokens(); | ||
| const $themePalette = themePalette(); | ||
| const { sendPoliteMessage } = useKLiveRegion(); | ||
| const { elementWidth } = useKResponsiveElement(); |
There was a problem hiding this comment.
praise: Exactly the right tool here — useKResponsiveElement follows the project rule of using responsive-element composables instead of CSS @media queries, gets JSDOM-safe behaviour for free in tests (ResizeObserver is absent so elementWidth stays 0, which falls back cleanly to 3 columns), and registers/unregisters the observer via onMounted/onBeforeUnmount without any manual cleanup needed.
| > | ||
| <template #header-leading-actions> | ||
| <AuthContextHeading | ||
| v-if="landscapeLayout" |
There was a problem hiding this comment.
suggestion: The v-if="landscapeLayout" here is redundant — AuthBase already gates this slot's wrapper <div> with the same condition (see AuthBase.vue line 19). When landscapeLayout is false the slot container is never rendered, so the content guard never fires. Safe to remove the v-if from AuthContextHeading here.
| <template #header-leading-actions> | ||
| <AuthContextHeading | ||
| v-if="landscapeLayout" | ||
| style="margin-top: 0" |
There was a problem hiding this comment.
nitpick: Project convention (AGENTS.md) puts non-dynamic styles in <style> blocks. margin-top: 0 could be extracted as a modifier class (e.g., .landscape-heading { margin-top: 0; }) in the scoped style block. Although margin-top isn't directional so there's no RTL risk, keeping it out of inline styles is consistent with the rest of the component.
| }); | ||
|
|
||
| const landscapeLayout = computed(() => { | ||
| return windowIsLandscape.value && isTouchDevice; |
There was a problem hiding this comment.
suggestion: isTouchDevice is a boolean constant evaluated once at module load — if a user has a touchscreen at all (including hybrid laptops like Surface Pro or an iPad with a keyboard case), landscapeLayout will activate in landscape even when they're using keyboard/mouse. The PR notes this is intentional for touch-only devices; worth a brief comment here so future readers understand why this shorthand was chosen and don't remove it thinking it's a simpler alternative to a proper tablet detection. If the goal is specifically "tablet held in hand", a note confirming isTouchDevice is the agreed proxy would help.
|
|
||
| // How many columns can we fit given the current width of the component? | ||
| // Pick the largest allowed number of columns that will fit. | ||
| const columns = computed(() => { |
There was a problem hiding this comment.
suggestion: The elementWidth → columns mapping introduced here has no test coverage. useKResponsiveElement isn't mocked in PicturePasswordGrid.spec.js, so elementWidth.value is always 0 in tests and columns always falls back to 3 — the new logic is silently untested. Consider adding a test that mocks useKResponsiveElement to return specific widths (e.g. 240 → 3, 320 → 4, 480 → 6) and asserts the resulting grid-template-columns inline style on the .icon-grid element.
| } | ||
| } | ||
|
|
||
| .flex-row-reverse { |
There was a problem hiding this comment.
nitpick: .flex-row-reverse and .flex-space-between (line 559) name CSS properties rather than the layout's purpose. Semantic names like .logo-title-header and .header-row would make the template easier to read without needing to cross-reference the style block. .footer-links-landscape nearby is a good example of the naming pattern to follow.
3 participants
Summary
Display a 4 or 6-column grid for picture login sign-in in landscape mode, depending on available space.
columnsnumber calculated on PicturePasswordGrid depending on available space. Given that the login card in portrait mode has a fixed value, it will always only show 3 columns on that layout.AuthBasecomponent update to match the layout for landscape mode, when alandscapeLayoutprop is passed. Adds#header-leading-actionsslot so that pages can render elements next to the Kolibri logo.PicturePasswordSignInPagemodified to set thelandscapeLayoutprop to true when picture password sign-in is displayed in landscape mode.Grabacion.de.pantalla.2026-05-07.a.la.s.2.30.49.p.m.mov
References
Closes #14662.
Notes
columnsto consider a 4-column layout too, because on small devices, there was the possibility of getting a layout like this, where icons couldn't shrink further (we could still play with the gap, but that was going to be a bit more complex IMO, and wouldn't probably handle all cases):