iLoveElysia/pjax
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

Enhance scrolling behavior #111

Merged
BehindTheMath merged 4 commits from fix/scroll into master 2018-01-22 20:32:58 -05:00
Conversation 10 Commits 4 Files Changed 4 +2 -2
1 changed files with 2 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 546b9abba3 - Show all commits

4
index.js
View File

@@ -247,7 +247,7 @@ Pjax.prototype = {
url: window.location.href,
title: document.title,
uid: this.maxUid,
scrollPos: 0
scrollPos: [0, 0]
},
document.title)
}
@@ -259,7 +259,7 @@ Pjax.prototype = {
url: state.href,
title: state.options.title,
uid: this.maxUid,
robinnorth commented 2018-01-22 16:31:15 -05:00 (Migrated from github.com)
Review
Copy Link

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?

This will only run if `state.options.history` is falsy, which I don't think is what you intended? Perhaps something like ```js 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?
BehindTheMath commented 2018-01-22 17:31:56 -05:00 (Migrated from github.com)
Review
Copy Link

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.

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.
robinnorth commented 2018-01-22 18:18:31 -05:00 (Migrated from github.com)
Review
Copy Link

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.

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.
BehindTheMath commented 2018-01-22 18:26:16 -05:00 (Migrated from github.com)
Review
Copy Link

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).

`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).
robinnorth commented 2018-01-22 18:42:46 -05:00 (Migrated from github.com)
Review
Copy Link

Yes, I wondered why it hadn't been documented.

Adding a scrollRestoration option sounds fine though.

Yes, I wondered why it hadn't been documented. Adding a `scrollRestoration` option sounds fine though.
scrollPos: 0
scrollPos: [0, 0]
},
state.options.title,
state.href)