-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Store and restore scroll positions #26
Conversation
…ink` If a user only navigates using the browser's back and forward buttons and scrolls the window and/or some divs in between, the scroll positions will now also be updated.
src/index.js
Outdated
return; | ||
} | ||
// cycle through all elements on the current page and restore their scroll positions, if appropriate | ||
const $elements = document.querySelectorAll('[data-swup-restore-scroll]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use the queryAll
helper here, it converts the NodeList to an array which I think Safari still has problems with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops really? Do you have a source describing the Safari issue? If so, I have been building a lot of broken websites in the last few years 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh. Officially it's supported from Safari 10, so that should never have been an issue. Maybe I'm misremembering and it was Edge? It definitely was an issue as recently as 1/2 years ago, even with Babel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, we should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how I could actually access swups queryAll
helper function from the plugin? I don't really care, would just as well use that instead of querySelectorAll
, if I would have access to it ;)
src/index.js
Outdated
} | ||
|
||
onClickLink = (e) => { | ||
this.deleteStoredScrollPositions(e.delegateTarget.href); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to delete the scroll position on link click?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically Restoring the scroll position should only be done while navigating the history. Otherwise, it should be 0, just like the browser does it. Maybe we could introduce another attribute here, something like [data-swup-history-link]
or the like. Then we could preserve the scroll positions for links matching this attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link would then basically behave for scroll positions like a link with href="javascript:history.back()"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need an attr for changing the behavior, though, it should be fine the way it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just remembered a few cases I had in the past, where clients wanted to go back to a list of posts from a detail page, and the browser back button wasn't enough for them. So we do need a solution for that IMO. This is what I came up with: d10f95d
The new option behaves just like swup's own option skipPopstateHandling
and could be used like this, for example:
{
skipDeletingScrollPositions: linkElement => {
return linkElement.classList.contains('backlink') // don't delete the scroll positions for link elements with the class "backlink"
}
}
src/index.js
Outdated
elements: [] | ||
} | ||
// fill up the object with scroll positions for each matching element | ||
const $elements = document.querySelectorAll('[data-swup-restore-scroll]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the selector configurable? We could also exit early here if a normal query
returns null
and skip the forEach
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add the option for the selector. Right now, this really is just a POC to see what you think. Could you take it for a spin in some little test project?
We could also exit early here if a normal query returns null and skip the forEach.
querySelectorAll
always returns a list, so no need to detect null
and bail early here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added scrollContainersSelector to the options.
I'll take it for a spin later or tomorrow! |
Allows to skip the deletion of scroll positions on `clickLink`, based on a callback.
Just a heads-up, this are the new options in this PR: {
scrollContainersSelector: `[data-swup-scroll-container]`,
skipDeletingScrollPositions: linkElement => {
return linkElement.classList.contains('backlink') // don't delete the scroll positions for link elements with the class "backlink"
}
} |
I have uploaded a test site for this feature to my server: https://test.rassohilber.com/swup-scroll-plugin/test/ How to test:
The bordered div's scroll position should be restored. If you change the option swup.options.animateHistoryBrowsing = true |
Looking really good! I've had a few questions/suggestions I just haven't come around to write up properly, I'll try and get to it tomorrow. |
Working really well, I can see this being super useful in future projects! A few thoughts:
Let me know if you need help with the docs, happy to help with that. |
Thanks for your great feedback!
You mean in keeping with swup's
Great idea! One thing to keep in mind: If we do that and handle window and custom elements the same, the automatic scroll restoration should never be animated (technical limitations in gmrchk/scrl, it only supports the
😄 I was actually going back and forth with the naming of this option, couldn't decide. Will use yours, instead. Also, the singular (
Sounds interesting, looking forward to hear what you are imagining. |
- Remove duplicate code - Avoid unreachable else-statements - Introduce guard clauses - Rename new options
@daun after our chat I did some more work here. The main two features:
To bring some sanity to this update, I did some major cleanup and refactoring in the code. That's why it looks like I have changed a lot, but I haven't, really :) Happy to walk you through in another call! The above test site has been updated with the newest version. |
Looks great and works great, too. The code now is super logical. Is the functionality feature/finalized on your part, i.e ready to merge? I have two minor nitpicks I'll add as comments, but this is great work! Looking forward to have this in the plugin 🥯 |
src/index.js
Outdated
containers: [] | ||
}; | ||
// fill up the object with scroll positions for each matching element | ||
const $containers = document.querySelectorAll(this.options.scrollContainers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the codebase uses the $
prefix for dom elements, I would keep that consistent.
Great to hear! I'll apply your comments and finalize the inline docs, then it's ready to be merged from my side ⛱️ |
src/index.js
Outdated
}; | ||
// fill up the object with scroll positions for each matching element | ||
const $containers = document.querySelectorAll(this.options.scrollContainers); | ||
$containers.forEach((el) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we turn this into a .map
and create the containers object directly, we'd save a few lines. Also maybe reorder to create the containers first, then construct the final entry?
(Untested)
const window = { top: window.scrollY, left: window.scrollX };
const containers = queryAll(this.options.scrollContainers)
.map((el) => ({ top: el.scrollTop, left: el.scrollLeft }));
this.scrollPositionsStore[url] = { window, containers };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I'm old-school, keep forgetting that .map
exists nowadays ;) ...window
can't be used as a variable I think, but still – I will adopt your suggestion partly just to get used to thinking with .map
more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure window
can be assigned to a const
; it's scoped locally to that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry, wrong explanation – you just can't use it if you are immediately referencing window... Anyways, too complicated to explain, wait for the push :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even then, no? You're grabbing the scroll values from the global window object, only then are you creating a new local const. Maybe I have it backwards, but I'm doing this all over the place and it seems to be working 🤡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I type this in the console:
const window = { top: window.scrollY, left: window.scrollX };
I get the error:
Uncaught SyntaxError: Identifier 'window' has already been declared".
If I put it in the program, I get this error:
ReferenceError: Cannot access 'window2' before initialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyways, let's not make this more complicated than it necessarily has to be, shall we? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, you're right, the scope doesn't work out here. I'll stop spreading nonsense now 🧃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏😁
src/index.js
Outdated
* @returns {object} | ||
*/ | ||
getStoredScrollPositions(url) { | ||
return this.scrollPositionsStore[url] || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest not returning the fallback object {}
here. It makes it easier to check for a missing entry elsewhere and it's more obvious what's happening. The check on line 309/310 (scrollPositions.containers == null
) would need to change then to check for null instead of assuming an empty object. But as a new contributor it'd be easier to grok I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we did this, we would have to do scrollPositions && scrollPositions.containers
, also the same for scrollPositions.window
in on line 237. That's why I would prefer the function to always return an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move the || {}
fallback to after the method call. That'd make it just as simple. Returning an empty object is a bit of a code smell as you need to reach into the empty object to even know if you got a return.
const position = this.getStoredScrollPositions(url) || {};
src/index.js
Outdated
|
||
// cycle through all containers on the current page and restore their scroll positions, if appropriate | ||
const $containers = document.querySelectorAll(this.options.scrollContainers); | ||
$containers.forEach((el, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency and simplicity, queryAll(this.options.scrollContainers).forEach()
I added a few comments, let me know what you think or if you agree. |
Ready to merge from my side. I'll create another PR for the docs when this has landed in master. |
Looks good! Ready to merge? Do we need to build dist files again? |
Ah of course. The dist files 🤖 |
👏 |
As described in #24 :
window
and all elements matching the selector[data-swup-scroll-container]
for thepreviousUrl
onwillReplaceContent
contentReplaced
. Also takes care of restoring the window's scroll position, if swup's optionanimateHistoryBrowsing
is set to true.clickLink
, reset the scroll positions for the URL the link points towards.skipDeletingScrollPositions