[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. |
🔵 Review postedLast updated: 2026-08-04 12:46 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.
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.
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.
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.
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.
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.
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.
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.
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.
2 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.