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): fix typo in className #2004

Merged
merged 4 commits into from
Sep 1, 2017
Merged

fix(Modal): fix typo in className #2004

merged 4 commits into from
Sep 1, 2017

Conversation

layershifter
Copy link
Member

We have the awesome codepen in #1979. It show that scrolling className isn't removed from body after Portal's unmount. In fact, there was a typo and this PR fixes it.

Fixes #1979.

@codecov-io
Copy link

codecov-io commented Aug 25, 2017

Codecov Report

Merging #2004 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2004   +/-   ##
======================================
  Coverage    99.8%   99.8%           
======================================
  Files         148     148           
  Lines        2561    2561           
======================================
  Hits         2556    2556           
  Misses          5       5
Impacted Files Coverage Δ
src/modules/Modal/Modal.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b60e2aa...7b7d2b9. Read the comment docs.

@levithomason
Copy link
Member

Looking into why our tests for this were passing :)

@levithomason
Copy link
Member

levithomason commented Aug 27, 2017

We were missing quite a bit of coverage on adding/removing classes. I've updated tests and added missing coverage.

Oddly enough, fixing these tests somehow fails the Sticky tests? I may not have time today to figure out why today.

@layershifter
Copy link
Member Author

I'm almost sure that problem is with window.innerHeight usage in tests.

@layershifter
Copy link
Member Author

Yep, working on fix.

@levithomason
Copy link
Member

Heh, oh boy... Thanks!

@levithomason levithomason merged commit 1596486 into master Sep 1, 2017
@levithomason levithomason deleted the fix/modal-scrolling branch September 1, 2017 15:24
@levithomason
Copy link
Member

Released in [email protected]

layershifter added a commit that referenced this pull request Sep 11, 2017
* fix(Modal): fix typo in className

* test(Modal): add missing body class coverage

* test(Modal): restore window.innerHeight after tests
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: scroll breaks if two are adjacent and of different lengths
3 participants