When hiding a Spinner, also hide its screen reader text
#3021
Conversation
🦋 Changeset detectedLatest commit: 42596f3 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 |
| <% end %> | ||
| <% if no_aria_label? %> | ||
| <span class="spinner-screenreader-text sr-only"><%= @sr_text %></span> | ||
| <%= render(Primer::BaseComponent.new(tag: :span, hidden: @hidden, data: { target: @target })) do %> |
There was a problem hiding this comment.
aaah, I'm guessing the Primer::BaseComponent.new() is the magic bit I needed to be able to access the screenreader-only element from other spinner files?
There was a problem hiding this comment.
Sort of, we just needed to wrap both the <svg> and the <span class="sr-only"> in another <span> so we could hide them both together. It was a little tricky because usually we pass **system_arguments to the top-level element (eg. the new <span>), but that would break the existing API which puts **system_arguments on the <svg> element. I had to extract only certain attributes, eg. data-target and hidden, to be able to put them on the new <span> and pass everything else down to the <svg>.
What are you trying to accomplish?
Unfortunately I merged #3011 a little too fast. This is a follow-up PR that adjusts the approach taken in the original PR by hiding/showing the entire
Spinner, including the screen reader text.Integration
No changes necessary in production.
Risk Assessment
Accessibility
This doesn't fix an axe scan violation, but it does address the following accessibility issue: https://github.com/github/primer/issues/3690
Merge checklist
- [ ] Added/updated documentation- [ ] Added/updated previews (Lookbook)Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.