[Remove Vuetify from Studio] Sign-in page - #6056
Conversation
Replace VCard with StudioRaisedBox, Banner with StudioBanner, and EmailField / PasswordField with StudioEmailField / StudioPasswordField. Drop VApp, VLayout and VDivider in favour of plain elements and scoped styles, and swap VForm for a native form driven by generateFormMixin. Field errors are surfaced on blur or after a failed submit, preserving the previous validate-on-blur behaviour. Vuetify spacing and colour helpers are replaced with scoped CSS and KDS theme tokens.
Extend the sign-in suite for the behaviour introduced by the move to generateFormMixin: submission is blocked while the form is invalid, field errors stay hidden until a field is blurred or a submit fails, and the password is sent without its surrounding whitespace trimmed. Also assert the offline banner renders.
|
👋 Hi @LightCreator1007, 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. |
🟡 Waiting for changesLast updated: 2026-08-18 08:16 UTC |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6056 — the Vuetify removal is faithful: spacing helpers map to the same pixel values, --v-backgroundColor-base and $themePalette.grey.v_100 resolve identically, and the form now follows the generateFormMixin + Studio*Field pattern Create.vue established. No new Vuetify, no ::v-deep, no inline directional styles. CI passing.
Main gaps are accessibility — the three feedback paths (login failure, offline, validation failure) are all silent to a screen reader — plus a few load-bearing decisions that survive only in the PR description and will be undone by the next reader.
- important: error banners render with no live region (inline, line 27); validation failure gives no announcement and no focus move (inline, line 211)
- suggestion:
theme--lightretention, thethis.passwordbypass, and the barereturnon network errors all need in-code comments (inline) - suggestion: the
touchedblur gate diverges fromCreate.vue— worth an epic-level decision (inline, line 52) - nitpick:
fireEvent.blurin an otherwiseuserEventsuite (inline, line 112)
Not verified: manual QA did not run, so nothing here rests on how the page actually renders. Worth eyeballing the fixed width: 300px card and the bullet-separated footer links at ~320px, and confirming PolicyModals / LanguageSwitcherModal still centre now that they sit in a flex container rather than a VApp.
Comments on lines not in diff:
AccountsMain.vue:154 — nitpick: validEmailMessage (/.+@.+\..+/) duplicates Create.vue's emailValidationMessage (/\S+@\S+\.\S+/) with identical English text and a slightly different notion of validity. Matching legacy EmailField is a fair justification for this PR; worth aligning when the accounts pages are next touched together.
AccountsMain.vue:266 — nitpick: overflow: auto carried over from .main, where it sat on a Vuetify fill-height layout. .page grows with its content, so it never scrolls.
@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
| </template> | ||
| <template #main> | ||
| <div class="card-body"> | ||
| <StudioBanner |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: These banners are gated by v-if and StudioBanner renders a plain <div class="banner"> with no role/aria-live (shared/views/StudioBanner.vue:3-9), so the node and its text are inserted together. Submit wrong credentials and focus stays on the sign-in button — a screen reader user gets nothing. Same for the offline banner, which can appear mid-session when shared/vuex/connectionPlugin/index.js:17-19 dispatches handleDisconnection.
Because v-if rules out a pre-existing live region, role="alert" at the call site is the cheap fix — StudioBanner's root is a plain div with default inheritAttrs, so it lands without touching the shared component:
<StudioBanner
v-if="loginFailed"
role="alert"
error
>Applies to the offline banner too. The loginToProceed banner is present at mount and correctly needs no role.
| } | ||
| return Promise.resolve(); | ||
| // eslint-disable-next-line vue/no-unused-properties | ||
| onValidationFailed() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: onValidationFailed only flips the touched flags — no announcement, no focus move. Tab to "Sign in" on an empty form and press Enter: both KTextboxes render their invalid text, but invalidText is associated with its own input, so it reaches a user focused on the field and not a user standing on the button. The press appears to do nothing.
Create.vue:503-512 handles the equivalent case by setting valid = false (rendering a summary StudioBanner) and scrolling ref="top" into view. With only two fields here, moving focus to the first invalid one is more direct:
onValidationFailed() {
this.touched.username = true;
this.touched.password = true;
this.$nextTick(() => {
const firstInvalid = this.$el.querySelector('[aria-invalid="true"]');
if (firstInvalid) firstInvalid.focus();
});
},A summary banner carrying role="alert", matching Create.vue, works too.
| style="width: 300px; margin: 0 auto" | ||
| > | ||
| <div | ||
| class="page theme--light" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: theme--light is load-bearing and non-obviously so — it's the only occurrence in the whole frontend, on a page whose stated purpose is to be Vuetify-free. shared/vuetify/theme.js spreads KDS themeTokens() into the Vuetify theme, so Vuetify 1.5 generates a global .link { background-color } helper that collides with the five appearance="basic-link" KButtons here; the only suppression is shared/styles/main.scss:100-115, nested under .theme--light. The theme stylesheet is still injected because accounts/components/MessageLayout.vue:3 uses VApp, so the failure only shows after sign-in → forgot-password → back. A cleanup pass will read this as leftover and delete it. One comment naming main.scss and the .link collision prevents that.
On the alternative you offered in the description: I'd take the main.scss un-nesting. Every remaining sub-issue of #5060 hits this same wall, and the outcomes are either theme--light copied onto every de-Vuetified page root or a central fix. Un-nesting is one line on a rule that exists solely to undo Vuetify damage, and the blast radius is bounded (no dark theme, KDS links carry no background). Keeping this PR strictly in-scope is defensible — but then file the main.scss fix and reference it in the comment so the workaround has an expiry date.
| this.busy = true; | ||
| const credentials = { | ||
| username: formData.username, | ||
| password: this.password, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: formData.username for one field and this.password for the other reads as an inconsistency, and someone will "tidy" it. The reason — clean() in shared/mixins.js:433-446 trims every non-multiSelect value — deserves a one-line comment here.
Worth a clause in the same comment: validate() (mixins.js:447) runs against the cleaned data, so password's default Boolean(v) validator (mixins.js:362-364) now tests the trimmed value. An all-whitespace password is rejected client-side where the old PasswordField required-rule accepted it. Vanishingly rare, but it's the one input the trimming still touches.
| .catch(err => { | ||
| this.busy = false; | ||
| if (err.message === 'Network Error') { | ||
| return; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Dropping loginFailedOffline is a genuine cleanup — it was dead state, the template bound the offline banner to offline from the store. But an empty branch with no explanation is indistinguishable from an accidentally-eaten error. A comment ("the offline banner is driven by state.connection.online, no local flag needed") and dropping the else after the return would make that legible.
| <StudioEmailField | ||
| v-model="username" | ||
| autofocus | ||
| :errorMessages="touched.username && errors.username ? [usernameErrorText] : []" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The blur gate is well-motivated — the legacy fields passed :validate-on-blur="!validate", whereas the mixin's computed setters (shared/mixins.js:406-413) mark errors on every keystroke, so without touched this page would regress to live per-character errors.
The consistency problem is that Create.vue — same directory, same Studio*Field components, same mixin, same epic — has no gate and does show email errors while typing. Two adjacent account forms now validate differently, and each future migration re-hand-rolls this touched object. Not something to fix by expanding this PR, but worth deciding at the epic level: does the gate belong inside StudioEmailField/StudioPasswordField, which already own hasError/errorText? A note on #5060 would be enough.
One behavioural note either way: once blurred, touched stays true, so errors then update on every keystroke — different from the old validate-on-blur, and arguably better. Flagging only so it's deliberate.
| const emailField = screen.getByLabelText(/email/i); | ||
|
|
||
| await user.type(emailField, 'not-an-email'); | ||
| await fireEvent.blur(emailField); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: fireEvent.blur dispatches a synthetic blur without moving focus, so document.activeElement is still the email input after the test claims the field lost focus — it would pass even if the real blur path broke for focus-related reasons. await user.tab() blurs for real and keeps the suite on one interaction model. It's also the only reason fireEvent was added to the import on line 1.
While in here: line 83's comment still says "from EmailField and PasswordField components", which this branch renamed.
| expect(screen.getByText(/you seem to be offline/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should preserve leading and trailing whitespace in the password', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Pins exactly the trap the clean()-vs-this.password asymmetry creates, asserting on the dispatched payload rather than component internals — this is what stops a future consistency cleanup from locking users out.
|
Hi @LightCreator1007! Have you by any chance looked into the feedback provided by @rtibblesbot? |
Hi @akolson ! 😁 |
Thank you! |
…ments Add role="alert" to the login-failed and offline banners, move focus to the first invalid field on validation failure, and document the load-bearing theme--light class, the untrimmed password, and the offline early-return that the review flagged as needing explanation.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6056 — 6 of 8 prior findings resolved, 2 acknowledged; 1 still open, plus 1 new suggestion.
The focus and announcement behaviour is real: removing the $nextTick block in onValidationFailed fails exactly the two new focus tests. CI passing. Manual QA did not run for this round, so nothing here is a visual verification.
- suggestion (inline) — two of the four new tests duplicate the assertion they sit next to.
- suggestion, still open —
AccountsMain.vue:61: thetouchedblur gate still diverges fromCreate.vuewith no note on #5060. Epic-level bookkeeping, outside this PR.
Prior-finding status
RESOLVED — AccountsMain.vue:34 — banners need role/aria-live
RESOLVED — AccountsMain.vue:220 — onValidationFailed makes no announcement and moves no focus
RESOLVED — AccountsMain.vue:251 — empty Network Error branch needs a comment; drop the else
RESOLVED — accountsMain.spec.js:112 — fireEvent.blur does not move focus
RESOLVED — accountsMain.spec.js:154 — praise, whitespace-password payload test
ACKNOWLEDGED — AccountsMain.vue:11 — theme--light is load-bearing and non-obviously so
ACKNOWLEDGED — AccountsMain.vue:239 — formData.username vs this.password asymmetry needs a comment
UNADDRESSED — AccountsMain.vue:61 — touched blur gate diverges from Create.vue
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| expect(screen.getByText(/you seem to be offline/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should announce the offline banner to screen readers', () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: getByRole('alert')).toHaveTextContent(/you seem to be offline/i) strictly subsumes the getByText assertion in the test above — it fails on both a missing banner and a missing role — and the setup is identical. Same pair at 186-199 vs 170-184, where the duplicated part is a four-step login-failure setup plus its rejected mock.
Folding the role assertion into the existing test keeps the coverage and drops two render calls:
it('should show error message when login fails', async () => {
...
await waitFor(() => {
expect(screen.getByRole('alert')).toHaveTextContent('Email or password is incorrect');
});
});The two focus tests are not in this category — each pins a distinct new branch.
…rningequality#5060 The offline and login-failure role="alert" assertions strictly subsume the getByText checks in the tests they duplicated, so merge each pair. Document the touched blur gate as an epic-level decision instead of fixing it locally.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6056 — 9 of 9 prior findings resolved; none open. No new findings in this delta. CI passing; manual QA did not run, so this is a comment rather than an approval.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/AccountsMain.vue:34 — banners gated by v-if render with no role="alert"
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/AccountsMain.vue:224 — onValidationFailed makes no announcement and moves no focus
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/AccountsMain.vue:11 — theme--light is load-bearing and non-obviously so
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/AccountsMain.vue:243 — formData.username vs this.password asymmetry reads as a bug
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/AccountsMain.vue:255 — dropping the loginFailedOffline local flag needs a note
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/AccountsMain.vue:61 — blur gate diverges from Create.vue; now documented in-code and tracked on #5060
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/tests/accountsMain.spec.js — fireEvent.blur does not move focus
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/tests/accountsMain.spec.js — getByRole('alert') assertion subsumes the getByText duplicate
RESOLVED — contentcuration/contentcuration/frontend/accounts/pages/tests/accountsMain.spec.js:148 — praise, whitespace-preservation test pins the clean() trap
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
3 participants
Summary
Removes Vuetify from the sign-in page.
VCard→StudioRaisedBox,Banner→StudioBanner,EmailField/PasswordField→StudioEmailField/StudioPasswordFieldVForm→ native<form>withgenerateFormMixin;VApp/VLayout/VDivider→ plain elements + scoped styles$themePalette/$themeTokensclean()would otherwise strip leading/trailing spaces<h1>, styled the same, so the page doesn't start at level 2References
Fixes #5930
Reviewer guidance
Screen.Recording.2026-07-31.at.12.56.04.AM.mov
Note
theme--lighton the page root is retained. Vuetify generates a.linkclass that collides with KDS's basic-link class,main.scssonly neutralises it under.theme--light, whichVAppused to supply. Without it, links render as blue blocks after anyVApppage mounts.Alternative is un-nesting that rule in
main.scss, fixes it centrally for all of the pages/components, happy to switch.AI usage
Used Claude Code in a review-and-iterate loop.