-
Notifications
You must be signed in to change notification settings - Fork 116
Tutorial Feedback #2
Description
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
- The
.eslintrc.jsonfile already has the configs in it - My Lighthouse results differ: [shop: 89, about: 96, find: 91]
- Please explain how you identified your list of issues to fix based on the tools and manual testing. Learning how to compile a list is the basis for identifying fixes
- The
- ✅👨🏼💻 Intro to this tool was really helpful. A modification to prevent redundancy if you like:
// Routes
{ path: 'shop', component: ShopComponent, data: { title: 'Our Shop' } },
{ path: 'about', component: AboutComponent, data: { title: 'Our Story' } },
{ path: 'locate', component: LocationComponent, data: { title: 'Find Us' } },
// app component
this.router.events
.pipe(...)
.subscribe((title) => {
const uniqueTitleParts = new Set<string>([title, appTitle]);
this.titleService.setTitle(
Array.from(uniqueTitleParts).join(' | ') // or ' - ' if you prefer
);
});-
✅✅ 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.
- However, per the html spec, heading hierarchy should start at
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. - and, I believe, per WCAG 1.3.1 and 2.4.6
- The way the
h1is being used doesn't represent the page, it represents the site.
From my research, I believe the h1 should be a part of themainlandmark 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 theh1should be what I have in thetitleproperty of the suggested route config above (not containing the entire text of the title but just the most relevant piece).
- However, per the html spec, heading hierarchy should start at
-
❓ 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:
- The information about vegan options is lost
- We didn't see how to actually address the a11y of the existing design
- The color contrast that results from the code snippet using
color="primary"does not meet contrast requirement. Changing it tocolor="accent"does meet the contrast requirement.
-
✅❌ The concept is well conveyed. However:
- while Voiceover is active in Chrome, I am unable to move the slider using the controls dictated to me by Voiceover (or any other key combo).
- This is likely a material issue though.
- The color contrast that results from the code snippet using
color="primary"does not meet contrast requirement. Changing it tocolor="accent"does meet the contrast requirement and matches the screenshot provided in the tutorial.
- while Voiceover is active in Chrome, I am unable to move the slider using the controls dictated to me by Voiceover (or any other key combo).
-
✅✅
-
✅✅ 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:
- Change the text from
Select color: ${color}toSelected color: ${color} - Link to the docs of the service.
- The docs do a great job of calling out why the service exists (testing) over using
aria-liveand would be good to include here.
- Change the text from
-
✅❌ 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.