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

[CORL-1227] Fix scroll-able modal position popping #3069

Merged
merged 4 commits into from Aug 5, 2020

Conversation

nick-funk
Copy link
Contributor

@nick-funk nick-funk commented Aug 3, 2020

What does this PR do?

Adds a disableScroll option onto modals and uses it for the SiteModeratorModal.

The site moderator modal already has a scroll bar inside it and its height is less than 480px. We don't need a scrollbar on the modal background since the modal window will always be fully visible on even the smallest screens.

This eliminates the bug of the modal sliding away when there are enough sites in the org to cause the background scroll to be activated.

Fixes the input element positioning on our custom Checkboxes

The checkboxes use a ::before to create a custom styled checkbox that matches our theming. They also hide away the actual input element, which was not being positioned relative to the checkbox root. Because of this, they were appearing relative to the scrolled modal root, sometimes far below the modal's scroll-able area, which shoved the modal way up on the screen.

This positions the checkbox input element relative to the checkbox root and fixes it to the positioning of the stylized checkbox. This prevents it from appearing in weird places due to being inside a scroll-able element or paginated, scroll-able element.

What changes to the GraphQL/Database Schema does this PR introduce?

None

How do I test this PR?

  • Enable the SITE_MODERATOR feature flag
  • Create a lot of sites under your org (10-20 should do)
  • Shrink the height of your window to roughly 600px
  • Go to the community page
  • Go to change a user's role to site moderator
  • Scroll down through your massive list of sites
  • See that the modal background does not have a scroll bar
  • See that the modal does not scroll up and away through the top of the screen

The site moderator modal already has a scroll bar
inside it and its height is less than 480px. We
don't need a scrollbar on the modal background since
the modal window will always be fully visible on
even the smallest screens.

This eliminates the bug of the modal sliding away
when there are enough sites in the org to cause the
background scroll to be activated.

CORL-1227
Copy link
Collaborator

@wyattjoh wyattjoh left a comment

Choose a reason for hiding this comment

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

This still seems to be an issue on Google Chrome:

Screen Shot 2020-08-03 at 6 11 26 PM

@wyattjoh wyattjoh added this to the v6.3.2 milestone Aug 4, 2020
The checkbox input element that we style away
using ::before css was being shoved down the
dom non-relative to the checkbox root.

Now it's relative to the checkbox root and positioned
accordingly.

CORL-1227
@nick-funk nick-funk changed the title [CORL-1227] Disable background scrolling on site moderators modal [CORL-1227] Fix scroll-able modal position popping Aug 4, 2020
@nick-funk nick-funk requested a review from wyattjoh August 4, 2020 16:53
@wyattjoh wyattjoh requested a review from cvle August 4, 2020 17:08
Copy link
Contributor

@tessalt tessalt left a comment

Choose a reason for hiding this comment

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

this makes sense to me

@wyattjoh wyattjoh added the 🚀 merge it! Pull requests that should be merged after status checks pass with a review label Aug 5, 2020
@kodiakhq kodiakhq bot merged commit 88eb955 into master Aug 5, 2020
@kodiakhq kodiakhq bot deleted the bug/CORL-1227-modal-scroll branch August 5, 2020 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 merge it! Pull requests that should be merged after status checks pass with a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants