You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi! Thanks for the codelab! It was interesting and useful. Below are some issues & suggestions:
1. Setup
Issue: While completing this codelab on a MacBook, I ran into an issue following the setup steps.
The issue: npm install (step 5) fails due to conflicting peer dependencies. Running npm install --force works. It seems like either dependencies or instructions should be updated.
Improvement: It would be useful to additionally specify (before or during step 5) that installing npm is a prerequisite for running npm install.
5. Ensure adequate color contrast
Conceptual: Although we fixed the contrast for the home/view type icons, we haven't solved all the high contrast issues on the page i.e. the number picker. It's strange to mark it as 'Done'
7. Create selectable controls with Angular Material
Conceptual: although we have solved the a11y concern, we had to change the intended content of the page. It would be great to show how we can use a11y to improve experience without changing what we need to convey.
10. Control focus with FocusTrap
Issue: For me, the focus is already trapped and does not exit the dialog. Adding cdkFocusInitial did not change anything.
Also, Focus is now initially set on Change Color in the dialog! doesn't make sense because there is no element Change Color.
11. Announce changes with LiveAnnouncer
Improvement: In src/app/shop/color-picker/color-picker-dialog/color-picker-dialog.component.ts, Inject and MAT_DIALOG_DATA should be additionally imported (this might be worth putting into the snippet).
12. Enable HighContrast mode
Issue: Copying code directly produces SassError: Undefined mixin. for cdk-high-contrast(). As per Angular Material docs it should be cdk.high-contrast(), and then the page works.
Additional suggestions:
Improvement: It would be useful to separate ungrouped line changes into different snippets (even within the same file).
Improvement: // TODO: #5. Ensure adequate color contrast in the codebase should have a tailing period to align with the codelab (to search for TODO in the codebase). Same goes for the following TODOs.
The text was updated successfully, but these errors were encountered:
Hi! Thanks for the codelab! It was interesting and useful. Below are some issues & suggestions:
1. Setup
The issue: npm install (step 5) fails due to conflicting peer dependencies. Running npm install --force works. It seems like either dependencies or instructions should be updated.
5. Ensure adequate color contrast
Conceptual: Although we fixed the contrast for the home/view type icons, we haven't solved all the high contrast issues on the page i.e. the number picker. It's strange to mark it as 'Done'
7. Create selectable controls with Angular Material
Conceptual: although we have solved the a11y concern, we had to change the intended content of the page. It would be great to show how we can use a11y to improve experience without changing what we need to convey.
10. Control focus with FocusTrap
Issue: For me, the focus is already trapped and does not exit the dialog. Adding
cdkFocusInitial
did not change anything.Also,
Focus is now initially set on Change Color in the dialog!
doesn't make sense because there is no elementChange Color
.11. Announce changes with LiveAnnouncer
Improvement: In
src/app/shop/color-picker/color-picker-dialog/color-picker-dialog.component.ts
,Inject
andMAT_DIALOG_DATA
should be additionally imported (this might be worth putting into the snippet).12. Enable HighContrast mode
Issue: Copying code directly produces
SassError: Undefined mixin.
forcdk-high-contrast()
. As per Angular Material docs it should becdk.high-contrast()
, and then the page works.Additional suggestions:
// TODO: #5. Ensure adequate color contrast
in the codebase should have a tailing period to align with the codelab (to search for TODO in the codebase). Same goes for the following TODOs.The text was updated successfully, but these errors were encountered: