Unify Flare FocusWithin responder with useFocusWithin #18636
Merged
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
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1ddda57:
|
Details of bundled changes.Comparing: 71964c0...1ddda57 react-interactions
Size changes (experimental) |
Details of bundled changes.Comparing: 71964c0...1ddda57 react-interactions
Size changes (stable) |
necolas
reviewed
Apr 16, 2020
|
@necolas The tests were adapted and taken from internal code for |
|
We have a focus hook that is testing itself against a different user space abstraction that has to be built up in the test. The complexity suggests the divisions might not be right because you can't be sure of the behavior without doing this |
|
I've removed the tests from this PR. |
|
I'm not saying whether or not the tests should be part of the PR :) Just that if these sorts of tests are needed to verify the behavior of this hook, then maybe we're missing something about where this functionality belongs |
|
@necolas They're not needed to verify the behavior at all, I just brought them in to verify I didn't make any mistakes, given they pass with it, and I don't anticipate making any more changes, it should be fine. Furthermore, I guess given that we plan on migrating these to FB soon, where they'll get the full test coverage of FocusRegion's tests, it makes sense to leave it till then and not bring them in right now. The sync would fail if we broke something anyway. |
necolas
approved these changes
Apr 16, 2020
trueadm
added a commit
that referenced
this pull request
Apr 17, 2020
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.
This PR unifies the Flare
FocusWithinresponder with the recentuseFocusWithinresponder so the Flare version uses a fakeafterblurrather the fakeblur. This means we can easily A/B test the various implementations that rely on this internally. I've also added FocusRegion tests that confirm the functionality matches that as we expect internally too.