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

Prevent scrolling the background when a modal is open #26577

Merged
merged 1 commit into from
Sep 12, 2018

Conversation

DanReyLop
Copy link
Contributor

To test:

  • Go to any page on Calypso that lets you open a modal. For example, go to your site's Settings and change the site language.
  • Try to scroll using your mouse/trackpad while having your mouse pointer outside of the modal.

What happened before:

  • The background page scrolls.

What should happen:

  • The user can't interact with the background page in any way, not even scrolling.

This PR blocks background scrolling in all the modals in Calypso. I think that's the most sensible behaviour, since a modal dialog should prevent the user from interacting with the background entirely, and scrolling the background can be distracting. Can you folks think on any scenario when we would want the background to be scrollable?

@DanReyLop DanReyLop added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Aug 9, 2018
@DanReyLop DanReyLop self-assigned this Aug 9, 2018
@matticbot
Copy link
Contributor

@DanReyLop DanReyLop requested a review from a team August 9, 2018 17:55
@blowery
Copy link
Contributor

blowery commented Aug 10, 2018

Thanks @DanReyLop!

@blowery
Copy link
Contributor

blowery commented Aug 10, 2018

I think this is a sensible default. If we end up with a section that really wants background scrolling for some reason, that section could override the no-scrolling rule and enable it. I can't think of any reason to do that though, unless we're using Modal in a really weird way.

@alisterscott
Copy link
Contributor

Is this one good to go?

@DanReyLop
Copy link
Contributor Author

I was waiting for a :shipit: and then forgot about it.

@blowery are you OK with this change? I'm hesitant to go ahead since I don't know all the implications of changing all of Calypso's modals behaviours.

@blowery
Copy link
Contributor

blowery commented Sep 12, 2018

@DanReyLop I'm ok with it. If we find places where we need background scrolling to work, I'd be surprised, but we can also fix those cases.

Copy link
Contributor

@blowery blowery left a comment

Choose a reason for hiding this comment

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

LGTM

@DanReyLop DanReyLop merged commit cff5cee into master Sep 12, 2018
@DanReyLop DanReyLop deleted the fix/prevent-modal-background-scroll branch September 12, 2018 21:54
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants