Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Add valueText prop onto progress bar for readability. #1851

Merged
merged 10 commits into from
Sep 24, 2018

Conversation

JakeLaCombe
Copy link
Contributor

Summary

Resolves #1838

Progress Bars have a hard time reading values of a progress bar to users in a human readable fashion. To resolve this, aria-valuetext can be used which is the text that is read to users through the screen reader.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1851 September 10, 2018 18:57 Inactive
@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 10, 2018 19:00 Inactive
/**
* Value passed to aria-valuetext for accessibility
*/
valueText: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this prop should be required if needed for accessibly. Because we need to be passive, should we

  1. log an issue to make this prop required in the next MVB
  2. and add a non-prod warning message saying this prop will be made required in the next MVB and should be supplied for accessibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this attribute is always required, reading through the documentation it sounds like screen readers should be able to interpret the valueNow if represented as a percentage and the valueText is usually used to represent non-numeric / non-percentage based valueNows. But it does make me wonder whether or not we should be locking down the other aria attributes for min, max, and valueNow

https://www.w3.org/WAI/PF/aria/states_and_properties#aria-valuetext

https://www.w3.org/WAI/PF/aria/states_and_properties#aria-valuenow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you thinking of locking them down to?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this shouldn't be required, can we beef up the description to describe when this prop should be used with a link to the accessibility information? I imagine consumers would find that useful!

@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 12, 2018 15:20 Inactive
@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 12, 2018 18:43 Inactive
@JakeLaCombe JakeLaCombe moved this from In Review to Ready for verification in Terra Sep 14, 2018
@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 17, 2018 15:56 Inactive
@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 18, 2018 20:09 Inactive
@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 19, 2018 19:19 Inactive
@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 19, 2018 19:30 Inactive
@emilyrohrbough
Copy link
Contributor

@JakeLaCombe Should we add an example / test to verify accessibly when this value isn't provided?

@mmalaker
Copy link

Functional verification completed. The progress bar example values now can be read by screen readers.

@JakeLaCombe JakeLaCombe temporarily deployed to terra-core-deployed-pr-1851 September 21, 2018 20:16 Inactive
@JakeLaCombe JakeLaCombe merged commit 15d020b into master Sep 24, 2018
@emilyrohrbough emilyrohrbough deleted the Progress-Bar-Aria-Valuetext branch September 24, 2018 20:45
@bjankord bjankord moved this from Ready for verification to Ready for Release in Terra Sep 27, 2018
@bjankord bjankord removed this from Ready for Release in Terra Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terra Progressbar doesn't read current value accurately
5 participants