Convey Spinner to assistive technologies #4140
Conversation
deprecates aria-label prop
🦋 Changeset detectedLatest commit: 7c736b8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
src/Spinner/Spinner.tsx
Outdated
| /> | ||
| </svg> | ||
| /* inline-flex removes the extra line height */ | ||
| <Box sx={{display: 'inline-flex'}} role={hasSrAnnouncement ? 'status' : undefined}> |
There was a problem hiding this comment.
Would we want to add the role value as a prop instead of a static value? If so, we would only have two values to pick from, status and alert, with status being the default value.
There was a problem hiding this comment.
We can definitely do that. Great idea.
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
I'm converting this back to a draft until we decide on a direction based on feedback in this discussion: https://github.com/github/primer/discussions/2976 |
|
The aria-live issues should be resolved by #4174 |
|
Hey @mperrotti 👋! With |
|
@mperrotti Hi! Just wanted to check in on the status of this PR 🙏🏻 |
|
Sorry @lindseywild - this PR got lost and stale. Going to update today. Related PVC PR: primer/view_components#2884 |
|
@lindseywild @TylerJDev @joshblack - should I remove the announcements and follow @owenniblock's PVC implementation until we've figured out how to handle multiple announcements?
|
I think that's fair. Unless we can make it opt-in, where the live region only exists if the consumer turns it on themselves via a prop. Even without the live region, this PR will fix the accessibility issue on there being no context by default on the |
| /> | ||
| </svg> | ||
| /* inline-flex removes the extra line height */ | ||
| <Box sx={{display: 'inline-flex'}} role={hasSrAnnouncement ? 'status' : undefined}> |
There was a problem hiding this comment.
Since we're using the <Status> component now, we can remove the role here.
| </svg> | ||
| {hasSrAnnouncement ? ( | ||
| <VisuallyHidden> | ||
| <Status>{srText || ariaLabel}</Status> |
There was a problem hiding this comment.
Per #4140 (comment), we could make this opt-in and keep the live region, but I'll leave this up to you! It would probably involve an additional prop to toggle the live region being on/off with the default being off. We could also remove it and come back to it in the future, but there might be value in having it in now as opt-in.
|
@TylerJDev - after talking to @joshblack, I decided to remove the announcement until we land on a solution for using |
Closes #3942
Changelog
New
srTextprop (default value is "Loading"Changed
aria-labelon SpinnerRemoved
Rollout strategy
When we ship the next major release, we can remove
aria-labelfrom Spinner and provide a codemod to change it tosrText.Testing & Reviewing
Merge checklist