Enhance scrolling behavior #111
Conversation
Save scroll position when navigating away from a page, and restore it when navigating back to that page. Fixes #30.
When the URL contains a hash, try to find the corresponding element, and if found, scroll to its position. Based on darylteo/pjax@4893a2a Fixes #22.
index.js
Outdated
| window.history.replaceState({ | ||
| url: window.location.href, | ||
| title: document.title, | ||
| uid: this.maxUid | ||
| uid: this.maxUid, | ||
| scrollPos: 0 |
There was a problem hiding this comment.
I think you need to reset scrollPos to be [0, 0] rather than just 0, so that you're always storing the same type of data against this key.
index.js
Outdated
| @@ -235,7 +246,8 @@ Pjax.prototype = { | |||
| window.history.pushState({ | |||
| url: state.href, | |||
| title: state.options.title, | |||
| uid: this.maxUid | |||
| uid: this.maxUid, | |||
| scrollPos: 0 | |||
There was a problem hiding this comment.
This also should be [0, 0], per my other comment
index.js
Outdated
| } | ||
| } | ||
| else { | ||
| window.scrollTo(state.options.scrollPos[0], state.options.scrollPos[1]) |
There was a problem hiding this comment.
This will only run if state.options.history is falsy, which I don't think is what you intended?
Perhaps something like
else if (state.options.scrollTo !== false) {
// Update window scroll position
if (Array.isArray(state.options.scrollTo)) {
window.scrollTo(state.options.scrollTo[0], state.options.scrollTo[1])
}
else if (typeof state.options.scrollTo === "number") {
window.scrollTo(0, state.options.scrollTo)
}
else {
window.scrollTo(state.options.scrollPos[0], state.options.scrollPos[1])
}
}instead?
This would require the default value of this.options.scrollTo to change to true as well -- currently it's set to 0 if one is not provided by the user, meaning that the window will always be scrolled back to the top. If we didn't want to break backwards compatibility, then we could accept 'auto' as a value for the option to trigger scroll restoration, but it's probably a behavior that users would expect and prefer to happen -- what do you think?
There was a problem hiding this comment.
No, I believe my code is correct. options.history is true when we're navigating to a page, and false when the initiator was popstate. This is because you don't want to call pushState() when you're in middle of a popstate event.
So we only want to use the scroll position stored in history when we're responding to a popstate event, in which case options.history is false. Otherwise, when it's true, we want to either use the hash, or scrollTo.
See earlier in #46, where we decided the default when clicking a link should be to scroll to the top.
There was a problem hiding this comment.
Oh dear, that's a comprehension fail on my part, sorry! You are correct.
One thing that would be good to change though is to only restore the scroll position on popstate if this.options.scrollTo is not false -- another thing from #46 was a decision to let developers to decide on what is the best UX for their app, so we should make sure that this scroll restoration can be opted out of, don't you think? This matches the current behaviour when switching.
There was a problem hiding this comment.
options.scrollTo is only documented as taking a number value. From the README:
scrollTo(Integer, default to 0)
I'm not even sure why the check for options.scrollTo !== false exists at all.
I think we should add a scrollRestoration option instead, similar to the HTML spec (see #40).
There was a problem hiding this comment.
Yes, I wondered why it hadn't been documented.
Adding a scrollRestoration option sounds fine though.
|
@robinnorth Are we good to merge this? |
Save scroll position with history
Save scroll position when navigating away from a page, and restore it when navigating back to that page.
Fixes Save scroll position in navigation history #30.
Scroll to element position when URL contains a hash
When the URL contains a hash, try to find the corresponding element, and if found, scroll to its position.
Based on darylteo/pjax@4893a2a
Fixes handle hash in url #22.