Skip to content

[Dialog][AlertDialog] Enable outer scrolling dialog pattern #963

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

Merged
merged 3 commits into from
Nov 8, 2021

Conversation

benoitgrelard
Copy link
Contributor

@benoitgrelard benoitgrelard commented Nov 8, 2021

Fixes #887

I'll explain the thoughts I had and what I did here:

Problem

The main issue left with being able to create an outer scrolling dialog was that we have RemoveScroll in DialogContent, therefore making the overlay non-scrollable too.

Thoughts and solution

  • The overlay is only used when it's a modal dialog, which incidentally is when we want to remove scroll (sure enough, we were doing this in DialogContentModal)
  • So I moved RemoveScroll inside DialogOverlayImpl.
  • The only remaining issue was that we also disable outside pointer events from DialogContentModal, so that was still preventing scrolling the overlay, for that I specifically re-enable them on the overlay.
  • It also required moving the allowPinchZoom prop as it was previously on DialogContent. I remember almost raising this in the previous PR review that I thought perhaps it should live at the root as it's kinda an implementation detail that it was on the "content". I guess this solidified that feeling, we could have moved the prop to the overlay but it would definitely feel odd there I think.

Notes for docs:

  • Make sure the allowPinchZoom prop is documented on the right part (Root).

@benoitgrelard benoitgrelard changed the title [Dialog] Enable outer scrolling dialog pattern [Dialog][AlertDialog] Enable outer scrolling dialog pattern Nov 8, 2021
/**
* @see https://github.com/theKashey/react-remove-scroll#usage
*/
allowPinchZoom?: RemoveScrollProps['allowPinchZoom'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should move this prop in the other components that support it too then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably in a different PR.

@benoitgrelard benoitgrelard requested a review from jjenzz November 8, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrollable Dialog
2 participants