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: Target window.top.document.body; remove code for adjusting viewport changes #2

Merged
merged 2 commits into from
Oct 20, 2022

Conversation

pbt
Copy link

@pbt pbt commented Oct 19, 2022

There are two main changes here:

  1. Change the target to document.body ➡️ window.top.document.body. This is because in certain situations we might want to invoke the body scroll lock while inside of an iframe and want to make sure locking extends beyond the iframe.
  2. Remove the code that adjusts for viewport changes. While debugging this library I found that this passage of code that attempts to adjust for viewport changes did not work (it was causing the top value to be unset, meaning scroll position was not preserved). I don't think that it ought to be the responsibility of this library to adjust for viewport changes; it should just be to apply a body scroll lock. If they need to, consumers can adjust by listening to changes in visualViewport or using the new CSS display viewport dimensions.

@pbt pbt marked this pull request as ready for review October 20, 2022 16:11
@pbt pbt requested a review from emeraldsanto October 20, 2022 16:12
@emeraldsanto
Copy link
Member

Thanks for the detailed description 😌

@pbt pbt merged commit 557beee into vercel:master Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants