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

Make scrolling to top in HistoryIntegration optional #564

Open
Dav1dde opened this issue Jan 23, 2023 · 4 comments
Open

Make scrolling to top in HistoryIntegration optional #564

Dav1dde opened this issue Jan 23, 2023 · 4 comments
Labels
A-router Area: router C-enhancement Category: new feature or improvement to existing feature

Comments

@Dav1dde
Copy link
Contributor

Dav1dde commented Jan 23, 2023

Currently the HistoryIntegration click handler includes window.scroll_to_with_x_and_y(0.0, 0.0);, this immediately scrolls to top when the user clicks a navigation link.

This is not always desired, e.g. in the following flow click -> (load resources for new page) -> update view. The router immediately updates but the view is only changed once the resources for the new page are fully fetched. Github has this e.g. while the new page is loading it displays a global loading indicator at the top of the page, the scroll will only be reset after the new page is actually displayed.

Since the router is not aware of futures, the reset would need to happen manually. Alternatively the routers view function could be made async and only once the function completes the router would reset the scroll.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Jan 28, 2023

Actually a full scroll restoration would be better. I was messing around with attaching the scroll value to the browser history state. Unfortunately this breaks on multiple history navigations (forward, backward), because you can't intercept the popstate event, update the state for that route, then switch to the old route.

Two solutions:

  1. Update the state with a debounced onscroll listener
  2. Have an internal store for routes + scroll values

@arctic-hen7
Copy link
Contributor

arctic-hen7 commented Jan 28, 2023

@Dav1dde there's an issue I opened a while ago on scroll state restoration, but to be honest I wonder now if that should be in Sycamore or not. It would seem more natural to put it in something like Perseus (which already has the infrastructure of the page state store), or perhaps a separate library that provides a custom router integration or the like.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Jan 28, 2023

but to be honest I wonder now if that should be in Sycamore or not

I personally think it fits in sycamore-router, which already is a separate library from sycamore.

@lukechu10
Copy link
Member

Yeah this should probably go in sycamore-router. Also we could potentially integrate this with Suspense. Basically, only change URL / reset scroll state when suspense resolves.

Also there should definitely be a way to turn this off completely because there are cases where you don't want to scroll back to the top.

@lukechu10 lukechu10 added C-enhancement Category: new feature or improvement to existing feature A-router Area: router labels Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-router Area: router C-enhancement Category: new feature or improvement to existing feature
Projects
None yet
Development

No branches or pull requests

3 participants