Components: Make the Popover.Slot optional #53889
Merged
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
added
[Type] Enhancement
youknowriad
requested review from
ajitbohra,
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel,
kevin940726,
ellatrix,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
25 tasks
|
Size Change: +81 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
youknowriad
commented
Aug 23, 2023
| @@ -1,75 +0,0 @@ | |||
| /** | |||
There was a problem hiding this comment.
I'm removing this test because IMO, this test is just useless. It tests just the snapshot of the elements which IMO is just internal implementation details that will change every time we change how the component renders.
|
Flaky tests detected in 169561ea773d1a812a29f5d52d71bfc31265424e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5953028216
|
jsnajdr
reviewed
Aug 23, 2023
youknowriad
force-pushed
the
update/make-popover-slot-optional
branch
from
a76e7fb to
d79b6b4
Compare
|
Ok, so I've just realized that trunk also has the "navigation" dropdown offset, so it's not related to iframing or the current PR. Meaning this one should be ready to go. :) |
I don't have a solution yet, just an observation: before this PR, the Storybook popover was rendered inline inside the "Toggle Popover" button. Now it's rendered inside a portal element that's a direct child of the document's body. Seems to me that the Floating UI positioning algorithm runs into troubles in a situation where the reference (anchor) element is in an element that is scrollable ( The Storybook page has a large (300vw x 300vh) scrollable "canvas" element where the "Toggle Popover" button is rendered in its center, and there is an In the PR (#46845) that upgrades Floating UI to latest version and removes all custom iframe positioning, I see a slightly broken behavior in the Site Editor "zoom out mode". And there is also a scrolling element there! |
|
The problem was that the "anchor" was not rendered at the expected position. It should be fixed by #53982 |
7 tasks
|
👋🏼 Just reporting in that I bisected an issue we were having down to f7741a9 in this PR -- Before the pr -- up to version 16.5, we were doing some pop-up calendars with a Team51 
but after, a lot of the styling that would ordinarily propagate through the Emotion js/css library is missing, leading them to look like this: 
When I check the source, when it's behaving, the emotion style attributes seem to line up correctly like this: 
But after this goes in, they don't match properly, so the styles don't seem to apply as they aught: 
I'm a bit out of my league here, but any guidance on if it's an upstream issue, or if we're implementing something incorrectly? I'm happy to add anyone to our plugin repo for direct code access if it can help illuminate the issue. |
|
Hey @georgestephanis , you should be able to get back to the same
Hope this helps! |
|
Thanks for the pointer, @ciampo! I'm making some progress with it -- but the code we've got is using 
I assume it's likely because of where in the hierarchy it is rendering inline, rather than where it used to, higher up in an autogenerated popover slot. 
Is there an example somewhere that I can work off of to recreate the higher slot based rendering to match the prior behavior? |
|
Haven't double-checked, but maybe we just missed applying the |
I think if you apply |
mcsf
reviewed
Oct 16, 2023
| @@ -139,6 +140,19 @@ const AnimatedWrapper = forwardRef( | |||
|
|
|||
| const slotNameContext = createContext< string | undefined >( undefined ); | |||
|
|
|||
| const getPopoverFallbackContainer = () => { | |||
| let container = document.body.querySelector( | |||
There was a problem hiding this comment.
@youknowriad: At the risk of asking a stupid question, does this reliance on a global document carry any consequences for usages of Popover within/across the iframe boundary of block editors?
There was a problem hiding this comment.
no, I don't think there's impact on the iframe slots here. This one is the fallback if there's no Slot defined while if we had to render something within the iframe previously we were forced to have a Slot.
There was a problem hiding this comment.
Maybe to be extra thorough, we could use the anchor or the floating's ownerDocument?
6 tasks
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 11, 2023
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 12, 2023
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 14, 2023
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 19, 2023
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Dec 21, 2023
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Jan 12, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Jan 22, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Jan 22, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 12, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 15, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
costasovo
added a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 16, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
Aschepikov
pushed a commit
to mailpoet/mailpoet
that referenced
this pull request
Feb 19, 2024
It is not longer needed see: WordPress/gutenberg#53889 It was causing TS errors [MAILPOET-5714]
This was referenced Jul 11, 2024
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.
Related #53874
What and why?
Gutenberg can be used as a platform/framework to build block editors. Mainly thanks to the @wordpress/block-editor package. That said, the experience today is not as straightforward as it can be. There can be a lot of small gotchas and hacks you need to do in order to achieve the desired result. One of these small things is the need to manually render
Popover.Slotsomewhere in the React tree.I think ideally, Popover components should work properly out of the box without any provider or slot, and only if we really want to control their position in the DOM, we should use
Popover.Slot.This PR does that by using an "automatic container" for all popovers that is appended to the end of the "body" of the document if there's no slot that is defined manually using
Popover.Slot. I think that's a better default than inlining popovers which breaks all the styles and more.Todo:
Testing Instructions
1- Check that the stories updates (removal of popover and slot provider) still work properly.
2- Navigate the editor and ensure popovers, tooltips, drodowns are shown properly.