Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Popover] Expose new Portal part #1425

Merged
merged 2 commits into from May 31, 2022
Merged

[Popover] Expose new Portal part #1425

merged 2 commits into from May 31, 2022

Conversation

benoitgrelard
Copy link
Collaborator

@benoitgrelard benoitgrelard commented May 26, 2022

Closes #1306

Note: Easier to review without whitespace

The implementation is pretty much the same as what's in Dialog apart from the fact we don't need to map through the children of Portal because it doesn't have an overlay, only content.

I did encounter a surprise though, which was that because of the Popper wrapper, the newly introduced Portal didn't solve the zIndex issue for users, as they could only set it on the Content but not on its wrapper.

I consulted with @jjenzz, we considered a few options:

  • not having a wrapper and expose the popper stuff as CSS variables with a default <style> tag mapping them so they could be overriden
    • ❌ ruled that one out because it would mean users would need to compose transforms when wanting to animate or have their own inner element, etc but basically complicates out of the box animation support
  • exposing the wrapper part
    • ❌ ruled that one out because it brings a lot more complexity to the change and much wider breaking changes everywhere, whilst also potentially complicating animations/presence support
  • ✅ ultimately we took an idea from ariakit's playbook which is to copy the zIndex from content onto the wrapper which means users only have to set it once and it just works.

Note: Because of this change in Popper I will make this branch the base for the others.


Warning
Breaking changes in docs:

  • new Portal part, with forceMount and container props, meaning default has changed (not portalled if not using new part)
  • lose portalled prop on Content
  • different default for forceMount on Content
  • manual z-index management
  • update demos/marketing comps

@benoitgrelard benoitgrelard added PR: Breaking Change This PR contains a breaking change PR: Documentation Needed This PR warrants a documentation update labels May 26, 2022
Copy link
Collaborator

@andy-hook andy-hook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ultimately we took an idea from ariakit's playbook which is to copy the zIndex from content onto the wrapper which means users only have to set it once and it just works.

Nice, yeah I think this is the best of the bunch 👍

Changes look good, my only addition was to add it to the SSR test page and give it a rattle. The issue described in #1386 will of course still apply to all of these updates, but that's a separate issue.

Only other thing is that we should be sure to document the switch to manual z-index management per #1086 (comment).

@andy-hook andy-hook merged commit 085160d into main May 31, 2022
@andy-hook andy-hook deleted the 1306-popover-portal branch May 31, 2022 08:29
luisorbaiceta pushed a commit to luisorbaiceta/primitives that referenced this pull request Jul 21, 2022
* [Popover] Expose new `Portal` part

Closes radix-ui#1306

* [Popover] Add to SSR testing page

Co-authored-by: Andy Hook <hello@andyhook.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Breaking Change This PR contains a breaking change PR: Documentation Needed This PR warrants a documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Popover] Expose Portal part like in Dialog
2 participants