Uh oh!
There was an error while loading. Please reload this page.
UnderlineNav2: Add aria-hidden and sr-only class support for descriptive counters - #2551
Conversation
🦋 Changeset detectedLatest commit: 1c87674 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 📦
|
josepmartins
left a comment
There was a problem hiding this comment.
LGTM! ✨
Just a couple of non-blocking comments
| {loadingCounters ? ( | ||
| <LoadingCounter /> | ||
| <Box as="span" data-component="counter"> | ||
| <LoadingCounter /> |
There was a problem hiding this comment.
non-blocking and asking out of ignorance: Is it necessary to add a span/wrapper to the LoadingCounter or should be hidden from screen readers? 🤔
cc @ericwbailey
There was a problem hiding this comment.
That wasn't an intentional change, sorry! Thanks for pointing this out.
| <Box as="span" data-component="counter" sx={counterStyles}> | ||
| <CounterLabel>{counter}</CounterLabel> | ||
| <CounterLabel aria-hidden="true">{counter}</CounterLabel> | ||
| <VisuallyHidden> ({counter})</VisuallyHidden> |
There was a problem hiding this comment.
non-blocking: Maybe it's just relative to the UnderlineNav, but I wonder if we should consider adding this extra wrapping markup + the VisuallyHidden behavior inside the Counter component.
There was a problem hiding this comment.
I am not really sure if I can follow you, sorry!
Do you mean like this
<CounterLabel sx={counterStyles}>
<Box as="span" data-component="counter" aria-hidden="true">
{counter}
</Box>
<VisuallyHidden> ({counter})</VisuallyHidden>
</CounterLabel>
If so, the counterStyles doesn't really apply to position the counter itself rather they position the string within the counter label.
Wrapper inside the CounterLabel
wrapper outside the CounterLabel (matches with the design)
Let me know if I miss anything or misunderstand!
There was a problem hiding this comment.
My bad 🙇 ! The comment was more into adding all of these wrappers as part of the Counter component itself.
It seems that any other component using it, like Buttons with counters might benefit from adding the Box wrapper with the aria-hidden and the VisuallyHidden element.
There was a problem hiding this comment.
Ahh Okay! Yeah, that is a great idea. CounterLabel could definitely use some help. How about I create an issue for that and we can discuss from there? I'm going to merge this pull requests as you approved but let me know if you have any concerns 🙌🏼
Addressing an accessibility sign-off review issue that described below.
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.