AutocompleteUI: Close popup when click happens outside of the popover. #44795
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
|
Size Change: +81 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
Mamaduka
added
[Package] Components
BE-Webdesign
force-pushed
the
fix/block-menu-over-modal
branch
from
35b198b to
13f4944
Compare
|
Will fix the linting issues and investigate the failed e2e tests this weekend. |
BE-Webdesign
added
[Status] In Progress
ciampo
reviewed
Oct 18, 2022
There was a problem hiding this comment.
Thank you for working on this, @BE-Webdesign !
Cc'ing @jasmussen and @jameskoster in case they have any feedback, UX-wise.
Also:
- should we add tests ?
- we'll need to add a CHANGELOG entry before merging.
Comment on lines
46
to
48
| useOnClickOutside( popoverRef, () => { | ||
| reset(); | ||
| } ); |
There was a problem hiding this comment.
We're currently passing a new function every render, meaning that the useEffect inside useOnClickOutside will re-run every time.
Instead, we could either:
- pass
resetdirectly:useOnClickOutside( popoverRef, reset ) - extract the callback as a separate variable and wrap in in
useCallback
There was a problem hiding this comment.
Passing reset directly sounds good. I think I accidentally left it this way when I was doing some testing with console.log in the callback.
| @@ -63,6 +70,7 @@ export function getAutoCompleterUI( autocompleter ) { | |||
| id={ listBoxId } | |||
| role="listbox" | |||
| className="components-autocomplete__results" | |||
| ref={ popoverRef } | |||
There was a problem hiding this comment.
Shouldn't this ref be applied to the Popover component, instead of this div ? Currently, clicking on the "padding" of the popover closes the autocomplete list
There was a problem hiding this comment.
Sounds good to me. I think the ref is already forwarded in Popover, so that should work fine.
|
It makes sense to me and seems to be working well. Before before.mp4After after.mp4 |
Cool! I'll have a final round of review once all feedback gets addressed.
When setting up unit tests for this component, you can refer to other components (e.g. Actually, there's a chance that adding unit tests to this component may prove difficult (cc @chad1008 who previously resorted to e2e tests instead) — let us know in case you don't manage and what obstacles you met. Thanks! 🙏 |
|
@ciampo Changes made, and tests added. |
ciampo
approved these changes
Oct 24, 2022
ciampo
force-pushed
the
fix/block-menu-over-modal
branch
from
5ad5f25 to
e7da9fb
Compare
|
PHP unit test failures are not related — we will need to rebase after #45254 is fixed |
…. This prevents the popover from appearing above UI elements like the Patterns Explore Modal window.
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
…a new function on render.
ciampo
force-pushed
the
fix/block-menu-over-modal
branch
from
e7da9fb to
fd6a3ae
Compare
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.
Fixes #44767.
What?
This prevents the popover from appearing above UI elements like the Patterns Explore Modal window.
Why?
When the AutocompleteUI component renders, you can open the explorer for patterns modal window while the AutocompleteUI is still rendered. I don't believe the z-index for CSS should be changed as the modal windows need to appear below WordPress admin elements, where as the popover will be ontop of those.
How?
Adds a handler that looks for when the user clicks outside of the popover and closes the popover.
Testing Instructions
/to open the autocomplete popover for inserting a block.