Repository navigation
feat(FieldError): announce errors via role="alert" + fix react peerDep - #59
Conversation
hugomallet
left a comment
There was a problem hiding this comment.
Tooled self-review (author = reviewer; GitHub blocks a formal approve, so this is a COMMENT). Review conducted with the team review checklist; lint + tsc clean on changed files.
Verdict: mergeable. Scope is strictly respected (FieldError + peerDependencies only), role="alert" is placed before {...props} so it stays overridable, and both the default and the override are covered by tests. The peerDep fix is factually correct: src/lib/Form/Context.js uses createContext + useContext, so >=15 was a lie and >=16.8 is the real floor.
Minor findings, grouped — none blocking:
- Announcement relies on CSS un-hide, not on mount (
src/lib/FieldError/FieldError.scss). The default stylesheet renders the alertdisplay: noneas soon as the field has errors and only reveals it via.isSubmitted. Screen readers must therefore pick up thedisplay:none → blocktoggle, which is historically flakier than inserting the alert element with its content (the pattern that motivated this PR). Worth a quick VoiceOver/NVDA sanity check; the robust fix (gate rendering in JS on submitted/touched instead of CSS) can go in a #57 follow-up. - Release the peerDep change as ≥ 0.7.0, not a patch. Narrowing
reactto>=16.8makesnpm installfail (ERESOLVE on npm 7+) for consumers still on React <16.8. They were already broken at runtime, so this is surfacing reality — but it deserves a minor bump and an explicit line in the release notes. role="alert"=aria-live="assertive"+aria-atomic="true": submitting a form with several invalid fields fires several assertive announcements at once. Acceptable default (matches our guideline for form errors) and consumers can passrole="status"— just flagging the trade-off consciously.- Docs + remaining a11y wiring: README shows
<FieldError>but doesn't mention the new default role or how to override it; and the guideline's companion wiring (aria-invalid+aria-describedbyon the input pointing at this alert) is still absent. Both presumably later lots of #57 — please make sure they're tracked there.
Tests: the two new cases mirror the suite's conventions (mocked FormContext.Consumer, enzyme mount), bite was proven by mutation, and the snapshot diff is limited to the added role line. Good to merge with the notes above.
Add role="alert" as a default on the rendered element so screen readers announce validation errors when they appear. Placed before the props spread so consumers can still override it. Also fix peerDependencies: react >=15 was wrong since the code relies on createContext (16.3+) and useContext (16.8+); require >=16.8.
913f9d1 to
c4812d2
Compare
Fixes part of #57 (a11y).
role="alert"as a default on the element rendered by<FieldError>, so screen readers announce validation errors when they appear. Placed before the props spread, so consumers can still override it (test covers both).peerDependencies.react:>=15was wrong — the code usescreateContext(16.3+) anduseContext(16.8+). Now>=16.8.Tests: 6 suites, 65 tests (2 new), 5 snapshots, all green (
CI=true yarn test:runtime).Release / follow-up notes (from self-review)
>=15to>=16.8makesnpm installfail (ERESOLVE) for consumers still on React <16.8 (they were already broken at runtime)..isSubmitted, not mount-gated), README mention of the new defaultrole, andaria-invalid/aria-describedbywiring on<Field>(blocked by Fix/field non dom components #55).