chore: update modal accessibility improvements #1457
Conversation
|
🚀 Deployed on https://pr-1457--dhis2-ui.netlify.app |
components/modal/src/modal/modal.js
Outdated
| } | ||
| } | ||
|
|
||
| document.addEventListener('keydown', handleKeyDown) |
There was a problem hiding this comment.
I think it's better for this is to have it as a useEffect that subscribes to the event and cleans itself when the component is unmounted. Otherwise we will have an event listener registered forever even after the modal is closed.
const handleKeyDown = (event) => {
if (event.key === 'Escape' && onClose) {
onClose()
}
}
useEffect(() => {
document.addEventListener('keydown', handleKeyDown)
return cleanup = () => {
document.removeEventListener('keydown', handleKeyDown)
}
})There was a problem hiding this comment.
Thanks for the feedback. I have updated this commit with the changes above.
| data-test="dhis2-modal-close-button" | ||
| onClick={createClickHandler(onClick)} | ||
| aria-label="Close modal dialog" |
There was a problem hiding this comment.
for the labels, please use i18n.t('Close modal dialog') - like that, the label will be translated and the correct message in the appropriate language used (not hardcoded to english)
| import { Modal } from './modal.js' | ||
|
|
||
| describe('Modal', () => { | ||
| describe('Accessibility', () => { | ||
| it('closes when ESC key is pressed', () => { |
There was a problem hiding this comment.
As mentioned in the Split Button-PR I think it would make sense to add some test-cases with button component that also handle "ESC keys". Eg. Selects. And test that the modal is not closed if esc is pressed when that is open.
| } | ||
|
|
||
| const handleKeyDown = (event) => { | ||
| event.preventDefault() |
There was a problem hiding this comment.
I'm not sure if preventDefault is necessary, I believe it's just for some HTML-components. Maybe @ismay has some input in regards to this?
There was a problem hiding this comment.
without the prevent default, keys like enter and space bar work as well, what I want is for escape key only to be able to close the modal.
There was a problem hiding this comment.
Looks good to me! Just one question I'm not totally sure of in regards to preventDefault().
Would be great to have some tests for the modal that tests for correct escape-handling with other components like Select and SplitButton inside the modal. But we can add that later, and after the SplitButton PR is merged.
feat: update modal accessibility improvements
kabaros
force-pushed
the
LIBS-567/modal-accessibility
branch
from
21c8086 to
9e7abc3
Compare
Chisomchima
changed the title
feat: update modal accessibility improvements
…to LIBS-567/modal-accessibility
|
|
||
| const createClickHandler = (onClick) => (event) => { | ||
| onClick({}, event) | ||
| } | ||
|
|
||
| export const CloseButton = ({ onClick }) => ( | ||
| <button | ||
| title="Close modal dialog" |
There was a problem hiding this comment.
@Chisomchima the title is still not translated
…to LIBS-567/modal-accessibility
Chisomchima
force-pushed
the
LIBS-567/modal-accessibility
branch
from
6855a96 to
89edb8b
Compare
…essibility' of github.com:dhis2/ui into LIBS-567/modal-accessibility
Implements Jira Ticket
Description
Implements Jira Ticket by updating the Modal CloseButton component to update
titleandaria-labelattributes, as well as the close onClick of the escape buttonKnown issues
Checklist
All points above should be relevant for feature PRs. For bugfixes, some points might not be relevant. In that case, just check them anyway to signal the work is done.
Screenshots
supporting text