search

flekschas / svelte-simple-modal

A simple, small, and content-agnostic modal for Svelte v3 and v4
https://svelte.dev/repl/b95ce66b0ef34064a34afc5c0249f313
MIT License
422 stars 30 forks source link

Fix: force "instant" scrollTo behavior #97

Closed FluffyDiscord closed 1 year ago

FluffyDiscord commented 1 year ago

Background

I am using this package for rendering forms in modal and it has currently one annoying issue with forced smooth scroll.

Currently Observed Behavior

I am observing forced smooth scroll in Google Chrome (tested only there) when package scrolls back the page to previous saved position after modal is closed.

New Behavior

Added option to customize window.scrollTo() scroll behavior that can overwrite/force it.

flekschas commented 1 year ago

Thanks for the PR. Could you provide a video or a demo of the forced smooth scroll in Chrome?

FluffyDiscord commented 1 year ago

Thanks for the PR. Could you provide a video or a demo of the forced smooth scroll in Chrome?

I might have explained it improperly. My website is using forced smooth scroll via CSS

:root {
    scroll-behavior: smooth;
}

which window.scrollTo() uses as it's default behavior option. This PR allows me to overwrite this behavior.

RELP with the issue: https://svelte.dev/repl/a9a75697ea244273ba01adedbb18a09f?version=3.20.1

  1. scroll down
  2. click "Show a popup from an outside component!" button
  3. close the modal/popup
  4. see the page smoothly scrolling from top to bottom
flekschas commented 1 year ago

Awesome! Thanks for clarifying.

The code looks good but what I am still debating is why one would want a smooth scroll behavior upon closing the modal? I am tempted to fix it to instant for simplicity.

To explain my thinking: upon opening the modal, the background does not scroll. Internally, we're fixing the scroll y offset but that's really only for internal reasons. So upon closing, I can't seem to come up with a use case where it would be desirable to smoothly scroll back to where one seemingly has always been.

Any thoughts?

FluffyDiscord commented 1 year ago

Well I agree that it does not make sense to use anything besides instant. I just made it to be backwards compatible I guess.

This can be considered as a bug.

Should I change this PR into forcing instant?

flekschas commented 1 year ago

I appreciate the effort and consideration to make this a backward compatible PR but I think we can just treat the current behavior as a bug and enforce instant scrolling. Do you mind adjusting this PR accordingly? Thanks! On Jan 27, 2023 at 2:06 AM -0500, Mossshine @.***>, wrote:

Well I agree that it does not make sense to use anything besides instant. I just made it to be backwards compatible I guess. This can be considered as a bug. Should I change this PR into forcing instant? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>