-
Notifications
You must be signed in to change notification settings - Fork 30
fix(expandable components): Add/update aria attributes to expandable components #3054
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
Conversation
View your CI Pipeline Execution ↗ for commit 8a23092. ☁️ Nx Cloud last updated this comment at |
View your CI Pipeline Execution ↗ for commit c1991ca.
☁️ Nx Cloud last updated this comment at |
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.
code here looks great - could you add the screenreader behavior we should expect when testing?
@@ -36,6 +37,7 @@ export const DisclosureButton: React.FC<DisclosureButtonProps> = ({ | |||
<FlexBox> | |||
<DisclosureButtonWrapper | |||
aria-expanded={isExpanded} | |||
aria-controls={isExpanded ? ariaControlsId : undefined} |
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.
should the id
be on the button regardless of the expanded state?
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.
Woops, will update the testing instructions with the SR info, thanks!
—
re: id
tl;dr: an axe test was failing when, I think, aria-controls was trying to reference an id
on an element that wasn't rendered yet. Adding this ternary solved the issue, but wondering if all these aria-controls
should follow the pattern.
mini-story version: I didn't realize that Disclosure didn't have aria-expanded
or aria-controls
:\ so this is a new addition.
After I finished up with fixing some other failing mono tests, I saw that there was a failure on a Disclosure because of an "invalid value for aria-controls" https://github.com/codecademy-engineering/mono/actions/runs/14248719474/job/39936761982
Which I thought maybe that's cause I was using useId()
and that values weren't readable to a person. So I swapped ALL of the useId
instances with something more readable. And it was still failing lol (and I also had to fix a typo in the new id too x.x) So I read a bit more into it and saw this post: https://github.com/codecademy-engineering/mono/actions/runs/14248719474/job/39936761982 that mentioned that the element with the id
should be on the page.
Then I updated Disclosure's aria-controls
to this ternary and it fixed the issue. But I was hesitant about adding this logic to all the elements.
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.
word, makes sense to me!
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.
A couples questions and things I'm not seeing in storybook but mostly looking good!
- not seeing aria-controls on drawer/flyout when the component is collapsed
- should
Disclosure
havearia-controls
? the PR desc says no but testing instructions says yes - I don't see the id on the info tip, but possible i'm missing it or it needs to be added to the story
packages/styleguide/src/lib/Molecules/Flyout/Flyout.stories.tsx
Outdated
Show resolved
Hide resolved
Adds bi-directional option for multiview modals, refactors the CTAs in multiview modals, and adds danger variant. Adds unit tests and additional stories.
- @Codecademy/gamut@62.0.0 - @codecademy/[email protected] - @codecademy/[email protected]
Applying changes to the Modal and Dialog components per feedback from a11y team. Also adding a zIndex
- @Codecademy/gamut@62.0.1 - @codecademy/[email protected] - @codecademy/[email protected]
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.
One small note but otherwise LGTM!
<ExpandControl | ||
expanded={isExpanded} | ||
onExpand={() => setExpanded(!isExpanded)} | ||
disabled={false} | ||
/> |
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.
this button used to have yellow text and now its grey. probably fine since this is just the storybook example but wanted to note
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.
ya, thanks Amy — I should've left a comment.
That seems intentional based on what the original component, I'll ask Stacey just in case :)
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.
Gonna update to secondary
to match the textButton styling :) thanks again~
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
Overview
Components that expand other components should have an
aria-expands
attribute that denotes if an expandable component is expanded or not.When using Voiceover, an element with an


aria-expanded
attribute, should announce if the element is "expanded" or "collapsed".expanded example:
collapsed example:
This PR adds the
aria-expanded
attribute toDisclosure
andInfoTip
It also updates guidance for
Drawer
+Flyout
Also updates
ListRow
to useExpandControl
.Note: previously this PR also included
aria-controls
implementation, but that has moved to: #3069 and tabled for the time beingPR Checklist
Testing Instructions
aria-expanded
attributearia-expanded
is now updatedPR Links and Envs