Add a useDialog hook and replace the duplicated PopoverWrapper #27643
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
youknowriad
requested review from
talldan,
gziolo,
ItsJonQ,
diegohaz,
ellatrix and
ntsekouras
youknowriad
requested review from
adamziel,
ajitbohra,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Size Change: +730 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
It looks like it wouldn't be impossible now to replace all that logic with Reakit: https://reakit.io/docs/dialog/ Just saying 😃 |
|
Indeed, we're getting closer to reakit which is good. We're not there yet though. |
ntsekouras
reviewed
Dec 10, 2020
There was a problem hiding this comment.
Great work here and in all the other PRs to achieve this final one 💯 !
I tested it and works as expected. Code wise looks good as well - just left a minor comment about the documentation and you'll have to replace your comment about mousedown.
Test failure is unrelated and unfortunately axe-core need to be checked again, separately of course.
Having said that, I'd like some other folks with more experience on focus and accessibility to green light it, in case I missed some use case..
|
Since this doesn't add a new stable API and doesn't change the existing behavior (just moves it to a hook) I'm going to merge it. Happy to address follow-ups. |
|
That's great @youknowriad! I wonder what's the reason for returning a tuple of Also, is it possible to |
The main reason is that I want to remove the props and only return the ref. We can't that right now because of useFocusOutside. I believe @talldan has some ideas on how to do that.
I'm not sure I understand the question 100%. Right now, you just apply useDialog returned value to an element. You can do so in two ways:
or if your portal "node" is rendered in React, you can also apply the same hook on it. |
ellatrix
reviewed
Dec 12, 2020
| } | ||
| }, 0 ); | ||
|
|
||
| return () => clearTimeout( focusTimeout ); | ||
| }, [] ); | ||
| }, [ node ] ); |
There was a problem hiding this comment.
If you add a dependency here, the promise of only focussing on mount is broken.
There was a problem hiding this comment.
I don't think so, I'm focusing only when the "node" is mounted, not the component.
ellatrix
reviewed
Dec 12, 2020
| import { __experimentalUseDialog as useDialog } from '@wordpress/compose'; | ||
|
|
||
| const MyDialog = () => { | ||
| const [ ref, extraProps ] = useDialog( { |
There was a problem hiding this comment.
Why is ref separate? Ref is also a prop that can be spread?
There was a problem hiding this comment.
That's still subject to change, my hope is that we can remove the props entirely and just return the ref. more explanations here #27643 (comment)
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 is a follow up to #26355 and the different PRs refactoring our a11y HoCs to hooks.
This PR adds an experimental useDialog to apply all the a11y behaviors required for a popover using a single hook.
I'm marking it experimental because it users the experimental useFocusOutside. Ideally, it would just return a ref (without extra props).