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] Make portaled scroll containers scrollable #2250

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joaom00
Copy link
Contributor

@joaom00 joaom00 commented Jul 4, 2023

Description

As the shards of RemoveScroll are not portal friendly, just moving RemoveScroll to Content solves the problem but this approach has shown to have a problem with OuterScrollable setup, i.e. the scroll event does not bubble up to Overlay if it happens in Content. So to cover this case I opted to render RemoveScroll in both Overlay and Content and enable/disable depending on whether Content is inside Overlay or not.

Fixes #1159

@joaom00 joaom00 changed the title Make scroll containers scrollable [Dialog] Make scroll containers scrollable Jul 4, 2023
@joaom00 joaom00 changed the title [Dialog] Make scroll containers scrollable [Dialog] Make portaled scroll containers scrollable Jul 5, 2023
@dogukanoksuz
Copy link

When will it be merged?

@sgrund14
Copy link

excited for this 😄

@kulgavy
Copy link

kulgavy commented Aug 2, 2023

@benoitgrelard @andy-hook please give a look, we're waiting exited, thanks!

@Rieranthony
Copy link

Hey everyone, any news on this? This is also blocking me and my team 👀 Thanks!

@lramos33
Copy link

Up

1 similar comment
@lukasburns
Copy link

Up

@riccardolardi
Copy link

Also waiting for this. Is there any workaround for scrolling inside portaled Popover?

@joaom00
Copy link
Contributor Author

joaom00 commented Oct 24, 2023

Also waiting for this. Is there any workaround for scrolling inside portaled Popover?

Try wrapper the dialog content with the overlay

<Dialog.Overlay>
  <Dialog.Content>
    ...
  </Dialog.Content>
</Dialog.Overlay>

@riccardolardi
Copy link

Also waiting for this. Is there any workaround for scrolling inside portaled Popover?

Try wrapper the dialog content with the overlay

<Dialog.Overlay>
  <Dialog.Content>
    ...
  </Dialog.Content>
</Dialog.Overlay>

Thank you but I'm using the Popover primitive, which does not offer any Overlay. Is there any way to get around it with Popover too?

@joaom00
Copy link
Contributor Author

joaom00 commented Oct 24, 2023

Also waiting for this. Is there any workaround for scrolling inside portaled Popover?

Try wrapper the dialog content with the overlay

<Dialog.Overlay>
  <Dialog.Content>
    ...
  </Dialog.Content>
</Dialog.Overlay>

Thank you but I'm using the Popover primitive, which does not offer any Overlay. Is there any way to get around it with Popover too?

Maybe your issue is different. This PR is for a specific issue when used within Dialog. Would you mind opening a ticket and sharing a reproduction example there? Thanks

@tcolinpa
Copy link

up 👀

1 similar comment
@jgoncalv
Copy link

jgoncalv commented Mar 6, 2024

up 👀

@dogukanoksuz
Copy link

up

@klingat
Copy link

klingat commented May 11, 2024

@joaom00 Thank you for you solution, it fixed my exact problem!

Had my overlay like this:

<Ovelay />
<Content>...</Content>

and changing to this made everything still behave the same, but fixes the scroll issue:

<Overlay>
  <Content>...</Content>
<Overlay/>

kylemcd added a commit to knocklabs/telegraph that referenced this pull request Jun 20, 2024
### Description
The Radix docs seemed to incorrectly layout the example dialog in a way that doesn't allow for scrolling inside of it. Wrapping the Overlay around the content seems to fix that issue, see; radix-ui/primitives#2250 (comment)

### Tasks 
[KNO-6257](https://linear.app/knock/issue/KNO-6257/[dashboard]-unable-to-scroll-in-editor-pop-out)
@chaance chaance added Priority: High High priority issue Resolution: Needs Investigation This issue needs more investigation labels Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority issue Resolution: Needs Investigation This issue needs more investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dialog][Popover] Scrolling issue when Popover inside Dialog