Skip to content
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

fix(Button): pass through ARIA attrs #8115

Merged
merged 3 commits into from
Feb 2, 2023
Merged

fix(Button): pass through ARIA attrs #8115

merged 3 commits into from
Feb 2, 2023

Conversation

LeoMcA
Copy link
Member

@LeoMcA LeoMcA commented Feb 2, 2023

  • fix: pass through aira- props on Button component
  • fix: improve aria labels on mobile sidebar button

Summary

fixes #8015

Problem

  • Sidebar button had on aria label on it.
  • It was set in the code, but as aria-label rather than ariaLabel which is the prop the Button component was expecting
  • TypeScript doesn't type check any prop with a dash in it (e.g. aria-label but also foo-bar), including not checking if a component with typed props hasn't specified it as a prop it takes, so this bug was silently introduced

Solution

  • Remove our camelCase aria props on Button, and pass through all dashed props to underlying elements in Button component
    • This seemed safest as there's no way of preventing a developer adding a dashed prop, so it may continue to happen anyway
    • eslint still checks aria- props are in the right form (e.g. I mistyped aria-haspopup as aria-has-popup and got this error: aria-has-popup: This attribute is an invalid ARIA attribute. Did you mean to use aria-haspopup? jsx-a11y/aria-props)
    • if TypeScript behaviour changes, we haven't specified any dashed props in our props type, so we'll get some nice errors we can decide what to do with (either add dashed props to the type, only add aria- props, move back to camelCase aria props, etc.)
  • Improve aria-label on the sidebar button, and add aria-expanded and aria-controls props

How did you test this change?

Clicked around in Firefox's accessibility devtools.

as well as data- and any other dashed props

typescript won't alert us to a dashed prop being passed to a component
where it isn't accepted, so it's safest to pass them through wholesale:
microsoft/TypeScript#32447

partial fix to: #8015
@LeoMcA LeoMcA requested a review from caugner February 2, 2023 12:03
@github-actions github-actions bot added the plus work around features related to MDN Plus label Feb 2, 2023
@caugner caugner changed the title fix: pass through aira- props on Button component fix: pass through aria- props on Button component Feb 2, 2023
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one question/potential nit.

client/src/ui/atoms/button/index.tsx Outdated Show resolved Hide resolved
@caugner caugner changed the title fix: pass through aria- props on Button component fix(Button): pass through ARIA attrs Feb 2, 2023
@caugner caugner added the ♿ a11y An MDN Web Docs accessible by everyone label Feb 2, 2023
@caugner caugner merged commit 6bf0b50 into main Feb 2, 2023
@caugner caugner deleted the button-aria-dashed branch February 2, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♿ a11y An MDN Web Docs accessible by everyone plus work around features related to MDN Plus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No accessible label or state on
3 participants