ProgressBar: Adjust ProgressBar.Item for accessibility
#4878
Conversation
🦋 Changeset detectedLatest commit: a2ca83b 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 📦
|
| const ariaAttributes = { | ||
| 'aria-valuenow': progressAsNumber && progressAsNumber >= 0 ? Math.round(progressAsNumber) : undefined, | ||
| 'aria-valuemin': 0, | ||
| 'aria-valuemax': 100, | ||
| } | ||
|
|
||
| warning( | ||
| children && | ||
| !ariaAttributes['aria-valuenow'] && | ||
| typeof (rest as React.AriaAttributes)['aria-valuenow'] === 'undefined' && | ||
| typeof (rest as React.AriaAttributes)['aria-valuetext'] === 'undefined', | ||
| 'Expected `aria-valuenow` or `aria-valuetext` to be provided to <ProgressBar>. Provide one of these values so screen reader users can determine the current progress. This warning will become an error in the next major release.', | ||
| ) |
There was a problem hiding this comment.
I'm curious if there's a reason why we didn't include ariaAttributes['aria-valuenow'] as a value to include in the warning conditional? Since we automatically create the aria-valuenow based on progressAsNumber being true and a number, is that sufficient enough if consumers don't provide an aria-valuenow themselves? Also, I added progressAsNumber >= 0 so we can include aria-valuenow values that start at 0.
Cc: @kendallgassner - would love your insight, but I realize that it's been a minute since we've introduced these changes 😅
There was a problem hiding this comment.
huh, your right that is odd and if a user passed it in it would override the value, we are setting via ...rest. 🤔 I think it's safe to remove the warning all together.
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
the
integration-tests: recommended
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
🟢 golden-jobs completed with status |
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/348525 |
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| 'aria-valuemax': 100, | ||
| 'aria-valuetext': progressAsNumber ? `${Math.round(progressAsNumber)}%` : undefined, | ||
| } | ||
| const validChildren = React.Children.toArray(children).length |
There was a problem hiding this comment.
Would https://react.dev/reference/react/Children#children-count work here as well for getting the length?
There was a problem hiding this comment.
I tried this but ran into an issue where non-components would be included in the count:
<ProgressBar barSize="small" aria-label="Upload test.png">
{false}
{false}
{false}
</ProgressBar>This resulted in React.Children.count(children) to equal 3 😕 This isn't really a big deal, but I noticed this via a conditional in dotcom that was causing an error, and it was because the children were booleans instead of actual components 😅
| {children ?? <Item data-animated={animated} progress={progress} bg={bg} />} | ||
| <ProgressContainer ref={forwardRef} barSize={barSize} {...rest}> | ||
| {validChildren ? ( | ||
| children |
There was a problem hiding this comment.
For this check, would it be enough to check for the presence of children and toggle or is the count important / used somewhere else?
There was a problem hiding this comment.
Right now, you can use the ProgressBar alone, or with children. If you use it alone, you can apply all the props directly to <ProgressBar>, but with children, you need to apply them to the individual items.
I think I understand the ask, are you asking if we can switch up the current conditional? 🤔
Co-authored-by: Josh Black <joshblack@github.com>
Co-authored-by: Josh Black <joshblack@github.com>
Closes https://github.com/github/primer/issues/3359
Changelog
New
Changed
aria-*attributes toProgressBar.ItemProgressBar.Itemasrole="progressbar"Removed
Rollout strategy
Testing & Reviewing
Merge checklist