FAQ a11y sign off updates #358
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.
(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.
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.
JoshBowdenConcepts
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
| @@ -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.
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.
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: