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

Add aria-label to Spinner component #4226

Closed
wants to merge 2 commits into from

Conversation

owenniblock
Copy link
Contributor

@owenniblock owenniblock commented Feb 6, 2024

Closes https://github.com/github/primer/issues/2202

This adds a new option for setting the accessible label of the Spinner component. When it's not provided, we default to "Loading" so you have to intentionally hide the element with label="". Where we find Loading state is communicated to screenreader users twice in dotcom, we can set this label value to "", I have intentionally opted for the most accessible default so it over communicates rather than under-communicating the loading state.

Changelog

New

label option on Spinner component. Defaults to "Loading". When set (or left as default) it adds aria-label={label} to the Spinner's svg.

Adds role="img" to the svg element.

If the label is set to an empty string, it adds aria-hidden='true' to the svg element.

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Feb 6, 2024

🦋 Changeset detected

Latest commit: 5f711be

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

Copy link
Contributor

github-actions bot commented Feb 6, 2024

size-limit report 📦

Path Size
dist/browser.esm.js 113.25 KB (+0.03% 🔺)
dist/browser.umd.js 113.91 KB (+0.05% 🔺)

@owenniblock
Copy link
Contributor Author

owenniblock commented Feb 6, 2024

So interestingly, the loading spinner is used on TextInput (via TextInputInnerVisualSlot) - need to work out whether we want to make it aria-hidden or not 🤔

We can see this on the deployed storybook page

Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Hello @owenniblock 👋🏻 We have an open [github.com/github/primer/discussions/2976](Spinner API discussion) that @mperrotti created, sorry if you already know about it or discussed it somewhere else. I just wanted to make sure to link them and would love to understand their overlap.

Thank you!! 🙏🏻

@owenniblock
Copy link
Contributor Author

@broccolinisoup I had not seen that, thanks for bringing it to my attention!

@owenniblock owenniblock marked this pull request as draft February 7, 2024 12:22
@owenniblock
Copy link
Contributor Author

Converting to draft because there's ongoing discussions about this component.

@owenniblock
Copy link
Contributor Author

Going to close this. We need to handle it differently anyway!

@owenniblock owenniblock closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants