Dropdown: anchor popover to the dropdown wrapper (instead of the toggle) #43377
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
added
[Type] Bug
ciampo
requested review from
mirka,
talldan,
ellatrix,
andrewserong and
ntsekouras
This was referenced Aug 18, 2022
ciampo
commented
Aug 18, 2022
Comment on lines
-111
to
-115
| anchorRef={ | ||
| ! hasAnchorRef | ||
| ? containerRef?.current?.firstChild // Anchor to the rendered toggle. | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
This is the main change that introduces the fix
ciampo
commented
Aug 18, 2022
| @@ -66,7 +77,6 @@ $components-custom-gradient-picker__padding: $grid-unit-20; // 48px container, 1 | |||
| .components-custom-gradient-picker__inserter { | |||
| /*rtl:ignore*/ | |||
| direction: ltr; | |||
| width: 100%; | |||
There was a problem hiding this comment.
This was not necessary anymore, since components-custom-gradient-picker__insert-point-dropdown also sets the width
|
Size Change: +15 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ntsekouras
approved these changes
Aug 18, 2022
|
Since this change affects a lot of components in Gutenberg (every dropdown!), it'd be great if we could also get a second pair of eyes checking for any potential regressions. Thank you! |
andrewserong
approved these changes
Aug 19, 2022
There was a problem hiding this comment.
Nice work @ciampo, and good idea reverting part of the change from #41361, I think in general that makes the change here feel reasonably safe — and moving the styling to the dropdown wrapper instead of the button sounds good to me 👍
The only direct issue I ran into was with the Storybook example of the gradient picker, where the Popover appeared behind some of the input fields. I couldn't replicate this in the editor, though, so only encountered it in Storybook. (In the editor it's difficult to get the Popover into a position where it covers the controls, but when I did manage to do that, the Popover was correctly on top of the input fields)
To try to test as exhaustively as I could, I tested the below components in the post and site editors and they were working well for me: (I found these by searching the code base for <Dropdown> but might have missed a couple of obscure components here or there)
Tested components
✅ block-alignment-matrix-control
✅ block-navigation
✅ block-settings-menu
✅ block-switcher
✅ block-variation-transforms
✅ color-style-selector
✅ colors-gradients
✅ duotone-control
✅ inserter
✅ media-replace-flow
✅ preview-options
✅ publish-date-time-picker
✅ format-toolbar
✅ tool-selector
✅ post-date
✅ query-toolbar
✅ video block tracks dropdown
✅ border-control-dropdown
✅ circular-option-picker
✅ color-palette
✅ item-group
✅ navigator
✅ palette-edit
✅ toolbar
✅ toolbar-dropdown-menu
✅ toolspanel
✅ block-placeholder
✅ post-schedule
✅ post-template
✅ post-url
✅ post-visibility
✅ add-new-template
✅ document-actions
✅ actions
✅ sidebar
✅ template-card
✅ template-details
✅ table-of-contents
✅ more-menu-dropdown
Overall, all looking good, with a small caveat:
I ran into an issue with testing the Image block buttons, but fortunately that's been fixed in #43361 — looks like this PR will need a rebase just to make sure those still work. Once it's rebased, we'd then just want to double check that the aspect ratio dropdown in the toolbar is positioned correctly, then I think we'd be good to go.
Otherwise, this PR looks good to go to me, I think we can always look into the Popover z-index in the gradient picker storybook as a separate task, if you wanted to address that separately to unblock merging.
Happy to test further on Monday if need be!
ciampo
force-pushed
the
fix/dropdown-popover-anchor
branch
from
3ec2c5e to
a56b549
Compare
|
Thank you @andrewserong for your thorough testing!
That is a known issue, which I reported first in #41390 and recently in #42994 (should be unrelated to the changes in this PR)
Good idea — i've just rebased |
|
The aspect ratio dropdown looks good after the rebase
I'm going to wait for CI to pass ✅ and likely go ahead and merge |
8 tasks
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?
Part of #42770
This PR:
show button text labels#43327Dropdowncomponent picks its fallback popover anchor)Why?
See #43327 (comment)
How?
The main fix is that the
Dropdowncomponent switches back to using its wrapper/container as thePopoveranchor (instead of the toggle).The rest of the changes are needed to make
CustomGradientPickerwork with the new dropdown anchor — in particular, some style changes were necessary to make sure that the dropdown wrapper for control and insert points were positioned and sized correctly, since they are not used as the popover anchors.Testing Instructions
show button text labels#43327 is fixed on this PRCustomGradientPickercomponent still works as expected (in particular, clicking on the gradient bar on existing control points, or inserting a new control point should reveal a color picker popover positioned like intrunk)show button text labels#43327 (comment) can't be reproduced on this PR (you will need to apply the changes from this PR to the Storybook example)DropdownandDropdownMenuand make sure there aren't regressions compared totrunkScreenshots
DropdownMenuin Storybooktrunk:Kapture.2022-08-18.at.14.04.22.mp4
This PR:
Kapture.2022-08-18.at.14.01.44.mp4
"More" menu in the editor
trunk:Kapture.2022-08-18.at.14.07.15.mp4
This PR:
Kapture.2022-08-18.at.14.08.13.mp4