fix(Switch): updated a11y by removing dynamic labeling by thatblindgeye · Pull Request #10646 · patternfly/patternfly-react

fix(Switch): updated a11y by removing dynamic labeling by thatblindgeye · Pull Request #10646 · patternfly/patternfly-react

		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. */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

		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). */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

		if (!props.label && !props['aria-label'] && !props['aria-labelledby']) {
		// eslint-disable-next-line no-console
		console.error('Switch: Switch requires either a label or an aria-label to be specified');
		console.error(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would as unit test for this. That can be follow up. If we want to get this in.

Eric Olkowski added 8 commits

July 3, 2024 15:55

jelly mentioned this pull request

23 tasks