Popover: anchor correctly to parent node when no explicit anchor is passed #42971
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,
chad1008,
andrewserong,
ntsekouras and
aaronrobertshaw
|
Size Change: +19 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
fix/popover-fallback-parent-anchor-position
branch
from
3264602 to
1d32363
Compare
ciampo
commented
Aug 4, 2022
| /> | ||
| { isOpen && ( | ||
| <URLPopover | ||
| anchorRef={ buttonRef } |
There was a problem hiding this comment.
Passing an explicit anchorRef here so that the URL popover is anchored to the button (and not to the whole toolbar).
67 tasks
talldan
reviewed
Aug 9, 2022
talldan
approved these changes
Aug 9, 2022
aaronrobertshaw
approved these changes
Aug 9, 2022
There was a problem hiding this comment.
Thanks for working on this @ciampo 👍
In general, this tests well for me. I didn't find any instances of Popovers without an explicit anchor than those listed in the testing instructions.
I wasn't sure how to test the dop-tip popover but was able to eyeball the others on trunk and this PR.
✅ Pattern transformations menu - No change.
✅ Preview block popover - No change.
✅ URL Input - No change.
❓ URL Popover - Popover is slightly to the left and doesn't align to the block toolbar now. I'd say this is ok but wanted to note it at least.
❓ Legacy widget - Popover position is better but could do with constraining to viewport. Maybe a follow-up?
✅ Color palette panel - Positioning is improved
✅ Gradient palette panel - Positioning is improved
URL Popover Screenshots
| Before | After |
|---|---|
 |
 |
Legacy Widget Popover Videos
| Before | After |
|---|---|
Screen.Recording.2022-08-09.at.4.44.02.pm.mp4 |
Screen.Recording.2022-08-09.at.5.11.29.pm.mp4 |
I don't think these small issues should block this PR at all. Also, a changelog would be handy for this one.
True, it should resize to the height of the viewport. This PR is ten times better than trunk though. I can follow up with a fix for that once this is merged (I just about remember how it should work). |
ciampo
changed the title
ciampo
force-pushed
the
fix/popover-fallback-parent-anchor-position
branch
from
1d32363 to
0ab2981
Compare
|
Thank you both for the reviews! I've fixed the typo and added a CHANGELOG entry.
That makes sense, and it's caused by the fact that I've decided to pass an explicit anchor ref to the I can also drop the changes to I think we would still need to polish the popover's placement anyway top align it with the rest of the popovers that appear when clicking the other toolbar buttons (they all seem to be
Sounds good to me! Enabling I'm assuming we're good to merge one CI is green ✅ and open follow-up PRs where needed as discussed above |
|
Thanks so much for fixing this one up @ciampo, I think this explains (and resolves) a bunch of the confusing issues we've had since the refactor, so it's a great step forward. |
|
The legacy widget popover is still on my radar. I think it will require using floating-ui's It seems like the props of |
Absolutely! Opened #43191 to keep track of this separately (and in case we need to continue the conversation before opening a PR) |
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?
This PR fixes a regression introduced by #40740 which caused the popover to position incorrectly when no explicit anchor is passed via props. In this case, the popover is supposed to use its parent node as the anchor — as per the README:
gutenberg/packages/components/src/popover/README.md
Line 3 in 8f0c091
Why?
Fixing a regression. Here's how the code used to look like before #40740:
gutenberg/packages/components/src/popover/index.js
Lines 123 to 129 in b99e668
How?
The fix is simple — instead of using
fallbackAnchor.currentto compute the anchor positioning, we use its parent node (fallbackAnchor.current.parentNode).This change also required a slight tweak with the styles of the arrow's triangle which somehow seemed to conflict with some floating ui's calculation (switched from nested
absolutepositioning to a simplerdisplay: block)Testing Instructions
Storybook
placementoptions in the default Storybook example, and notice how the popover positions itself correctly with respect to the toggle button.trunkand notice how the popover anchors itself around a single pixel on screen, instead of the full button.Popover(e.g.Tooltip,Dropdown...)Editor
Please do test thoroughly in the editor, as I wasn't always able to "find" all of the popovers listed below while browsing the app in the browser.
This PR should introduce bug fixes (or at least, not cause regressions) for all instances of `Popover` that don't pass an explicit anchor via props. This is what I've found when searching in the codebase: (click to expand)
gutenberg/packages/block-editor/src/components/block-switcher/pattern-transformations-menu.js
Lines 59 to 62 in 5c1e913
gutenberg/packages/block-editor/src/components/block-switcher/preview-block-popover.js
Lines 16 to 20 in d9d06f3
gutenberg/packages/block-editor/src/components/url-input/index.js
Line 542 in 33d84b0
gutenberg/packages/block-editor/src/components/url-popover/index.js
Lines 32 to 38 in 33a9c48
gutenberg/packages/widgets/src/blocks/legacy-widget/edit/form.js
Lines 104 to 108 in 8b14921
Popoveris also used without an anchor in thePaletteEdit, but since the component doesn't have a Storybook example, we'd have to check directly in the editor:gutenberg/packages/edit-site/src/components/global-styles/color-palette-panel.js
Lines 49 to 76 in a5ef504
gutenberg/packages/edit-site/src/components/global-styles/gradients-palette-panel.js
Lines 55 to 82 in 2224d4f
Screenshots or screencast
On
trunk:Kapture.2022-08-04.at.12.25.27.mp4
On this PR:
Kapture.2022-08-04.at.12.23.50.mp4
Editing color palette:
trunk