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

[Dialog] Popup can't be nested in the Backdrop part #621

Open
vladmoroz opened this issue Sep 16, 2024 · 1 comment
Open

[Dialog] Popup can't be nested in the Backdrop part #621

vladmoroz opened this issue Sep 16, 2024 · 1 comment
Assignees
Labels
component: alert dialog This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module!

Comments

@vladmoroz
Copy link
Contributor

vladmoroz commented Sep 16, 2024

Summary

Nesting Popup in Backdrop currently ejects the Popup outside of the Backdrop:
https://codesandbox.io/p/sandbox/still-resonance-forked-tzyjgp

It doesn't match what you author, which feels unexpected.

In a practical sense, this is problematic for a couple reasons:

  1. Some apps, e.g. Trello, use scrollable backdrops to show as much content as possible in the viewport. See "Long dialog" on this Radix Themes test page.
  2. This makes migration from Radix non-trivial, as Radix allows nesting its Dialog.Content in Dialog.Overlay

Search keywords

dialog, popup, backdrop, trello, scroll

@vladmoroz vladmoroz added status: waiting for maintainer These issues haven't been looked at yet by a maintainer component: alert dialog This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module! labels Sep 16, 2024
@michaldudak
Copy link
Member

Both Popup and Backdrop are portaled, so they are rendered differently than defined in source code. No matter what you wrap around the Popup, it won't end up being around it in the resulting DOM:

<Dialog.Backdrop>
  <Dialog.Popup />
</Dialog.Backdrop>

// behaves the same conceptually as:

<div>
  <Dialog.Popup />
</div>

I understand the Backdrop->Popup case is more special, and I can think of two solutions:

  1. We detect when a Popup component is placed inside a Backdrop and disable its portal in such cases. This will also affect popups that are nested within the original popup.
  2. We introduce the disablePortal prop on the Popup that lets developers explicitly state their intent.

@michaldudak michaldudak removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 10, 2024
@michaldudak michaldudak changed the title [dialog] Popup can't be nested in the Backdrop part [Dialog] Popup can't be nested in the Backdrop part Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: alert dialog This is the name of the generic UI component, not the React module! component: dialog This is the name of the generic UI component, not the React module!
Projects
Status: Selected
Development

No branches or pull requests

2 participants