feat(Modal): Only renders modal in DOM when visible #95
Conversation
Codecov Report
@@ Coverage Diff @@
## master #95 +/- ##
=======================================
Coverage 66.15% 66.15%
=======================================
Files 21 21
Lines 458 458
Branches 93 93
=======================================
Hits 303 303
Misses 124 124
Partials 31 31Continue to review full report at Codecov.
|
|
@mathewmorris Awesome! One note - It looks like the inputs in the Modals have the autoFocus attribute but they're not autofocusing |
|
@kylealwyn Yeah, looks like the focus goes to the Cancel button first (which doesn't have a nice focus state I might add). I will get the autofocus on the input 👍 |
mathewmorris
force-pushed
the
feat/modal-rendering-solution
branch
from
28d3eb4 to
3b55d4a
Compare
| {title} | ||
| </Modal.Title> | ||
| )} | ||
| {title && <Modal.Title role="heading">{title}</Modal.Title>} |
There was a problem hiding this comment.
intentional to remove the tab-index?
There was a problem hiding this comment.
Yes. So it doesn't take the focus when the content of the modal has something that needs autofocus.
| tabIndex="0" | ||
| /> | ||
| </Box> | ||
| <Button |
There was a problem hiding this comment.
I like to specify type="button" incase this component is wrapped into a form.
ghost
requested review from
This is specific to accessibility and testing in general (though it's not a change to make testing work right, this was a problem that was unearthed by testing actually).
Modals and their content were being rendered when not visible, and so readable by screen readers which would not make sense if it's not showing. Now, the modal and it's content will render when it is supposed to be visible. 🎉
Makes the close icon a button. Also, it gives autofocus back to content.
No breaking changes.