-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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): prevent resize loop for full screen height #3024
Conversation
💖 Thanks for opening this pull request! 💖 Here is a list of things that will help get it across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Codecov Report
@@ Coverage Diff @@
## master #3024 +/- ##
==========================================
+ Coverage 99.96% 99.96% +<.01%
==========================================
Files 163 163
Lines 2738 2743 +5
==========================================
+ Hits 2737 2742 +5
Misses 1 1
Continue to review full report at Codecov.
|
@@ -252,7 +252,7 @@ class Modal extends Component { | |||
// SEE: https://github.com/Semantic-Org/Semantic-UI/issues/6185#issuecomment-376725956 | |||
// const marginTop = -Math.round(height / 2) | |||
const marginTop = null | |||
const scrolling = height >= window.innerHeight | |||
const scrolling = height > window.innerHeight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks! Let's fix the test which is currently too loose:
it('adds the scrolling class to the body when taller than the window', (done) => { |
Perhaps we can split this into two tests. One that it('does not add the scrolling class to the body when equal to the window height')
and leave the current test. This new test could set an inline style on the Modal to match the window height that is already set in the test.
This way, we are sure this works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levithomason thanks, that's done :)
This fixes the bug reported here.
My assumptions:
Modal
to override the full screen modal to fill the height of the screen.scrolling
class will continue to be set on modals which are larger thanwindow.innerHeight
.