Add parenthesis to trailing counter #2807
Conversation
🦋 Changeset detectedLatest commit: 307f7c7 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 |
| @@ -68,7 +68,10 @@ class Button < Primer::Component | |||
| renders_one :trailing_visual, types: { | |||
| icon: Primer::Beta::Octicon, | |||
| label: Primer::Beta::Label, | |||
| counter: Primer::Beta::Counter | |||
| counter: lambda { |**system_arguments| | |||
There was a problem hiding this comment.
Pls lmk if there is a better way to know that the trailing_visual is a counter, without setting this boolean.
khiga8
force-pushed
the
kh-add-paranthesis
branch
from
7ec9629
to
554de3f
Compare
|
| <span class="sr-only">(<%= trailing_visual %>)</span> | ||
| <span aria-hidden="true"><%= trailing_visual %></span> |
There was a problem hiding this comment.
Why is this causing a shift 🤔
There was a problem hiding this comment.
What I'm seeing in my browser does not match the changes in the snapshots. Does anyone know why I might not be seeing what's reflected in the snapshots?
There was a problem hiding this comment.
Ok actually nvm I see it now.
Having the counter nested inside another <span> seems to be the issue. Looks like I might want to directly set the aria-hidden on the counter.
khiga8
force-pushed
the
kh-add-paranthesis
branch
from
5dabdab
to
4b5b1e0
Compare
| counter: Primer::Beta::Counter | ||
| counter: lambda { |**system_arguments| | ||
| @trailing_visual_counter = true | ||
| Primer::Beta::Counter.new("aria-hidden": true, **system_arguments) |
There was a problem hiding this comment.
I am directly setting aria-hidden on the counter, because when I nested this inside a span, it caused issues. See #2807 (comment)
|
Thank you! Please merge whenever because I don't have permissions! |
What are you trying to accomplish?
This PR adds an sr-only parenthesis to improve screen reader output on the trailing counter. This update makes this have more parity with the React button. I followed the markup used there for consistency.
It is worth noting that if this button content is dynamically updated (e.g because the counter is incremented), the change will not be announced to screen reader users without additional markup changes. I'm still investigating a recommendation for this separately - https://github.com/github/accessibility/pull/6222.
Integration
This should not be a breaking change.
List the issues that this change affects.
Fixes: https://github.com/github/primer/issues/2208
Risk Assessment
What approach did you choose and why?
I conditionally rendered the parenthesis with an
sr-onlyspan.Anything you want to highlight for special attention from reviewers?
No
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.