Open ScottMGerstl opened 3 years ago
Had a great experience learning about accessibility. Great help. Thank you. There was one area that I couldn't do successfully. The last step with the scss file. I'm getting this:
Compiled with problems:X
ERROR in ./src/app/shop/shop.component.scss
Module build failed (from ./node_modules/sass-loader/dist/cjs.js): SassError: Can't find stylesheet to import. ╷ 19 │ @import "~@angular/cdk/a11y"; │ ^^^^^^^^^^^^^^^^^^^^ ╵ src\app\shop\shop.component.scss 19:9 root stylesheet
First of all, thanks so much for this. I'll be able to use this as a training too for my team as we're starting to build up an a11y competency in our dev team. I went through the entirety and found a few things I'd like to offer back.
The numbered list represents the feedback for each step as it's numbered in the tutorial.
✅✅ Great intro
✅✅ Good setup instructions
✅❌ Good walkthrough for getting the tools set up. However
.eslintrc.json
file already has the configs in it✅👨🏼💻 Intro to this tool was really helpful. A modification to prevent redundancy if you like:
✅✅ Great demo of how to discover and rectify contrast issues though, this does feel very material specific. If you haven't used material before it ends up as a copy/paste and then move on feeling. Maybe referencing the material docs would be helpful
✅❌ Does a great job at demonstrating the separation between semantics and styles in the component.
h1
. When I went looking for theh1
, I found it in the nav component where it's used to style the name of the app which is an incorrect usage based on the html spec.h1
is being used doesn't represent the page, it represents the site. From my research, I believe the h1 should be a part of themain
landmark element. Strategies for correcting this when it comes to making a proper page outline, are to place it in the DOM and potentially hide it for visual users using a screen-reader-only class. I believe the proper content of theh1
should be what I have in thetitle
property of the suggested route config above (not containing the entire text of the title but just the most relevant piece).❓ I'm split on this one. The solution definitely does make the interaction accessible and I think the design is ultimately better but, in the redesign, I feel like we lost 2 things:
color="primary"
does not meet contrast requirement. Changing it tocolor="accent"
does meet the contrast requirement.✅❌ The concept is well conveyed. However:
color="primary"
does not meet contrast requirement. Changing it tocolor="accent"
does meet the contrast requirement and matches the screenshot provided in the tutorial.✅✅
✅✅ Potentially linking to the directive docs could enhance this
✅👨🏼💻 Intro to this service is was great! A couple of things that could improve it though:
Select color: ${color}
toSelected color: ${color}
aria-live
and would be good to include here.✅❌ I appreciated bringing high-contrast mode to light but, at least on my Mac, I could not find a way to turn on high-contrast mode for firefox. Links to documentation on how to test these things would really enhance this section.