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: Fix modal causing body padding to increase when unmounted #1899

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

suzubara
Copy link
Contributor

@suzubara suzubara commented Feb 11, 2022

Summary

I fixed this issue by consolidating some of the code that handles DOM changes when the modal is opened & closed, and explicitly resetting body padding when its closed instead of toggling between the two values.

I also added to the docs about how to use yarn link to test the library locally with a NextJS app.

Related Issues or PRs

Fixes #1898

How To Test

  • You can test this on an application that uses the Modal component by causing the modal to mount & unmount (typically by navigating to a new page). Without this fix, the padding-right on the body element would continue to increase by 15px (or the width of your scrollbar)
  • Tested & verified on Space Force
  • I also added a test case for this that was failing without the fix.

@suzubara suzubara requested a review from jim February 11, 2022 20:56
@suzubara suzubara added type: bug Something isn't working like it's supposed to high priority Active blocker for a current project or just urgent labels Feb 11, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Warnings
⚠️ This PR does not include changes to storybook, even though it affects component code.

Generated by 🚫 dangerJS against 7d727fe

Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

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

Nice! I did some smoke testing in deployed storybook, but I trust your local testing with your project code (plus the test coverage here for the issue is great!)

Also glad to see what looks like prettier catching some formatting quirks that snuck through 🥳

The handleOpen/CloseEffect make things nice and clear too 🎉

@suzubara suzubara merged commit fb46e88 into main Feb 11, 2022
@suzubara suzubara deleted the 1898-fix-modal-padding-bug branch February 11, 2022 21:45
@suzubara suzubara mentioned this pull request Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Active blocker for a current project or just urgent type: bug Something isn't working like it's supposed to
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fix] Modal element adds padding to the body element on page navigation
3 participants