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 +67 -11
BehindTheMath commented 2018-01-22 11:50:25 -05:00 (Migrated from github.com)
Copy Link

  • Save scroll position with history

    Save scroll position when navigating away from a page, and restore it when navigating back to that page.

    Fixes #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@4893a2a657

    Fixes #22.

- Save scroll position with history Save scroll position when navigating away from a page, and restore it when navigating back to that page. Fixes #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@4893a2a6574b3490396f83a1863499e8a9ba171e Fixes #22.
MoOx (Migrated from github.com) approved these changes 2018-01-22 12:41:50 -05:00
MoOx (Migrated from github.com) left a comment
Copy Link

Wow, nice improvement!

Wow, nice improvement!
robinnorth (Migrated from github.com) reviewed 2018-01-22 16:31:31 -05:00
index.js
robinnorth (Migrated from github.com) commented 2018-01-22 16:25:24 -05:00
Copy Link

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.

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
@@ -220,3 +243,3 @@
if (state.options.history) {
if (this.firstrun) {
if (!window.history.state) {
this.lastUid = this.maxUid = newUid()
robinnorth (Migrated from github.com) commented 2018-01-22 16:26:00 -05:00
Copy Link

This also should be [0, 0], per my other comment

This also should be `[0, 0]`, per my other comment
index.js
@@ -236,3 +259,3 @@
url: state.href,
title: state.options.title,
uid: this.maxUid
uid: this.maxUid,
robinnorth (Migrated from github.com) commented 2018-01-22 16:31:15 -05:00
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 (Migrated from github.com) reviewed 2018-01-22 17:26:33 -05:00
index.js
BehindTheMath (Migrated from github.com) commented 2018-01-22 17:26:33 -05:00
Copy Link

Good point.

Good point.
BehindTheMath (Migrated from github.com) reviewed 2018-01-22 17:26:41 -05:00
index.js
@@ -220,3 +243,3 @@
if (state.options.history) {
if (this.firstrun) {
if (!window.history.state) {
this.lastUid = this.maxUid = newUid()
BehindTheMath (Migrated from github.com) commented 2018-01-22 17:26:41 -05:00
Copy Link

Good point.

Good point.
BehindTheMath (Migrated from github.com) reviewed 2018-01-22 17:31:56 -05:00
index.js
@@ -236,3 +259,3 @@
url: state.href,
title: state.options.title,
uid: this.maxUid
uid: this.maxUid,
BehindTheMath (Migrated from github.com) commented 2018-01-22 17:31:56 -05:00
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 (Migrated from github.com) reviewed 2018-01-22 18:18:31 -05:00
index.js
@@ -236,3 +259,3 @@
url: state.href,
title: state.options.title,
uid: this.maxUid
uid: this.maxUid,
robinnorth (Migrated from github.com) commented 2018-01-22 18:18:31 -05:00
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 (Migrated from github.com) reviewed 2018-01-22 18:26:16 -05:00
index.js
@@ -236,3 +259,3 @@
url: state.href,
title: state.options.title,
uid: this.maxUid
uid: this.maxUid,
BehindTheMath (Migrated from github.com) commented 2018-01-22 18:26:16 -05:00
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 (Migrated from github.com) reviewed 2018-01-22 18:42:46 -05:00
index.js
@@ -236,3 +259,3 @@
url: state.href,
title: state.options.title,
uid: this.maxUid
uid: this.maxUid,
robinnorth (Migrated from github.com) commented 2018-01-22 18:42:46 -05:00
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.
BehindTheMath commented 2018-01-22 18:57:13 -05:00 (Migrated from github.com)
Copy Link

@robinnorth Are we good to merge this?

@robinnorth Are we good to merge this?
robinnorth (Migrated from github.com) approved these changes 2018-01-22 19:00:44 -05:00
robinnorth (Migrated from github.com) left a comment
Copy Link

LGTM. Nice one!

LGTM. Nice one!
Sign in to join this conversation.
Reviewers
No Reviewers
MoOx
BehindTheMath
robinnorth
Labels
Clear labels
bug
dependencies
Pull requests that update a dependency file
 duplicate
enhancement
invalid
question
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#111
Delete Branch "fix/scroll"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?