feat: Modal Component #35
Conversation
There was a problem hiding this comment.
This looks great! I have some questions inline, and I also have a question about adherence to accessibility in this stripped-down format. I know we want to keep it simple, but I wonder if, bare minimum, it still passes contrast test. See attached.
Also - does it seem like the close button isn't appearing?
feat: add keyboard trap to modal for ada compliance
|
@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 Lastly, I fixed the icon display issue. Looks like the webpack script needed to explicitly point to the |
|
@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? |
| position: fixed; | ||
| top: 0; | ||
|
|
||
| &--active { |
There was a problem hiding this comment.
Probably want to unnest this to modal__overlay--active to match updated best practices
| response = `<p>Could not process request. Error code: ${request.status}</p>`; | ||
| } | ||
| }; | ||
| request.open('GET', url, false); |
There was a problem hiding this comment.
Why are we including ajax functionality for the modal? This seems very strange to me. The modern standard is fetch, some people still prefer axios, and even fetching data from a modal seems like an edge case.
We should remove this. Kudos for trying to cover all the bases, but let's let the small amount of people doing this decide how they want to implement fetching data.
| toggleChild.getAttribute('href') !== null | ||
| ) { | ||
| e.preventDefault(); | ||
| const request = new XMLHttpRequest(); |
There was a problem hiding this comment.
We should take all this out
| <div {{ bem('content', modal__modifiers, modal__base_class) }}> | ||
| {% block modal__content %} | ||
| {% include "@atoms/text/text/01-paragraph.twig" with { | ||
| paragraph_content: modal_content, |
There was a problem hiding this comment.
Why do we include modal content within a paragraph component? How do we know it will be text content? I think this should be a generic div
| {% set modal__modifiers = modal__modifiers|default([]) %} | ||
|
|
||
| {% if modal__id is empty %} | ||
| {% set modal__id = random(100000) %} |
There was a problem hiding this comment.
I get why you made it random here - in case there's multiple modals right? But part of the accessibility for having the id is to correspond with the label.
Is a screenreader reading modal-35827-heading ideal? This might need a revisit
There was a problem hiding this comment.
Maybe you could set the random id up top and use it for the id and for the label?
ryanhagerty
added
👉 Needs Work
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 installthennpm run storybookMolecules/Modaland 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.esccloses the modal