feat(web): persist scroll position on navigation back to album #11388
Conversation
the scroll position is still changed, however it centers the selected item which is more usable than just the top
|
I don't believe it does the same thing. This PR fixes navigation back from a specific album page ( It doesn't affect anything related to the timeline except for a slight fix for the timeline on the search page that uses the GalleryViewer. If that conflicts with the timeline PR, I can just remove that as it's a different bug from the albums and people fixes. |
|
I believe the reason the browser can't do it is because the main scrollable area is nested inside of multiple elements, only the page's scroll position is saved. Doing it this way handles both the browser's back/forward navigation, clicking the in-app back button (which can't use the browser's history as the asset viewer messes up the state), and also for when merging a face navigates back. Internally, SvelteKit does basically the same thing (but using the history API to store a scroll key that can be looked up in session storage, which doesn't cover all 3 of our cases). |
|
It seems like we're basically grabbing and saving the scroll position for an element before navigate and setting it back after navigate. Is there a way we can make this re-usable? Like using an action that binds to an element and saves it's scroll position automatically when enabled? |
|
That's probably doable, one concern is current for the infinite loading on the people page it will load the pages up to the correct one before scrolling, but that could probably be added easily as well. I can take a look at implementing that tomorrow/this weekend. The other thing is there are some guards to clear it when leaving the area of linked pages (like a specific person page clears the people page when going elsewhere) so there isn't some random position the next time you navigate to the people page. That could also be abstracted probably. |
|
@jrasm91 I've moved the repeated code into a set of actions which should make it a lot cleaner. |
|
Keep doing good work. I love immich. I really dislike that it always go to the top. S2 |
alextran1502
changed the title
|
@caburum Hey! I'm going through old PRs to try to get them moved along or closed. What's the state of this PR, was it awaiting review or was this still more work to be done on it? |
|
@zackpollard It was awaiting review, but I should probably check up on it and might have to make some changes since the Svelte 5 upgrade. |
Alright, ping me when you've had a chance to do that and then I'll take a look! Sorry that your PR was left for so long 😅 |
|
@zackpollard Just checked it and everything still seems to be working, I don't think the Svelte 5 upgrade actually affected anything that was changed. |
That's good, I think you may have checked slightly too early though 😅 There were two PRs for svelte 5, one was done a little while back, and one was merged yesterday to mostly move to the new syntax so there are some more merge conflicts, sorry about that... |
|
@zackpollard I've merged in what should be the latest svelte 5 changes & updated the props I changed so it should be good now. |
That's great, thanks! @jrasm91 you had opinions on this being a reusable module which it now is, do you want to re-review it now? |
zackpollard
changed the title
I was going to use
history.stateto store the data originally, but due to the way the asset viewer writes the state and back buttons navigate directly to the page instead of using the history, I had to implement it usingsessionStorageto save the scroll location (which is cleared when leaving the group of pages).Search page doesn't return to the exact position as I couldn't figure out where to hook the navigation properly, but now seems to at least center the clicked item instead of going to the top.
Fixes #11346