Skip to content

Add aria attributes to the progress bar #3517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kendallgassner merged 9 commits into from
Jul 20, 2023
Merged

Add aria attributes to the progress bar #3517

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
814bf19
Add aria attributes to the progress bar
kendallgassner Jul 13, 2023
b318e2c
changset
kendallgassner Jul 13, 2023
7bbd044
Account for multiple items
kendallgassner Jul 14, 2023
80e41b7
Update .changeset/plenty-mangos-sniff.md
kendallgassner Jul 14, 2023
48aafd7
add aria-label
kendallgassner Jul 14, 2023
5f03895
more aria-labels
kendallgassner Jul 14, 2023
d8dcf82
Merge branch 'main' into add-aria-to-progress-bar
kendallgassner Jul 18, 2023
7dc7fe9
Add display name
kendallgassner Jul 18, 2023
fb50b97
parse int fix and warning
kendallgassner Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/plenty-mangos-sniff.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@primer/react': patch
---

Adding aria-attributes and role to the ProgressBar component

<!-- Changed components: _none_ -->
41 changes: 23 additions & 18 deletions src/ProgressBar/ProgressBar.tsx
View file
Open in desktop
Copy link
Member

@broccolinisoup broccolinisoup Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have ProgressBar with multiple items like here and we don't get aria attributes in these I realised. There is no usage of ProgressBar.Item though at dotcom so not sure if it would worth the effort. What do you think??

Copy link
Contributor Author

@kendallgassner kendallgassner Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!!

To deal with multiple I now throw an error if children are passed in without aria-valuenow or 'aria-valuetext:

    throw new Error(
      'Use the `aria-valuenow` or `aria-valuetext` so screen reader users can determine the `progress`.',
    )

What do you think?

Copy link
Member

@broccolinisoup broccolinisoup Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see! I have a different take on this but I am not 100% sure, let me know what you think!

Seems like we are handling these aria attributes internally and I think it is a tad inconsistent to ask this from consumers when there are ProgressBar.Item children.

I am thinking maybe we would want to calculate the total progress internally and update the aria attributes as we do for the single progress item? Something along those lines (not a complete code)

if (children) {
      let totalProgress = 0
      React.Children.toArray(children).forEach(child => {
        if (React.isValidElement(child) && child.props.progress) {
          totalProgress += parseInt(child.props.progress)
        }
      })
    }

And use that totalProgress to calculate the value of aria attributes.

cc @siddharthkp @joshblack I would love their opinion before we decide anything concrete.

Copy link
Member

@siddharthkp siddharthkp Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

We could definitely loop through the children if the usage is predictable. However...

I couldn't find any usage of ProgressBar with multiple items, so we might be able to slide it under the rug and deprecate it safely without any remediation :)

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react'
import React, {forwardRef} from 'react'
import styled from 'styled-components'
import {width, WidthProps} from 'styled-system'
import {get} from '../constants'
Expand All @@ -23,6 +23,7 @@ const sizeMap = {
type StyledProgressContainerProps = {
inline?: boolean
barSize?: keyof typeof sizeMap
tabIndex?: number
Copy link
Member

@siddharthkp siddharthkp Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you need to add this explicitly 🤔

If you pass tabIndex to ProgressBar, it passes it already passes it down to the html element

<Progress tabIndex={0} progress={50} aria-label="Task completion" />

Copy link
Contributor Author

@kendallgassner kendallgassner Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! I'll try this out.

Copy link
Contributor Author

@kendallgassner kendallgassner Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I need the type added:

A screenshot of a ProgressBar storybook story. This story shows the tabIndex with a red underline indicating a typeerror.

Is there a better type to use? Maybe React.HTMLProps ?

Copy link
Member

@joshblack joshblack Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be an option?

type StyledProgressContainerProps = {
  inline?: boolean
  barSize?: keyof typeof sizeMap
} & WidthProps &
  SxProp &
  React.HTMLAttributes<HTMLSpanElement>

} & WidthProps &
SxProp

Expand All @@ -39,20 +40,24 @@ const ProgressContainer = styled.span<StyledProgressContainerProps>`

export type ProgressBarProps = React.PropsWithChildren & {bg?: string} & StyledProgressContainerProps & ProgressProp

export const ProgressBar = ({
progress,
bg = 'success.emphasis',
barSize = 'default',
children,
...rest
}: ProgressBarProps) => {
if (children && progress) {
throw new Error('You should pass `progress` or children, not both.')
}

return (
<ProgressContainer barSize={barSize} {...rest}>
{children ?? <Item progress={progress} sx={{backgroundColor: bg}} />}
</ProgressContainer>
)
}
export const ProgressBar = forwardRef<HTMLSpanElement, ProgressBarProps>(
({progress, bg = 'success.emphasis', barSize = 'default', children, ...rest}: ProgressBarProps, forwardRef) => {
if (children && progress) {
throw new Error('You should pass `progress` or children, not both.')
}

const ariaAttributes = {
'aria-valuenow': typeof progress === 'number' ? Math.round(progress) : undefined,
'aria-valuetext': typeof progress === 'string' ? progress : undefined,
'aria-valuemin': 0,
'aria-valuemax': 100,
'aria-busy': progress !== 100,
Copy link
Member

@broccolinisoup broccolinisoup Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider the string type here as well?

Copy link
Contributor Author

@kendallgassner kendallgassner Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@broccolinisoup Thanks for bringing this up. I ended up changing aria attributes a bit since t looks like this component really only works if progress is a number or a string of a number like ("66" or "100") Because the string is really just a number using aria-valuetext isn't actually necessary.

To reduce confusion maybe we should only allow a number passed in?

Copy link
Member

@broccolinisoup broccolinisoup Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the first look at the usages at dotcom, in practicality it seems fine but technically narrowing types are considered as major change. (reference) And I don't know if there is any historical reasons to include string type in the first place. Sorry @kendallgassner we are dealing with a bit of an old component here I guess and lots to improve - I am trying to keep my review only specific to your changes but seems like some overlaps are inevitable 😬 I'll post internally as well to bring some more reviewers to get more context on this!

}

return (
<ProgressContainer ref={forwardRef} role="progressbar" barSize={barSize} {...ariaAttributes} {...rest}>
{children ?? <Item progress={progress} sx={{backgroundColor: bg}} />}
</ProgressContainer>
)
},
)