Skip to content
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.

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

feat(progress-indicator): adds hidden span to announce state of completion #5125

Merged
merged 10 commits into from
Jan 24, 2020
Merged

feat(progress-indicator): adds hidden span to announce state of completion #5125

merged 10 commits into from
Jan 24, 2020

Conversation

abbeyhrt
Copy link
Contributor

Closes #4246

This PR adds a hidden span that will announce the state of the step, "Complete", "current", or "invalid" for screenreaders.

Changelog

New

  • span announcing step completion

Changed

  • div with role="button" to a button element
  • button reset and styling
  • update tests to not target role="button"

Testing / Reviewing

Using whatever screenreader you prefer to test the component and ensure that the completion state is announced.

@abbeyhrt abbeyhrt requested a review from a team as a code owner January 21, 2020 23:20
@ghost ghost requested review from aledavila and asudoh January 21, 2020 23:20
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thank you for this fix @abbeyhrt!

@@ -70,16 +70,31 @@ export const ProgressStep = ({ ...props }) => {
);
};

let message = 'Incomplete';
Copy link
Contributor

Choose a reason for hiding this comment

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

g11n; Do we want to add a prop to let app specify the assistive text? (Though translate-ability for assistive text didn't use to be a hard-requirements, I have seen several issues asking for it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I added it, it's the first time I've done it so let me know if it all looks good!

@netlify
Copy link

netlify bot commented Jan 21, 2020

Deploy preview for carbon-elements ready!

Built with commit b0fd7f4

https://deploy-preview-5125--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 21, 2020

Deploy preview for carbon-components-react failed.

Built with commit b0fd7f4

https://app.netlify.com/sites/carbon-components-react/deploys/5e2b167e53a5c7000933b3a7

@netlify
Copy link

netlify bot commented Jan 21, 2020

Deploy preview for the-carbon-components ready!

Built with commit b0fd7f4

https://deploy-preview-5125--the-carbon-components.netlify.com

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Seems great, also totally understand if you want to add support for translation per the feedback above 👍

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 ✅

@abbeyhrt abbeyhrt changed the title fix(progress-indicator): adds hidden span to announce state of completion feat(progress-indicator): adds hidden span to announce state of completion Jan 23, 2020
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @abbeyhrt!

@abbeyhrt abbeyhrt merged commit 99fde8e into carbon-design-system:master Jan 24, 2020
@abbeyhrt abbeyhrt deleted the progress-indicator-VO branch January 24, 2020 16:39
This was referenced Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AVT 3 - Progress Indicator has several screen reader issues
4 participants