-
Notifications
You must be signed in to change notification settings - Fork 0
A11y focus refs #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
A11y focus refs #29
Conversation
…d focusing invalid fields.
…and updating example components.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds accessibility features to enable form error handling: a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/a11y.js (1)
42-47: Consider handling equal nodes in the comparator.When
nodeA === nodeB,compareDocumentPositionreturns0, so the bitwise check fails and the function returns1. For a stable sort, equal elements should return0. This is unlikely in practice but worth handling for correctness.function byDomOrder(a, b) { const nodeA = a.ref.current const nodeB = b.ref.current if (!nodeA || !nodeB) return 0 + if (nodeA === nodeB) return 0 return nodeAPrecedesNodeB(nodeA, nodeB) ? -1 : 1 }readme.md (1)
173-184:useFormFieldrefdocs look consistent; consider explicitly tying it to the DOM elementThe additions documenting
refin theuseFormFieldreturn shape and output table read well and match the intended behavior (“pass this ref to the form field element to supportfocusFirstError” and “only available for basic fields”). This is a solid, minimal description.Optionally, you could make it even clearer that “form field element” means the actual DOM control (e.g.
<input>,<select>, etc.), not a higher‑level wrapper component, to avoid people accidentally attaching the ref to a custom component instead of the native element. For example:
ref– A ref object{ current: null }that should be passed to the underlying form field DOM element (e.g.<input>,<select>), …Purely a clarity tweak; the current text is already usable.
Also applies to: 186-202
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
example/src/domain/Full.js(2 hunks)example/src/domain/machinery/Form.js(3 hunks)index.js(1 hunks)readme.md(4 hunks)src/a11y.js(1 hunks)src/fields.js(3 hunks)src/hooks.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
example/src/domain/Full.js (2)
src/a11y.js (1)
focusFirstError(23-29)src/hooks.js (1)
form(16-20)
src/a11y.js (2)
src/hooks.js (8)
useFormFieldSnapshot(81-90)field(102-102)field(133-133)field(140-140)state(82-88)state(103-103)state(134-134)state(141-141)src/validation.js (1)
error(22-22)
🪛 markdownlint-cli2 (0.18.1)
readme.md
113-113: Link fragments should be valid
Expected: #formerrorregion; Actual: #FormErrorRegion
(MD051, link-fragments)
115-115: Link fragments should be valid
Expected: #focusfirsterror; Actual: #focusFirstError
(MD051, link-fragments)
🔇 Additional comments (10)
src/a11y.js (3)
57-70: LGTM!The
flattenErrorsfunction correctly handles various error structures: null/undefined, primitive errors, error objects withid/params, and nested structures withself/children.
6-17: LGTM!The
FormErrorRegioncomponent correctly implements a visually hidden live region witharia-live="polite"androle="status", which is the appropriate pattern for announcing form errors to screen readers without interrupting.
72-86: LGTM!The
defaultRenderErrorgracefully handles different error shapes, andvisuallyHiddenStylefollows the standard accessible hiding technique.example/src/domain/Full.js (1)
80-84: LGTM!Good example of the intended usage pattern: checking
snapshot.invalidbefore callingfocusFirstError(form)and returning early to prevent submission with invalid data.index.js (1)
17-20: LGTM!Clean addition of the new accessibility utilities to the public API, following the existing export pattern.
src/hooks.js (2)
108-125: LGTM!Consistent ref propagation through
useNumberFormFieldanduseBooleanFormField, maintaining the same pattern asuseFormField.
100-106: No action needed. Therefproperty is correctly available whenever these hooks are called. TheuseNumberFormFieldanduseBooleanFormFieldhooks are designed to work only with basic fields, which always have arefproperty created bycreateStableRef(). Object and array fields have different structures (fieldsandhelpersproperties instead ofeventHandlers) and would never be passed to these hooks.src/fields.js (2)
142-143: LGTM!Creating the ref at field construction time ensures stable identity for the lifetime of the field, which is the correct approach for programmatic focus management.
243-261: LGTM!Excellent documentation explaining why a plain object works as a ref outside React components. The implementation is minimal and correct.
readme.md (1)
452-485: New FormErrorRegion and focusFirstError docs are clear and align with the new a11y featureThe sections for
FormErrorRegionandfocusFirstErrorconcisely explain:
- what each does (live region for errors vs. focusing first invalid field),
- required props (
form, optionalrenderError),- and typical usage on failed submit (
if (snapshot.invalid) { focusFirstError(form) }).This is enough for consumers to adopt the new a11y helpers without digging into implementation. No changes strictly needed from a docs perspective.
…and add optional chaining to `focusFirstError`.
|
@AlbertSmit I think this is easier: |
|
Also, please base your pull request on |
Hmm, ja, misschien wel minder code qua regels, maar niet perse de React-way of doing things? Voorbeeldimplementatie van Wellicht ook wat breekbaar qua opzet; in
Ik kan me voorstellen dat dat gaat verwarren in de toekomst. Ik snap wel dat dat is hoe data-attributen werken (bijvoorbeeld bij Bij het gebruik van |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.