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

[terra-icon] accessibility upgrade #3598

Merged
merged 44 commits into from
Mar 17, 2022
Merged

[terra-icon] accessibility upgrade #3598

merged 44 commits into from
Mar 17, 2022

Conversation

sdadn
Copy link
Contributor

@sdadn sdadn commented Mar 12, 2022

Summary

A major upgrade for terra-icon to meet accessibility guidelines.

Closes #3123

Deployment Link

https://terra-core-deployed-pr-#.herokuapp.com/

Testing

Additional Details

Thank you for contributing to Terra.
@cerner/terra

@sdadn sdadn self-assigned this Mar 12, 2022
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 12, 2022 01:21 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 12, 2022 01:23 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 12, 2022 01:24 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 14, 2022 15:05 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 16, 2022 22:17 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 16, 2022 22:18 Inactive
@jeremyfuksa
Copy link
Contributor

Trying to do VoiceOver testing on the meaningful/decorative icon test. How have you been checking icons with screen readers? I can't get VoiceOver to focus on the icons no matter what I do.

@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 16, 2022 22:48 Inactive
@sdadn
Copy link
Contributor Author

sdadn commented Mar 17, 2022

@jeremyfuksa I did ctrl + shift + option + right to go through the contents of that page after clicking on it.

packages/terra-icon/scripts/src/README.md Outdated Show resolved Hide resolved
@@ -1,10 +1,20 @@
# Update all SVG icons
Copy link
Contributor

Choose a reason for hiding this comment

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

A command could be added for each heading in this file with more than one command under it: update-all-svg-icons, generate-meaningful-icons, generate-decorative-icons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated scripts here: 6e36b79

Copy link
Contributor

@eawww eawww Mar 17, 2022

Choose a reason for hiding this comment

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

Dove a little deeper into this:

  • npm run migrate-cerner-one-icons won't work now because npm generate-icon isn't a script.
    • If npm run generate-all-icons is supposed to be part of npm run migrate-cerner-one-icons, wouldn't all steps in this document be completed in step 1?
  • Under the "Step 2 - Generate meaningful icons" heading:
    • npm run generate-icon isn't a thing now.
    • Once it's changed to reflect the new name for the command, everything but the last command is done directly by the command in step 1.
  • npm run replace-decorative-import is part of the generate-decorative-icons command but isn't in the list of commands in this file to generate decorative icons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh try pulling. I updated the readme and scripts.

@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 17, 2022 18:38 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 17, 2022 19:10 Inactive
@benbcai benbcai temporarily deployed to terra-core-issue-6242-lvndxegc March 17, 2022 20:30 Inactive
@eawww
Copy link
Contributor

eawww commented Mar 17, 2022

(My +1 does not constitute a UX review, by the way. That's still @jeremyfuksa )

Copy link
Contributor

@jeremyfuksa jeremyfuksa left a comment

Choose a reason for hiding this comment

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

Tested: PR #3598
Resolves: Issue #3123

Reviewed:

  • Follows accessibility standards as determined by UX Foundations.

Verified by @jeremyfuksa, Marking as Verified, Ready for Release.

Pass
UX Review

@jeremyfuksa jeremyfuksa added the ⭐ UX Reviewed UX Reviewed and approved. label Mar 17, 2022
@sdadn sdadn merged commit 26077d6 into A11y-MVB Mar 17, 2022
@sdadn sdadn deleted the issue-6242 branch March 17, 2022 22:17
@sdadn sdadn restored the issue-6242 branch March 21, 2022 17:20
@sdadn sdadn deleted the issue-6242 branch March 21, 2022 17:21
sdadn added a commit that referenced this pull request May 11, 2022
sdadn added a commit that referenced this pull request May 11, 2022
sdadn added a commit that referenced this pull request May 11, 2022
sdadn added a commit that referenced this pull request May 11, 2022
sdadn added a commit that referenced this pull request Sep 30, 2022
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.

6 participants