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(a11y): Adds aria labels to required roles #1327

Merged
merged 5 commits into from
Dec 1, 2020

Conversation

driskull
Copy link
Member

@driskull driskull commented Dec 1, 2020

Related Issue: None

Summary

feat(a11y): Adds aria labels to required roles

…m, tooltip): Adds aria labels to required roles. (a11y)
@driskull driskull added the 1 - assigned Issues that are assigned to a sprint and a team member. label Dec 1, 2020
@driskull driskull added this to the v1.0.0-beta.47 milestone Dec 1, 2020
@driskull driskull requested a review from a team as a code owner December 1, 2020 17:44
@driskull driskull self-assigned this Dec 1, 2020
@paulcpederson
Copy link
Member

Nice, thanks @driskull

@driskull
Copy link
Member Author

driskull commented Dec 1, 2020

The following dependencies couldn't be updated:

  • tailwind (major version change)
  • autoprefixer (major version change)
  • workbox-build (major version change)

Just an FYI for future.

@driskull
Copy link
Member Author

driskull commented Dec 1, 2020

I had to skip a calcite-alert test that is failing color contrast a11y test when auto-dismiss is enabled. I think it might have to do with timing or animation though.

@driskull
Copy link
Member Author

driskull commented Dec 1, 2020

@macandcheese
Copy link
Contributor

Is that result correct? 161 changes? Is that based on files touched and their resultant screenshots or is that saying there are 161 changes based on pixel diffs?

Regarding the alert... yeah auto-dismiss animates like the progress bar... not sure if that's solve-able visually for a11y or if auto-dismiss necessitates a separate kind of aria-role.. always had wondered about that.

@driskull
Copy link
Member Author

driskull commented Dec 1, 2020

It seems it's just the whole calcite-alert failing color contrast. I skipped both tests.

I think the visual screenshot pixel changes could be in regard to updates to storybook or sass.

@eriklharper
Copy link
Contributor

Is that result correct? 161 changes? Is that based on files touched and their resultant screenshots or is that saying there are 161 changes based on pixel diffs?

@macandcheese click the
image icon on an individual screenshot diff in Screener to expand the DOM diffing panel that shows you exactly what changed:

image

Looks like the body element's padding was altered, so the components themselves don't appear to have visual changes.

@eriklharper
Copy link
Contributor

Screener Pro-Tip: Use Shift + Up to Approve Changes and Shift + Down to Reject Changes. Use ArrowRight to move to the next screenshot and ArrowLeft to go to the previous one. Keyboard shortcuts make it so much easier to approve a bunch of diffs.

@macandcheese
Copy link
Contributor

Ok, sounds good.

Re: alert color contrast: #994
Re: "changes" in alert in Screener - that is because of the auto-dismiss bar being at a different %, so if possible should ignore that change in Screener similar to Loader / Progress

@eriklharper
Copy link
Contributor

Ok, sounds good.

Re: alert color contrast: #994
Re: "changes" in alert in Screener - that is because of the auto-dismiss bar being at a different %, so if possible should ignore that change in Screener similar to Loader / Progress

Here are Screener's docs on ignoring: https://www.screener.io/v2/docs/ignore

@macandcheese
Copy link
Contributor

OK, yeah these just seem to be Storybook related padding changes to the viewer container. Going through (with snazzy shortcuts) and verifying...

@eriklharper
Copy link
Contributor

So awesome to finally see us using Screener! 🥇 🎉

@macandcheese
Copy link
Contributor

This one's interesting: https://screener.io/v2/states/1leJJn.Esri-calcite-components/dris0000%2Fa11y-fixes-aXe/1024x768/Chrome/componentscard-dark-theme-footer-text-buttons-tooltips-0

Looks like the change in height of preview container changed where the tooltip is placed.. that seems safe to approve?

@eriklharper
Copy link
Contributor

This one's interesting: https://screener.io/v2/states/1leJJn.Esri-calcite-components/dris0000%2Fa11y-fixes-aXe/1024x768/Chrome/componentscard-dark-theme-footer-text-buttons-tooltips-0

Looks like the change in height of preview container changed where the tooltip is placed.. that seems safe to approve?

I would say yeah, safe to approve.

@driskull
Copy link
Member Author

driskull commented Dec 1, 2020

I'll create an issue to fix the color a11y test

@driskull driskull merged commit e0d8fe3 into master Dec 1, 2020
@driskull driskull deleted the dris0000/a11y-fixes-aXe branch December 1, 2020 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants