Modal: use code instead of keyCode for keyboard events #43429
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Type] Enhancement
|
Size Change: -2 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
Contributor
Author
I realized that |
ciampo
commented
Aug 19, 2022
| const confirmDialog = screen.getByRole( 'dialog' ); | ||
|
|
||
| //The overlay click is handled by detecting an onBlur from the modal frame. | ||
| // TODO: replace with `@testing-library/user-event` |
Contributor
Author
There was a problem hiding this comment.
I tried to simulate a click on document.body and even on the Modal's overlay, but that was not enough. I therefore decided to leave fireEvent for now to unblock the rest of this PR (same for another test a few lines below)
ciampo
commented
Aug 19, 2022
| const frame = wrapper.baseElement.querySelector( | ||
| '.components-modal__frame' | ||
| ); | ||
| const confirmDialog = screen.getByRole( 'dialog' ); |
Contributor
Author
There was a problem hiding this comment.
.components-modal__frame is the element with role="dialog"
ciampo
commented
Aug 19, 2022
| // TODO: replace with `@testing-library/user-event` | ||
| fireEvent.blur( confirmDialog ); | ||
|
|
||
| jest.useRealTimers(); |
Contributor
Author
There was a problem hiding this comment.
There wasn't any need for fake timers to start with
ciampo
commented
Aug 19, 2022
| 'aria-labelledby', | ||
| 'title-id' | ||
| expect( screen.getByRole( 'dialog' ) ).toHaveAccessibleName( | ||
| 'Modal Title Text' |
Contributor
Author
There was a problem hiding this comment.
The previous test was not actually testing for the computed accessible name
ciampo
commented
Aug 19, 2022
| const titleId = within( dialog ).getByText( 'Test Title' ).id; | ||
| expect( dialog ).toHaveAttribute( 'aria-labelledby', titleId ); | ||
| expect( screen.getByRole( 'dialog' ) ).toHaveAccessibleName( | ||
| 'Modal Title Attribute' |
Contributor
Author
There was a problem hiding this comment.
Same here — the previous test was not actually testing for the computed accessible name
tyxla
approved these changes
Aug 19, 2022
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Beautiful, thanks for fixing this one so quickly 🚀 Also love all the side improvements
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Refactor the
Modalcomponent to rely oncodeinstead ofkeyCodefor keyboard eventsWhy?
keyCodeis deprecated, and replaced bycodeHow?
Easy swap of values
Testing Instructions
On Storybook, make sure that the
Modalstill closes when pressing theEscapekey (if theshouldCloseOnEscproperty is set totrue)