Skip to content

Update Modal.svelte #45

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 21, 2021
Merged

Update Modal.svelte #45

merged 5 commits into from
Jun 21, 2021

Conversation

truongvan
Copy link
Contributor

fix 'document is not defined' on sveltekit.

@truongvan
Update Modal.svelte
e9c77b5 
fix 'document is not defined'  on sveltekit.
@flekschas
Copy link
Owner

Thanks for this PR. How does svelte.afterUpdate() ensure that disableScroll() and enableScroll() is call at the very same time it's been right now when open() is called?

@flekschas flekschas added the improvement Improved feature label Jun 14, 2021
@truongvan
Copy link
Contributor Author

When open() is called, Component will appear, then disableScroll() will be called. It takes under 10ms from creating Component to calling disableScroll() . If using svelte.beforeUpdate() it takes under 1ms.

@flekschas
Copy link
Owner

It sounds like beforeUpdate() makes more sense then or why did you choose afterUpdate()?

@truongvan
Copy link
Contributor Author

The diffence of the loading time between beforeUpdate() and afterUpdate() is quite small. Besides, I want to make sure Component appears before locking scroll.

@flekschas flekschas self-requested a review June 21, 2021 13:05
flekschas
flekschas approved these changes Jun 21, 2021
View reviewed changes
Copy link
Owner

@flekschas flekschas left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the explanation and the PR!

@flekschas flekschas merged commit bad5fd7 into flekschas:master Jun 21, 2021
flekschas added a commit that referenced this pull request Jun 21, 2021
@flekschas
Update with PR #45
12345b8
@flekschas flekschas mentioned this pull request Jul 2, 2021
Merged
flekschas added a commit that referenced this pull request Jul 2, 2021
@andrew-mc @flekschas
Fix: Only call enable/disableScroll on open/close (#47)
ec8b65a 
* Only call enable/disableScroll once per open/close

Svelte will sometimes update the Modal when variables tracked by its parent (e.g. App) and rendered within the content are updated, meaning enableScroll/disableScroll may be called multiple times. This can cause the page to jump back to the top unexpectedly, and can also cause scrolling to remain disabled after closing the modal.

* Ensure component renders in Vite + SvelteKit

Fixes an issue introduced in #45 

Co-authored-by: Fritz Lekschas <code@lekschas.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flekschas flekschas flekschas approved these changes

Assignees

No one assigned

Labels

improvement Improved feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@truongvan @flekschas