-
Notifications
You must be signed in to change notification settings - Fork 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
FAQ a11y sign off updates #358
base: main
Are you sure you want to change the base?
Conversation
|
@@ -107,7 +107,7 @@ details[open] .Accordion__summary--expanded::after { | |||
details[open] > .Accordion__content { | |||
padding-left: var(--base-size-40); | |||
padding-bottom: var(--base-size-24); | |||
margin-top: calc(var(--base-size-16) * -1); /* for 8px gap between question and answer */ |
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.
(sev 2) When the accordion controls are expanded using the keyboard, the focus indicator overlaps the first line of text in the panel decreasing readability.
This needed to be removed to support this a11y issue
🟢 No design token changes found |
|
||
function FAQGroup({className, children}: FAQGroupProps) { | ||
return ( | ||
// eslint-disable-next-line jsx-a11y/no-redundant-roles |
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.
sev 2) For all examples, the accordion controls are not using list semantics. Screen reader users will not be made aware of the grouped nature of the controls, nor how many items are in the collection.
Solution: Use an unordered list ul to wrap the related accordion controls, with an li wrapping each individual item. Add an aria-labelledby attribute to the ul, and reference the ID of the section heading. If list-style: none is applied with CSS, be sure to add role="list" to the ul element to ensure that all screen readers report the list structure. Some screen reader/browser combinations will remove list semantics if no bullets are present, and the explicit role declaration restores them.
This needed to be added to follow this guidance. Maybe the linter can be updated so we can remove this in the future.
|
@@ -120,8 +120,8 @@ export const AllOpen: StoryFn<typeof FAQ> = () => { | |||
return ( | |||
<Container> | |||
<FAQ> | |||
<FAQ.Heading>Frequently asked questions</FAQ.Heading> | |||
<> | |||
<FAQ.Heading id="all-open-faq">Frequently asked questions</FAQ.Heading> |
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.
Could we apply this inside the component using a unique id? I suspect users may forget to do this otherwise so we could do it for them internally
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.
How would you feel about making these props required and leaving information in the documentation? We could do this dynamically but it would require a second loop of the children to first grab the id if a user specifies one and then set the id on the group. Of course, if they didn't supply one then this second loop wouldn't be needed but I was thinking we would want to support them passing their own id if they wanted to access that component using an id query.
Summary
A number of a11y changes to ensure full accessibility for the FAQ component.
List of notable changes:
What should reviewers focus on?
Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
Reviewer checklist:
Screenshots: