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 8 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: ProgressBar -->
6 changes: 3 additions & 3 deletions docs/content/ProgressBar.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {ProgressBar} from '@primer/react'
## Examples

```jsx live
<ProgressBar progress={80} />
<ProgressBar progress={80} aria-label="Upload test.png" />
```

### Inline
Expand All @@ -24,7 +24,7 @@ If you'd like to use ProgressBar inline, pass the `inline` boolean prop & **be s
```jsx live
<>
<Text mr={3}>5 of 10</Text>
<ProgressBar progress={50} inline sx={{width: '100px'}} />
<ProgressBar progress={50} inline sx={{width: '100px'}} aria-label="Upload test.png" />
</>
```

Expand All @@ -34,7 +34,7 @@ If you want to show multiple segments in a ProgressBar, pass separate `Item`s as

```jsx live
<>
<ProgressBar>
<ProgressBar aria-label="Upload test.png">
<ProgressBar.Item progress={33} />
<ProgressBar.Item progress={33} sx={{backgroundColor: 'danger.emphasis'}} />
</ProgressBar>
Expand Down
20 changes: 10 additions & 10 deletions src/ProgressBar/ProgressBar.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
import React from 'react'
import {ComponentMeta} from '@storybook/react'
import {Meta} from '@storybook/react'
import {ProgressBar} from '..'

export default {
title: 'Components/ProgressBar/Features',
component: ProgressBar,
} as ComponentMeta<typeof ProgressBar>
} as Meta<typeof ProgressBar>

export const ProgressZero = () => <ProgressBar progress="0" />
export const ProgressHalf = () => <ProgressBar progress="50" />
export const ProgressDone = () => <ProgressBar progress="100" />
export const ProgressZero = () => <ProgressBar progress="0" aria-label="Upload test.png" />
export const ProgressHalf = () => <ProgressBar progress="50" aria-label="Upload test.png" />
export const ProgressDone = () => <ProgressBar progress="100" aria-label="Upload test.png" />

export const SizeSmall = () => <ProgressBar progress="66" barSize="small" />
export const SizeLarge = () => <ProgressBar progress="66" barSize="large" />
export const SizeSmall = () => <ProgressBar progress="66" barSize="small" aria-label="Upload test.png" />
export const SizeLarge = () => <ProgressBar progress="66" barSize="large" aria-label="Upload test.png" />

export const Inline = () => <ProgressBar inline progress="66" sx={{width: '100px'}} />
export const Inline = () => <ProgressBar inline progress="66" sx={{width: '100px'}} aria-label="Upload test.png" />

export const Color = () => <ProgressBar progress="66" bg="done.emphasis" />
export const Color = () => <ProgressBar progress="66" bg="done.emphasis" aria-label="Upload test.png" />

export const MultipleItems = () => (
<ProgressBar>
<ProgressBar aria-valuenow={70} aria-label="Upload test.png">
<ProgressBar.Item progress={33} />
<ProgressBar.Item progress={23} sx={{backgroundColor: 'danger.emphasis'}} />
<ProgressBar.Item progress={14} sx={{backgroundColor: 'severe.emphasis'}} />
Expand Down
4 changes: 2 additions & 2 deletions src/ProgressBar/ProgressBar.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ export default {
component: ProgressBar,
} as Meta<typeof ProgressBar>

export const Default = () => <ProgressBar />
export const Default = () => <ProgressBar aria-label="Upload test.png" />

export const Playground: ComponentStory<typeof ProgressBar> = args => (
<ProgressBar {...args} sx={args.inline ? {width: '100px'} : {}} />
<ProgressBar {...args} sx={args.inline ? {width: '100px'} : {}} aria-label="Upload test.png" />
)

Playground.args = {
Expand Down
54 changes: 36 additions & 18 deletions src/ProgressBar/ProgressBar.tsx
View file
Open in desktop

@broccolinisoup broccolinisoup Jul 14, 2023

Copy link
Copy Markdown
Member

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??

@kendallgassner kendallgassner Jul 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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?

@broccolinisoup broccolinisoup Jul 19, 2023

Copy link
Copy Markdown
Member

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.

@siddharthkp siddharthkp Jul 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

@siddharthkp siddharthkp Jul 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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" />

@kendallgassner kendallgassner Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

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.

@kendallgassner kendallgassner Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

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 ?

@joshblack joshblack Jul 19, 2023

Copy link
Copy Markdown
Member

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,37 @@ 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.')
}

if (
children &&
typeof (rest as React.AriaAttributes)['aria-valuenow'] === 'undefined' &&
typeof (rest as React.AriaAttributes)['aria-valuetext'] === 'undefined'
) {
throw new Error(
'Use the `aria-valuenow` or `aria-valuetext` so screen reader users can determine the `progress`.',
)
Comment thread
kendallgassner marked this conversation as resolved.
Outdated
}

const progressAsNumber = typeof progress === 'string' ? parseInt(progress) : progress
Comment thread
kendallgassner marked this conversation as resolved.
Outdated

const ariaAttributes = {
'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,

@siddharthkp siddharthkp Jul 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,
'aria-valuenow': progressAsNumber || undefined,

Don't think we need to Math.round because parseInt already assumes base 10 and returns an integer

parseInt(20.5) === parseInt(20.5, 10) === 20

@kendallgassner kendallgassner Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh good point! I added the math.round before I used parseInt.

@kendallgassner kendallgassner Jul 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh wait... I need it in the case that the typeof the value is a number not a string

@siddharthkp siddharthkp Jul 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah I see! My bad!

@joshblack joshblack Jul 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@siddharthkp one note with parseInt, I believe it will infer the base if no radix is provided based on: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#radix

If 0 or not provided, the radix will be inferred based on string's value. Be careful — this does not always default to 10!

@siddharthkp siddharthkp Jul 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wow, good note!

'aria-valuemin': 0,
'aria-valuemax': 100,
'aria-busy': progressAsNumber ? progressAsNumber !== 100 : false,
}

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

ProgressBar.displayName = 'ProgressBar'
29 changes: 22 additions & 7 deletions src/__tests__/ProgressBar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,41 @@ describe('ProgressBar', () => {
})

it('should have no axe violations', async () => {
const {container} = HTMLRender(<ProgressBar progress={80} barSize="small" />)
const {container} = HTMLRender(<ProgressBar progress={80} barSize="small" aria-label="Upload test.png" />)
const results = await axe(container)
expect(results).toHaveNoViolations()
})

it('respects the "barSize" prop', () => {
expect(render(<ProgressBar progress={80} barSize="small" />)).toHaveStyleRule('height', '5px')
expect(render(<ProgressBar progress={80} barSize="default" />)).toHaveStyleRule('height', '8px')
expect(render(<ProgressBar progress={80} barSize="large" />)).toHaveStyleRule('height', '10px')
expect(render(<ProgressBar progress={80} barSize="small" aria-label="Upload test.png" />)).toHaveStyleRule(
'height',
'5px',
)
expect(render(<ProgressBar progress={80} barSize="default" aria-label="Upload test.png" />)).toHaveStyleRule(
'height',
'8px',
)
expect(render(<ProgressBar progress={80} barSize="large" aria-label="Upload test.png" />)).toHaveStyleRule(
'height',
'10px',
)
})

it('respects the "inline" prop', () => {
expect(render(<ProgressBar progress={80} inline />)).toHaveStyleRule('display', 'inline-flex')
expect(render(<ProgressBar progress={80} inline aria-label="Upload test.png" />)).toHaveStyleRule(
'display',
'inline-flex',
)
})

it('respects the "width" prop', () => {
expect(render(<ProgressBar progress={80} inline width="100px" />)).toHaveStyleRule('width', '100px')
expect(render(<ProgressBar progress={80} inline width="100px" aria-label="Upload test.png" />)).toHaveStyleRule(
'width',
'100px',
)
})

it('respects the "progress" prop', () => {
expect(render(<ProgressBar progress={80} />)).toMatchSnapshot()
expect(render(<ProgressBar progress={80} aria-label="Upload test.png" />)).toMatchSnapshot()
})
})
10 changes: 10 additions & 0 deletions src/__tests__/__snapshots__/ProgressBar.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ exports[`ProgressBar renders consistently 1`] = `
}

<span
aria-busy={false}
aria-valuemax={100}
aria-valuemin={0}
className="c0"
role="progressbar"
>
<span
className="c1"
Expand All @@ -46,7 +50,13 @@ exports[`ProgressBar respects the "progress" prop 1`] = `
}

<span
aria-busy={true}
aria-label="Upload test.png"
aria-valuemax={100}
aria-valuemin={0}
aria-valuenow={80}
className="c0"
role="progressbar"
>
<span
className="c1"
Expand Down