Open callinmullaney opened 2 years ago
@mndonx Thank you for the thorough review! I addressed your inline comments and added variable/block documentation for the twig file. I also made adjustments to the default colors we're using to help address the contrast warnings you mentioned. This will affect all components so hopefully should eliminate this accessibility error for others once merged.
One accessability error you will come across when re-testing is Ensures tabindex attribute values are not greater than 0
- This is intentional as it plays into the keyboard-trap JS and allows the modal to draw focus above everything else on a page.
Lastly, I fixed the icon display issue. Looks like the webpack script needed to explicitly point to the ./images/icons
folder and not just ./images
@joetower @ryanhagerty Adding you to this PR to help move things along if you have time. Can you confirm I addressed Amanda's feedback from above and let me know if this is ready for an accessibility review?
Summary
This PR fixes/implements the following bugs/features
Explain the motivation for making this change. What existing problem does the pull request solve?
Documentation Update (required)
How to review this PR
npm install
thennpm run storybook
Molecules/Modal
and confirm you can see multiple modal options. The functionality for each should be the same. The differences center around the markup that triggers the modal which can be any type of markup. It does not have to be triggered from a button/link only.esc
closes the modal