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

fix(Modal): scrolling disabled with multiple modals bug #2255

Closed

Conversation

RetroCraft
Copy link
Contributor

@RetroCraft RetroCraft commented Oct 26, 2017

Fixes #1157, fixes #2248

See the discussion at #1157 for more details, bug was reopened as #2248. The gist of it is that closing a modal removes the scrolling class from body, so if multiple scrollable modals are open the top one closing disables the bottom one's scroll.

@guillaumep
Copy link

guillaumep commented Oct 26, 2017

Wow, that's great, I'm amazed by the responsiveness of the Semantic-UI-React community!

I would have a suggestion for an alternative approach: check if the scrolling class is already present when the modal component wants to add it, and when closing the modal, only remove the class if it was added. This could lead to a more generic fix.

@RetroCraft
Copy link
Contributor Author

I'm really just a high school kid who's got too much time on his hands 😉

I'll see if I can figure something like that out, that sounds like a lot more sense than the workaround I ripped off of that other issue.

@guillaumep
Copy link

It's nice to see you using this free time this way!

@levithomason
Copy link
Member

Thanks for the contribution! Looks like we need to update tests. You can run tests with yarn test:watch:

FAILED TESTS:
  Modal
    dimmer
      false
        ✖ does not render a dimmer
          HeadlessChrome 0.0.0 (Linux 0.0.0)
          AssertionError: document.body should not have class "dimmable", but it has class="dimmable dimmed": expected true to equal false
              at test/utils/assertBodyClasses.js:22:0
              at Array.forEach (<anonymous>)
              at assertBodyClasses (test/utils/assertBodyClasses.js:17:0
              at Context.<anonymous> (test/specs/modules/Modal/Modal-test.js:275:0

    scrolling
      ✖ adds/removes the scrolling class to the body when the window grows/shrinks
        HeadlessChrome 0.0.0 (Linux 0.0.0)
        Error: Uncaught AssertionError: document.body should not have class "scrolling", but it has class="dimmable dimmed scrolling": expected true to equal false (node_modules/chai/lib/chai/assertion.js:141:0

While we're at it, let's add a test with a second scrolling Modal to ensure this works like we think it does. You can see many test examples in Modal-test.js. Specifically, we have tests for the scrolling behavior right here.

@guillaumep
Copy link

I have proposed an alternative approach to this fix in #2394.

@layershifter
Copy link
Member

Closing in favor of #2407 as it promotes more proper solution.

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.

Modal: scrolling issue when using sub-modals Multiple modals scrolling bug
4 participants