✨ Added support for email autocomplete when signing up / subscribing.#28321
✨ Added support for email autocomplete when signing up / subscribing.#28321chipironcin wants to merge 4 commits into
Conversation
This change was suggested by an user in X. Improves user experience by autocompleting the last part of an email address (email provider) based on the characters being typed by the user.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
WalkthroughThis PR adds email domain autocomplete to the signup form. The 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/portal/src/components/pages/signup-page.js (1)
785-790: ⚡ Quick winRemove the dead
listprop fromInputFormhere.
InputFormdoesn't read a top-levellistprop; the working contract is alreadyfield.listviagetInputFields(). Keeping both makes it look likeInputFormsupports two wiring paths when only one is real.Suggested cleanup
<InputForm fields={fields} onChange={(e, field) => this.handleInputChange(e, field)} onKeyDown={e => this.onKeyDown(e)} - list="email-domains" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/portal/src/components/pages/signup-page.js` around lines 785 - 790, Remove the dead top-level list prop passed to InputForm here: InputForm currently reads per-field lists via field.list (as produced by getInputFields()), so drop the extraneous list="email-domains" from the InputForm call to avoid implying a second wiring path; ensure only fields={fields}, onChange and onKeyDown remain so InputForm continues to rely on field.list populated by getInputFields().apps/portal/test/unit/components/common/input-field.test.js (1)
10-10: ⚡ Quick winAssert the new
listcontract directly in this unit test.Right now this setup passes
list: 'data-list', but nothing here fails ifInputFieldstops applying it to the DOM. A direct attribute assertion keeps this test aligned with the API you just added.Suggested assertion
test('renders', () => { const {inputEl} = setup(); expect(inputEl).toBeInTheDocument(); + expect(inputEl).toHaveAttribute('list', 'data-list'); });Also applies to: 27-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/portal/test/unit/components/common/input-field.test.js` at line 10, The test currently passes list: 'data-list' into the InputField props but never asserts that the rendered input actually receives the list attribute; update the unit test for InputField to directly query the rendered input element (the element rendered by the InputField component) and assert that it has attribute list="data-list" (and add the same assertion covering the other case around lines 27-30) so the spec fails if the component stops applying the list prop to the DOM.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/portal/src/components/pages/signup-page.js`:
- Around line 785-790: Remove the dead top-level list prop passed to InputForm
here: InputForm currently reads per-field lists via field.list (as produced by
getInputFields()), so drop the extraneous list="email-domains" from the
InputForm call to avoid implying a second wiring path; ensure only
fields={fields}, onChange and onKeyDown remain so InputForm continues to rely on
field.list populated by getInputFields().
In `@apps/portal/test/unit/components/common/input-field.test.js`:
- Line 10: The test currently passes list: 'data-list' into the InputField props
but never asserts that the rendered input actually receives the list attribute;
update the unit test for InputField to directly query the rendered input element
(the element rendered by the InputField component) and assert that it has
attribute list="data-list" (and add the same assertion covering the other case
around lines 27-30) so the spec fails if the component stops applying the list
prop to the DOM.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b813fb9-e63f-4a67-b268-c1ff914b34c8
📒 Files selected for processing (5)
apps/portal/src/components/common/input-field.jsapps/portal/src/components/common/input-form.jsapps/portal/src/components/pages/signup-page.jsapps/portal/test/unit/components/common/input-field.test.jsapps/portal/test/unit/components/pages/signup-page.test.js
|
Hey @chipironcin, thanks for the suggestion. Most users have some form of autofill from their own credential managers these days, so this feels unnecessary for Ghost and may add frustration with more 'popup' sort of behavior. |
This change was suggested by an user in X. Improves user experience by autocompleting the last part of an email address (email provider) based on the characters being typed by the user.
Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Please check your PR against these items:
We appreciate your contribution! 🙏