[Remove Vuetify from Studio] 'Forgot password' page - #6131
Vaibhavkumar-tech wants to merge 5 commits into
Conversation
|
👋 Hi @Vaibhavkumar-tech, 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. |
|
@akolson I've opened the PR with the changes for the Forgot Password page. I've kept the changes limited to the page as requested and followed the validation approach discussed above. When you have a chance, could you please review it and let me know if any changes or improvements are needed? Thank you! |
There was a problem hiding this comment.
Hi @Vaibhavkumar-tech! Great changes on this. Code changes makes sense to me! However, it appears that there is a noticeable/major change(and not minor as suggested) in the sizing of the field and submit button comparing with what is currently present in production. Applying appropriate css styling should resolve this.
UI in production
UI from pr
|
@akolson Thanks for pointing that out! To ensure the layout matches production I will correct it . |
|
@akolson Thanks for pointing this out. I’ve updated the CSS styling to match the production sizing for the email field and submit button. The changes are pushed to the PR. Please take another look when you have a chance. |
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesLast updated: 2026-09-30 07:29 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6131: CI fails on lint and on the existing spec. The form also overflows narrow screens. The component swaps match the issue.
CI: Linting and Javascript Tests fail (inline).
Manual QA: happy path, trimming, validation, failure banner, keyboard and RTL desktop all work. Axe reports 0 violations.
- blocking:
vue/order-in-componentslint error - blocking:
forgotPassword.spec.jsnot updated - blocking: form overflows at 320–360px
- suggestion: simpler error display, as in
RequestNewActivationLink.vue - suggestion: empty email lost "This field is required"
- suggestion: new strings drop existing Arabic translations
- suggestion: failure banner not announced
- suggestion: validation and banner text centred
- suggestion:
.w-100override andmb-4 - nitpick: relative
sharedimports, unusedref
@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
|
|
||
| mixins: [generateFormMixin(formFields)], | ||
|
|
||
| components: { |
There was a problem hiding this comment.
blocking: Linting CI fails: The "components" property should be above the "mixins" property on line 62 vue/order-in-components. Move mixins below components, as RequestNewActivationLink.vue does.
| {{ $tr('forgotPasswordFailed') }} | ||
| </StudioBanner> | ||
|
|
||
| <StudioEmailField |
There was a problem hiding this comment.
blocking: accounts/pages/__tests__/forgotPassword.spec.js fails. It queries getByLabelText('Email'), but the new field's label is "Email address". Reproduced locally: 1 failed, 4 passed. The AC asks for the suite to be meaningfully updated.
- Pass
:labelto keep "Email" (asAccountsMain.vuedoes), or update the query. - Add VTL cases: error shown for empty and invalid input on submit, no request sent.
| <style lang="scss" scoped> | ||
|
|
||
| .forgot-password-form { | ||
| width: 400px; |
There was a problem hiding this comment.
blocking: The form is cut off at 320px and 360px widths. The form, field and Submit stay 400px wide. LTR loses the right edge; /ar/ loses the left edge. This fails WCAG 1.4.10 Reflow.
- Cause:
StudioMessageLayout's.message-slot-containershrinks to its content, somax-width: 100%here resolves against 400px. - Fix: set
.message-slot-container { width: 100%; }(QA: 328px form at 360px, still 400px on desktop), or drop the fixedwidthhere. - Screenshots:
.reviews/qa-s6-360.png,.reviews/qa-s6-rtl-360.png.
| autofocus | ||
| class="email-field w-100" | ||
| :error-messages="emailErrors" | ||
| @blur="showEmailError" |
There was a problem hiding this comment.
suggestion: The email error hides inconsistently. A mouse click hides it, but tabbing into the field does not. Every keystroke also hides it. RequestNewActivationLink.vue:23 shows the error from errors.email alone; AccountsMain.vue:62 uses a touched flag. Replace emailValidationVisible and its three handlers with one of those patterns. Also use :errorMessages (camelCase), like every other call site.
| return []; | ||
| } | ||
|
|
||
| return [this.$tr('validEmailMessage')]; |
There was a problem hiding this comment.
suggestion: An empty email now shows "Please enter a valid email" instead of "This field is required". That breaks the "no functional differences" AC. Handle both cases as emailErrorText does in RequestNewActivationLink.vue:66-72.
| <Banner | ||
| :text="$tr('forgotPasswordFailed')" | ||
| :value="error" | ||
| <StudioBanner |
There was a problem hiding this comment.
suggestion: Screen-reader users hear nothing when the failure banner appears. It has no live region. Add role="alert", as AccountsMain.vue:36 does. Also replace the Vuetify mb-4 helper with a scoped class (.banner { margin-bottom: 16px } in RequestNewActivationLink.vue).
|
|
||
| <style lang="scss" scoped> | ||
|
|
||
| .forgot-password-form { |
There was a problem hiding this comment.
suggestion: The validation message and banner text are centred, while the label and input are left-aligned (.reviews/qa-s2-invalid.png). The form inherits text-align: center from StudioMessageLayout. Add text-align: left; here; RTLCSS flips it.
| .w-100 { | ||
| width: 100%; | ||
| display: block; | ||
| width: 100% !important; |
There was a problem hiding this comment.
suggestion: This override also forces display: block and !important onto the KButton. AccountsMain.vue:63 and Create.vue:70 pass :appearanceOverrides="{ maxWidth: '100%' }" to StudioEmailField instead. Use that and restore plain width: 100%.
| import Banner from 'shared/views/Banner'; | ||
| import StudioMessageLayout from '../../components/StudioMessageLayout'; | ||
| import StudioEmailField from '../../components/form/StudioEmailField'; | ||
| import StudioBanner from '../../../shared/views/StudioBanner'; |
There was a problem hiding this comment.
nitpick: Use the aliases shared/views/StudioBanner and shared/mixins, as sibling pages do. ref="form" (line 8) is unused now; remove it.
| this.emailValidationVisible = true; | ||
| }, | ||
|
|
||
| submit() { |
There was a problem hiding this comment.
praise: Validation and the payload both use the same trimmed email. QA confirmed a padded email posts as a@a.com.
3 participants
Summary
Migrated the Forgot Password page from the legacy Vuetify components to the Studio/KDS components.
MessageLayoutwithStudioMessageLayout.EmailFieldwithStudioEmailField.BannerwithStudioBanner.VFormand its validation usage.References
Reviewer guidance
Manual verification
UI
Minor visual differences from the previous Vuetify implementation are expected because the page now uses Studio/KDS components.
AI usage
I used ChatGPT to help understand the existing Vuetify implementation, the Studio components, and the project's form-validation pattern. I reviewed the generated suggestions critically, compared them with the existing code and maintainer guidance, and kept the implementation limited to the Forgot Password page as requested.