🎨 Changed signup-form email input to use 'email' type - #25648
Conversation
WalkthroughFormView now generates an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between ca876fb24a54a8ccec4ca4d799c914ca19ffc6fb and 278ee2f. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/signup-form/src/components/pages/form-view.tsx (1)
62-68: Email input semantics and validation configuration look correctAdding
noValidate={true}on the form while switching the field totype="email"cleanly achieves the PR goal (better autofill/mobile keyboards) without letting native browser validation interfere with your existing custom validation flow insubmitHandler/onSubmit. As an optional follow‑up, consider wiring the input to the error message via something likearia-invalid={!!error}andaria-describedbyon the input plus anidon the error<p>so that, with native validation disabled, assistive tech still gets a clear association between the field and its error state.apps/signup-form/test/e2e/form.test.ts (1)
168-172: Switch totoHaveTextstrengthens error message assertionsUsing
await expect(errorMessage).toHaveText(...)for the invalid‑email, network‑error, and 4xx cases is a solid improvement over asserting oninnerText(): you get Playwright’s auto‑waiting and clearer diagnostics while keeping the intent of the tests unchanged. If these strings are likely to evolve, you could optionally relax them totoContainTextor a regex, but the current exact matches are perfectly fine if the copy is stable. Based on learnings, this aligns well with the recommended Playwright‑style assertions for e2e tests.Also applies to: 205-208, 228-232
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6606ab7 and fe54b0775a41bab91c9e38348d3882d1c3dd8917.
📒 Files selected for processing (2)
apps/signup-form/src/components/pages/form-view.tsx(1 hunks)apps/signup-form/test/e2e/form.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Prefer semantic locators (`getByRole`, `getByLabel`, `getByText`) over test IDs and never use CSS selectors, XPath, nth-child, or class names
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use CSS/XPath selectors - only use semantic locators or data-testid
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Prefer semantic locators (`getByRole`, `getByLabel`, `getByText`) over test IDs and never use CSS selectors, XPath, nth-child, or class names
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test names should be lowercase and follow the format 'what is tested - expected outcome'
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use CSS/XPath selectors - only use semantic locators or data-testid
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Always follow ADRs in `../adr/` folder (ADR-0001: AAA pattern, ADR-0002: Page Objects)
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Follow the AAA (Arrange, Act, Assert) pattern in test structure
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Manual login should not be used - authentication is automatic via fixture
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use hard-coded waits like `waitForTimeout()`
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Applied to files:
apps/signup-form/test/e2e/form.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Use `waitFor()` for guards in Page Objects, never use `expect()` in page objects
Applied to files:
apps/signup-form/test/e2e/form.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Signup-form tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/signup-form/src/components/pages/form-view.tsx (1)
16-18: Fix inconsistentaria-describedbywiring in minimal layoutIn non-minimal mode the
id="error-message"on the paragraph at Line 33 correctly matchesaria-describedby={error ? 'error-message' : undefined}on the input, but in the minimal branch (Line 17) the error<p>lacks thisid. WhenisMinimalis true anderroris set, the input getsaria-describedby="error-message"but there is no element with that id, which is an accessibility issue.You can align both layouts by adding the same
idin the minimal error paragraph:- {error && <p className='text-red-500' data-testid="error-message">{error}</p>} + {error && <p className='text-red-500' data-testid="error-message" id="error-message">{error}</p>}Also applies to: 31-35, 62-65
🧹 Nitpick comments (1)
apps/signup-form/src/components/pages/form-view.tsx (1)
59-66: Form/input changes look consistent; consider minor ARIA refinements
noValidate={true}on the<form>plustype="email"on the input correctly gives you email-specific UX (keyboard, heuristics) while leaving validation to your custom logic.aria-describedbyandaria-invalidwiring is sound once the erroridis present in both layouts.Optionally, you could:
- Set
aria-invalidonly when there is an error, e.g.aria-invalid={!!error}(React will emit"true"only when needed).- If this form might ever be rendered multiple times on a page, derive a unique
errorIdviaReact.useId()and use that instead of a hard-coded"error-message".Also applies to: 70-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between fe54b0775a41bab91c9e38348d3882d1c3dd8917 and 088094986d2eb7a2d93c97d79a3c3a55e642f9d2.
📒 Files selected for processing (1)
apps/signup-form/src/components/pages/form-view.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Signup-form tests
- GitHub Check: Build & Push
0c0034e to
ca876fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/signup-form/src/components/pages/form-view.tsx (1)
67-68: Well-implemented accessibility and input type change.The changes correctly implement the PR objectives:
type="email"enables browser autofill and presents an email-focused keyboard on mobile devicesaria-describedbyproperly links the input to its error message only when an error existsaria-invalidaccurately reflects the error state for screen readersOptional enhancement: Consider adding
autoComplete="email"to further improve the autofill experience:<input aria-describedby={error && errorMessageId ? errorMessageId : undefined} aria-invalid={!!error} + autoComplete="email" className={`w-full px-2 py-1 focus-visible:outline-none disabled:bg-white xs:p-2`}Also applies to: 73-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 0c0034ef0164992f53e5e6191e3ea50442c7a157 and ca876fb24a54a8ccec4ca4d799c914ca19ffc6fb.
📒 Files selected for processing (1)
apps/signup-form/src/components/pages/form-view.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/signup-form/src/components/pages/form-view.tsx (1)
apps/signup-form/src/utils/helpers.tsx (1)
isMinimal(12-14)
🔇 Additional comments (4)
apps/signup-form/src/components/pages/form-view.tsx (4)
13-14: Excellent use of React.useId() for accessibility!Generating a stable unique ID for the error message enables proper ARIA linkage between the input field and its error description. This improves screen reader support.
18-19: Proper error message linking in both rendering modes.Both minimal and standard views correctly propagate
errorMessageIdto the Form component and apply it as theidfor error paragraphs. This establishes the necessary linkage foraria-describedbyin the input field. The different rendering approaches (conditional vs. visibility toggle) both work correctly with the accessibility attributes.Also applies to: 34-35
49-49: Type definitions correctly updated.The
FormPropstype andFormcomponent signature properly includeerrorMessageIdto support the accessibility enhancements.Also applies to: 53-53
65-65: Correctly disables browser validation.Adding
noValidate={true}appropriately disables HTML5 validation since the form implements custom validation handling, as noted in the PR objectives.
closes https://linear.app/ghost/issue/BER-3092/ - using `type="email"` improves usability by allowing browsers to auto-fill and display an email-focused keyboard on mobile - added `noValidate={true}` to disable browsers built-in validation as we already have custom validation handling
no issue - improves markup for assistive tech
ca876fb to
278ee2f
Compare
2 participants
closes https://linear.app/ghost/issue/BER-3092/
type="email"improves usability by allowing browsers to auto-fill and display an email-focused keyboard on mobilenoValidate={true}to disable browsers built-in validation as we already have custom validation handling