useFocusOutside: rewrite hook to TypeScript, rewrite tests to model RTL and user-event #45317
Conversation
This file contains 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
force-pushed
the
fix/modal-iframe-blur
branch
from
923f415 to
1f298d9
Compare
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
ciampo
changed the title
ciampo
added
[Package] Components
This was referenced Oct 27, 2022
|
This looks good. I appreciate the improvements to the unit tests. Though it looks like these tests need more scrutiny because I can try breaking them by removing the hook and 4/5 still pass. I came across it accidentally while working on a fix for #40912 on another branch. The screenshot is from testing this branch. I'm finding it a bit hard to believe so it'd be great if anyone can try and reproduce it. Not sure if we'd have to address it with this PR because I think it's the same on trunk. UPDATE: |
tyxla
approved these changes
Oct 28, 2022
| @@ -4,7 +4,8 @@ | |||
|
|
|||
| ### Internal | |||
|
|
|||
| - `useDisabled`: Refactor the component to rely on the HTML `inert` attribute. | |||
| - `useDisabled`: Refactor the component to rely on the HTML `inert` attribute ([#44865](https://github.com/WordPress/gutenberg/pull/44865)). | |||
There was a problem hiding this comment.
Thanks for adding the missing PR link 👍
| * | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus | ||
| * | ||
| * @param eventTarget The target from a mouse or touch event. |
There was a problem hiding this comment.
Missing the param type?
Suggested change
| * @param eventTarget The target from a mouse or touch event. | |
| * @param {EventTarget} eventTarget The target from a mouse or touch event. |
There was a problem hiding this comment.
In the past, I've usually avoided having types in JSDocs in TypeScript files, as the code itself is already typed. The only exceptions have been for when the JSDoc types where necessary for automatic doc generation.
Adding types here would also require us to add back a lot of JSDoc extra type def (see the code deletions in this file)
There was a problem hiding this comment.
I generally agree, just wanted to pick your thoughts 👍 Happy to keep it as you suggested.
stokesman
reviewed
Oct 28, 2022
ciampo
force-pushed
the
fix/modal-iframe-blur
branch
from
b952b3b to
6309e63
Compare
|
Thank you both @tyxla and @stokesman for the reviews! I've addressed all minor feedback, but I'll wait before merging until we have an agreement on #45317 (comment).
Interesting, would be curious to see what you're approach is about. I've opened a draft PR but I'm not sure it's a good approach. |
I think we agree there already, so you have my green light ✅ |
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?
While trying to fix #40912 I got to rewrite the hook to TypeScript and the tests to modern RTL techniques (including
user-event)Why?
Better tests and TypeScript as a more solid foundation for working on a tricky problem (which I haven't solved, but I thought I'd push these improvements anyway)
How?
Tests:
user-eventinstead offireEvent(or even imperative events called on each element) where possibledocument.hasFocusTestComponentwrapperHook:
FocusEventHandleretc)useDialogTesting Instructions
Check the
ModalStorybook example (and modals around the codebase) and make sure that they behave as currently ontrunkCode changes can reviewed more easily when approached commit-by-commit