fix(Switch): updated a11y by removing dynamic labeling #10646
Conversation
thatblindgeye
requested review from
a team,
tlabaj and
kmcfaul
and removed request for
a team
thatblindgeye
requested review from
a team and
jessiehuff
and removed request for
a team
|
Preview: https://patternfly-react-pr-10646.surge.sh A11y report: https://patternfly-react-pr-10646-a11y.surge.sh |
thatblindgeye
requested review from
nicolethoen
and removed request for
jessiehuff
| labelOff?: React.ReactNode; | ||
| /** Adds an accessible name to the switch when the label prop is not passed, and must describe the isChecked="true" state. */ | ||
| 'aria-label'?: string; | ||
| /** Adds an accessible name to the switch via a list of referenced id's. The computed accessible name must describe the isChecked="true" state. */ |
There was a problem hiding this comment.
Not necessarily a suggestion, but I wonder if someone will think it needs to be a list of id's (versus being able to use a single id) the way it's worded currently?
| /** Adds an accessible name to the switch via a list of referenced id's. The computed accessible name must describe the isChecked="true" state. */ | |
| /** Adds an accessible name to the switch via referenced id(s). The computed accessible name must describe the isChecked="true" state. */ |
There was a problem hiding this comment.
LGTM! One question - if there is no label, aria-label, and aria-labelledby, we have a console.error, but we still apply aria-labelledby="[switch-id]-label", which wouldn't work out of the box since no label was passed. Just want to make sure that's preferred over not applying aria-labelledby?
There was a problem hiding this comment.
VO and keyboard look good!
I think it may be worth even just a sentence somewhere in the Switch docs to ensure that the consumer knows that they need to consider a11y and wire things up correctly to be a11y compliant. Maybe directing them to the a11y docs or just calling out the most common scenario - something like that?
| @@ -30,8 +32,6 @@ export interface SwitchProps | |||
| isDisabled?: boolean; | |||
| /** A callback for when the switch selection changes. (event, isChecked) => {} */ | |||
| onChange?: (event: React.FormEvent<HTMLInputElement>, checked: boolean) => void; | |||
| /** Adds accessible text to the switch, and should describe the isChecked="true" state. When label is defined, aria-label should be set to the text string that is visible when isChecked is true. */ | |||
| 'aria-label'?: string; | |||
| /** Flag to reverse the layout of toggle and label (toggle on right). */ | |||
There was a problem hiding this comment.
can you update the text while in there. Or we can open a follow up issue. For RTL purposes it should say "end" rather than right". I would advocate for actually changing the name to better align with other components. Maybe togglePosition. we can introduce a new prop post v5. i can add new issue for that.
| // eslint-disable-next-line no-console | ||
| console.error('Switch: Switch requires either a label or an aria-label to be specified'); | ||
| console.error( |
There was a problem hiding this comment.
I would as unit test for this. That can be follow up. If we want to get this in.
What: Closes #10645
Codemod issue: patternfly/pf-codemods#672
Additional issues: