Update the popover component to rely on useDialog #27675
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
youknowriad
commented
Dec 11, 2020
|
Size Change: +35 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
6 tasks
Member
|
Sorry, I missed this PR... I already adjusted focus outside in #27700, but had problems with the other two. |
youknowriad
force-pushed
the
update/popover-use-dialog
branch
from
4793a66 to
706d86e
Compare
ellatrix
reviewed
Dec 14, 2020
ellatrix
reviewed
Dec 14, 2020
| } else if ( ! onClickOutside ) { | ||
| if ( onClose ) { | ||
| onClose(); | ||
| } else if ( type === 'focusoutside' && onClickOutside ) { |
Member
There was a problem hiding this comment.
Do you think we can somehow removing this now? How long has it been deprecated?
Contributor
Author
There was a problem hiding this comment.
That's a good question, I don't really know but I think at some point we should start having some regular schedule. If something have been reprecated in the WP version v - 10 (just example), remove it. Something we might want to discuss in #core-js cc @gziolo
Member
There was a problem hiding this comment.
Right. Especially for code that hard to maintain like this. I'm not sure if this code is even tested anywhere?
Contributor
Author
|
There's a lot of failures in e2e tests :( Feeling demotivated now, but I'll circle back to the PR later. |
Member
|
Let's do it in smaller steps. I reopened the PR (#27707) I made yesterday, which just removes the HoCs and replaces it with the corresponding hooks. It's all green, so I'll merge so this can be rebased. |
youknowriad
force-pushed
the
update/popover-use-dialog
branch
from
706d86e to
2138feb
Compare
ellatrix
reviewed
Dec 15, 2020
ellatrix
reviewed
Dec 15, 2020
youknowriad
force-pushed
the
update/popover-use-dialog
branch
from
51d57aa to
8897a58
Compare
youknowriad
commented
Dec 16, 2020
Contributor
Author
|
@talldan @ellatrix I might need some help here. It seems the nested popover onFocusOutside case is not working as expected here (the more menu closes when you open the preferences breaking two e2e tests). I have no idea why though, since this change is supposed to do the exact same things as master. |
youknowriad
force-pushed
the
update/popover-use-dialog
branch
from
762a682 to
fc7ffdd
Compare
Contributor
Author
|
@ellatrix I finally got back to this PR and made everything pass properly. Mind ✅in it? |
ellatrix
reviewed
Jun 11, 2021
| // Ideally the popover should have just a single onClose prop and | ||
| // not three props that potentially do the same thing. | ||
| if ( type === 'focus-outside' && onFocusOutside ) { | ||
| onFocusOutside( event ); |
Member
There was a problem hiding this comment.
Should we deprecate this in the future as well, in favour of onClose?
Contributor
Author
There was a problem hiding this comment.
Sounds like something we could try, I'm not certain yet of the impact.
ellatrix
reviewed
Jun 11, 2021
| const [ dialogRef, dialogProps ] = useDialog( { | ||
| focusOnMount, | ||
| __unstableOnClose: onDialogClose, | ||
| onClose: onDialogClose, |
Member
There was a problem hiding this comment.
What's the difference between these two?
Member
There was a problem hiding this comment.
I also mean: do we need to set both here?
Contributor
Author
There was a problem hiding this comment.
Ideally we should remove __unstableOnClose it's there because Popover need to differentiate between the closing when it happens on focus out and the closing when you use escape... Dialogs in general don't care and if we do your suggestion (merge onFocusOutside and onClose) we should be able to remove the unstable one
ellatrix
reviewed
Jun 11, 2021
ellatrix
reviewed
Jun 11, 2021
youknowriad
added
[Feature] UI Components
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.
Follow-up to #27643
Updates the Popover component to rely on useDialog and remove the duplicated code. I noticed that we have three props in Popover to do the same thing (close the popover): onClickOutside, onFocusOutside and onClose. Ideally we should deprecate the first two ones. The code to keep BC here is not straightforward.