-
Notifications
You must be signed in to change notification settings - Fork 10
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
Address numerous a11y issues in WB Stories #2509
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b42a7b6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: NOT Published🤕 Oh noes!! We couldn't find any changesets in this PR (e4dd2ca). As a result, we did not publish an npm snapshot for you. |
Size Change: 0 B Total Size: 98.9 kB ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-jmfuvfsauu.chromatic.com/ Chromatic results:
|
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.
LGTM. Thanks for getting these fixed up!
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.
Yay looks great! Thanks for addressing these issues in our stories 🎉 Left some questions!
@@ -83,7 +104,6 @@ export const Default: StoryComponentType = { | |||
render: (args) => <ChoiceWrapper {...args} />, | |||
args: { | |||
label: "Pineapple (Control)", | |||
value: "pineapple", |
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.
Do we need the value
arg set so the value
is set on the Pineapple choice on line 57?
<Choice aria-label="Pineapple" {...args} /> |
@@ -55,7 +55,9 @@ export default { | |||
decorators: [ | |||
(Story): React.ReactElement<React.ComponentProps<typeof View>> => ( | |||
<View style={styles.example}> | |||
<Story /> | |||
<div role="listbox" aria-label="Example"> |
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.
Something I've been doing to address a11y warnings is to disable specific rules if we know it is expected.
For example, for the NavigationTabItem
stories, because it renders a <li>
without a <ul>
(since that's provided by NavigationTabs
), we expect a related warning about that, but there should be no warnings when NavigationTabs
and NavigationTabItem
are used together! Here's an example of disabling the listitem
rule when it's expected
Do we have a preference for disabling expected rules or wrapping it with additional things to make sure it's semantically correct? Curious to see if others have thoughts on this!
Summary:
[misc-a11y-issues] WB-1838: Add labels to toolbar icons
[misc-a11y-issues] WB-1837: Add labels to switches
[misc-a11y-issues] WB-1833: Add labels to radios
[misc-a11y-issues] WB-1832: Fix ids on Choices
[misc-a11y-issues] WB-1831: Add labels to checkboxes
[misc-a11y-issues] WB-1823: Add aria-label to listbox
[misc-a11y-issues] WB-1822: Fix actionItem role issue
[misc-a11y-issues] WB-1821: Fix tab role issue
[misc-a11y-issues] WB-1820: Fix label in button story
[misc-a11y-issues] WB-1819: Fix banner spinner label
[misc-a11y-issues] WB-1826: add listbox to options
Parent Issue: WB-1817
Test plan: