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

[Portal] Explore giving more control for layering #760

Closed
jjenzz opened this issue Jul 2, 2021 · 26 comments
Closed

[Portal] Explore giving more control for layering #760

jjenzz opened this issue Jul 2, 2021 · 26 comments
Assignees
Labels
Type: Enhancement Small enhancement to existing primitive/feature

Comments

@jjenzz
Copy link
Contributor

jjenzz commented Jul 2, 2021

We currently apply the maximum z-index on portals and have had a few requests to allow more control here. This ticket is to explore ways of allowing that for a few reasons:

  • People have requested it
  • Can only override all portal instances via the data-radix-portal attribute atm
  • Other Radix components provide an API to control things (consistency)
  • The data-radix-portal attribute is an inconsistent escape hatch that we removed from other comps

An initial discussion on this topic provided a couple ideas:

  • A layer/zIndex prop for portalled parts, e.g. <DialogContent layer={3}>
  • A Portal provider part exposed per component that provides its props to the internal portal, e.g. <DialogPortal style={{ zIndex: 3 }}><DialogContent /></DialogPortal>. The provider suggestion is because we need control internally of how/where/when the portal is rendered.
  • Expose a Portal part for people to wrap when they want portals, e.g. Dialog.Portal. Examples.
  • Use Slot internally on the Portal. This would reduce elements rendered as an additional win but it wouldn't work with:
    • Popper based parts because popper wraps its own additional div for positioning
    • Parts that use https://github.com/theKashey/react-remove-scroll because portal needs to wrap the RemoveScroll part for it to work, and RemoveScroll isn't slot-aware.
  • Any more for any more?
@jjenzz jjenzz added the Type: Enhancement Small enhancement to existing primitive/feature label Jul 2, 2021
@benoitgrelard
Copy link
Collaborator

This is great! Thanks!

One other reason why this is tricky is because most (if not all) of these components have Presence and the concept of a XxxImpl and the Portal needs to be inside the XxxImpl rather than inside the XxxContent to avoid it being rendered at all times.

@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 2, 2021

and the Portal needs to be inside the XxxImpl rather than inside the XxxContent to avoid it being rendered at all times.

On this, I was just about to ask why we need the provider thing also. Because, like all parts we expose, we can control when/how they render still and if they're wrapping it around DialogContent then it will always be around RemoveScroll which solves that ordering issue.

Pseudo-code:

const DialogPortal = (props) => {
	return <Presence present={forceMount || context.open}><Portal {...props} /></Presence>
}

const DialogContent = (props) => {
	const forceMount = context.isInsidePortal || props.forceMount;
	return <Presence present={forceMount || context.open}><DialogContentImpl {...props} /></Presence>
}

@benoitgrelard
Copy link
Collaborator

And then we'd conditionally render the portal for them inside DialogContentImpl if !context.isInsidePortal?

@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 2, 2021

Why do we need to conditionally render the Portal for them if the DialogPortal renders it?

@benoitgrelard
Copy link
Collaborator

Ha ok, you're talking about them always having to render the DialogPortal part then?

@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 2, 2021

Yep, annoying thing there though is that if they want all their dialogs to portal they would have to wrap into their own parts:

export const DialogOverlay = (props) => {
	return <DialogPrimitive.Portal><DialogPrimitive.Overlay {...props} /></DialogPrimitive.Portal>;
}

export const DialogContent = () => {
	return <DialogPrimitive.Portal><DialogPrimitive.Content {...props} /></DialogPrimitive.Portal>;
}

Which isn't ideal, BUT, it does give them the control to do this #345

Also, most people will want to close over things like Tooltip but we don't worry about it there.

@benoitgrelard
Copy link
Collaborator

It's definitely a DX concern. If we were to make all components that can be portalled have to render the portal part, then yes DX wise there's more work to do for each of those.

@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 2, 2021

Yep agreed, that's why we've always avoided this historically and why I suggested the prop but I'm starting to wonder if it's that big of a deal. Most of our parts are required for the majority of use-cases but we always make folks render them too, it's an open API.

The docs would just provide examples they could copy/paste as always.

<Dialog.Root>
  <Dialog.Trigger>Open</Dialog.Trigger>
  <Dialog.Portal><Dialog.Overlay /></Dialog.Portal>
  <Dialog.Portal><Dialog.Content /></Dialog.Portal>
</Dialog.Root>

<Dialog.Root>
  <Dialog.Trigger>Open</Dialog.Trigger>
  <Dialog.Portal>
    <Dialog.Overlay>
      <Dialog.Content />
    </Dialog.Overlay>
  </Dialog.Portal>
</Dialog.Root>

I dunno... I was against it before but starting to re-consider (especially because of #345 which is how I've always composed my dialogs 😅 ). This approach aligns so much better with my opinions on composability. I've always been against boolean props that render parts for this reason because obviously, if we just give the parts to consumers they can choose when to enable them and where to wrap them and can add whatever DOM props they need.

I'd still like to see what else we can come up with though. I feel like the Slot approach would be the ideal when we build our own RemoveScroll but I dunno how we get around the Popper issue. Perhaps we just need to do some thinking there?

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 5, 2021

Another benefit of exposing Portal parts for our components:

https://discord.com/channels/752614004387610674/803656530259738674/872829639742078976

@garand
Copy link

garand commented Aug 12, 2021

Coming here from this: https://discord.com/channels/752614004387610674/875105353568768010/875333950392926238

I would absolutely be in favor of the Portal!

My use case is to be able to center <Dialog.Content /> without the transform method.

@MildTomato
Copy link

MildTomato commented Aug 26, 2021

Not sure if this is revelant, but having a hard time using Dropdown inside a Dialog.

The radix portal seems to add style={{pointer-events:none}} to the body tag, which then the Dropdown inside Dialog directly inherits from body.

There's sometimes some funky stuff like this happening or portals opening on a layer that is unexpected.
I think even being able to control the z-index of the portal so they do not all have a z-index of 2147483647 might be helpful to control the ordering/layering of portals.

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 26, 2021

@MildTomato Is there a particular bug you're experiencing? I've just tried the composition you describe and things appear to be working okay for me: https://codesandbox.io/s/serverless-dust-pvolx

I think even being able to control the z-index of the portal so they do not all have a z-index of 2147483647 might be helpful to control the ordering/layering of portals.

I agree this would be useful and is one of the main requests we get 🤞

@jjenzz
Copy link
Contributor Author

jjenzz commented Sep 2, 2021

Someone wanted to achieve a grid layout with Dialog but isn't able to because we don't give them control of how the Overlay and Content are composed: https://discord.com/channels/752614004387610674/803656530259738674/881179052692602881

Exposing the Dialog.Portal as a separate part would solve this for them.

@MildTomato
Copy link

MildTomato commented Sep 16, 2021

@MildTomato Is there a particular bug you're experiencing?

Sorry - i think i was using older version of both Dialog and Dropdown (or maybe just 1 of them).
Thanks for checking though 🚀 💯
Not experiencing problems anymore

@pstoica
Copy link

pstoica commented Sep 17, 2021

Ran into a similar layout issue. I wanted to render a sidebar dialog, but it can't have the highest z-index and it needs to fill the height of a container. Dialogs couldn't work because the default portal can't be turned off, and Popover is too difficult for this scenario since it depends on an anchor and I can't turn off positioning.

@jjenzz
Copy link
Contributor Author

jjenzz commented Sep 22, 2021

While @vladmoroz was building the new primitives docs site he wanted to inject the dialog into a different container instead of the body. The Portal has a containerRef prop for this purpose but he cannot use it because we don't expose it to consumers. Instead, he will have to use an iframe so preventing this part from being exposed is introducing unnecessary complexity for him.

@ethanresnick
Copy link

I would also love to see this. My use case is that I have a form inside a dialog; if form submission fails, I show an error message in a toast widget [which is more consistent with the rest of our UI than putting the error message inside the dialog content]. Right now, the dialog overlay is covering up the toast, which obviously needs to be on top.

@pdong
Copy link

pdong commented Oct 22, 2021

Having more control here would be really helpful. My use-case is that I'm using react-toastify to show toasts; on mobile I'm using modals as full-page takeovers and sometimes want to show a toast over it. I can use Portal to put my toasts above everything but since they're always max z-index as soon as the overlay and modal mount they're always on top of the toasts. Having a little wiggle room here would be helpful.

I think exposing Portal in the Dialog primitive makes sense and will allow more control over how the Dialog renders. It's definitely a little more overhead but not much more than having to compose all the other parts anyway and will give developers much more control over how things behave.

@jjenzz
Copy link
Contributor Author

jjenzz commented Oct 26, 2021

We've published a release candidate for this in Dialog and AlertDialog to try. The API has breaking changes with this change so we would love feedback on the approach before we roll it out across our other components.

The new anatomy is as follows:

<Dialog.Root>
  <Dialog.Trigger />
  <Dialog.Portal>
    <Dialog.Overlay />
    <Dialog.Content />
  </Dialog.Portal>
</Dialog.Root>

Note:

  • The Dialog.Content can be rendered inside Dialog.Overlay if preferred for styling purposes.
  • The Dialog.Portal doesn't render a div anymore so z-index styles can be applied to Dialog.Overlay/Dialog.Content directly.
  • The Dialog.Portal can portal into a custom container using the container prop:
     const [container, setContainer] = React.useState(null);
     
     return (
       <>
         <Dialog.Root>
           <Dialog.Trigger />
           <Dialog.Portal container={container}>
             <Dialog.Overlay />
             <Dialog.Content />
           </Dialog.Portal>
         </Dialog.Root>
         <div ref={setContainer} />
       </>
     );

@garand
Copy link

garand commented Oct 26, 2021

@jjenzz This is great! Looking forward to updating my component library.

@pdong
Copy link

pdong commented Oct 26, 2021

@jjenzz Thanks! I was able to update to the latest RC and can confirm that it works with the use-case I described earlier and resolved issues I was running into. 👍

@predaytor
Copy link

@jjenzz YES!

staevs added a commit to tablecheck/tablekit that referenced this issue Dec 11, 2021
@radix-ui stopped putting z-index on portal component
so tablekit adjusted to use its own z-indices

reference: radix-ui/primitives#760 (comment)
staevs added a commit to tablecheck/tablekit that referenced this issue Dec 13, 2021
@radix-ui stopped putting z-index on portal component
so tablekit adjusted to use its own z-indices

reference: radix-ui/primitives#760 (comment)
@hipstersmoothie
Copy link

For my tooltips I need to control where the tooltip portal renders. So more control would be really nice here

@sikanhe
Copy link

sikanhe commented Jan 17, 2022

Can we do this for Dropdown as well? I have a dropdown that has drag-n-drop list inside it, and during dragging i have a floating item that needs to be layered above the Dropdown.

@jjenzz
Copy link
Contributor Author

jjenzz commented Jan 17, 2022

@sikanhe The plan is to do it for all of our portalled components I believe.

@benoitgrelard
Copy link
Collaborator

Exploration was done, and validated with Dialog and AlertDialog.
We will bring this to other relevant primitives: #1302, #1303, #1304, #1305, #1306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Small enhancement to existing primitive/feature
Projects
None yet
Development

No branches or pull requests

10 participants