Skip to content
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

feat: add API to pause scroll position saving

Merged
merged 5 commits into from Dec 18, 2019

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Oct 16, 2019

In Rudy it is possible that updateScroll is not called immediately after a transition (rather it might occur asynchronously afterwards).

Further, the content of the new route might not be rendered immediately. for example, the following things might happen

  1. transitionHook called to indicate that a transition is about to occur
  2. URL changes
  3. loading spinner is displayed as the page content
  4. asynchronous API request is done
  5. final page content is rendered
  6. updateScroll is called to restore the scroll position on the new route

Sometimes, scroll events happen at steps 3 or 5, because the page content height reduces such that it is lower than the current window.scrollY value. In this case, it is not desired to save the scroll position, because it is just noise occurring during the transition.

This adds a flag _isTransitioning which is used to skip saving the scroll position in between calls to the transition hook and updateScroll. The assumption is that updateScroll is always called after every transition.

@taion
Copy link
Owner

taion commented Oct 19, 2019

I'm really sorry for the delay here. We've been swamped at work lately. I'll take a look by early next week. Unfortunately probably not this weekend

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Oct 19, 2019

No problem, thanks for you help so far - I'll be grateful if you can have a look soon.

I'd like to make the tests cover his change but it looks like that would require a bit of surgery on he test setup - to make it call updateScroll not immediately after the URL transition.

@taion
Copy link
Owner

taion commented Oct 26, 2019

can you merge with master to avoid the conflicts with your other PR?

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Oct 27, 2019

conflict fixed

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Nov 13, 2019

any chance of getting to this soon?

@taion
Copy link
Owner

taion commented Nov 13, 2019

sorry, i've been really swamped. i'll look by end of week

Copy link
Owner

@taion taion left a comment

apologies for the delay here

src/index.js Outdated Show resolved Hide resolved
@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Dec 18, 2019

Besides implementing your suggestion for the API, I also added tests (which was easier with this API), and in the process fixed a bug that prevented this feature from working for custom elements.

I think it's ready now.

@hedgepigdaniel hedgepigdaniel changed the title fix: don't save the scroll position between transitionHook and updateScroll feat: add API to pause scroll position saving Dec 18, 2019
taion
taion approved these changes Dec 18, 2019
@taion taion merged commit c57872a into taion:master Dec 18, 2019
3 checks passed
@taion
Copy link
Owner

taion commented Dec 18, 2019

published to v0.9.11

@taion
Copy link
Owner

taion commented Dec 18, 2019

thanks for your help here – sorry for the delay

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Dec 19, 2019

No worries, thankyou!

hedgepigdaniel added a commit to respond-framework/rudy that referenced this issue Dec 20, 2019
Now that taion/scroll-behavior#306 has been merged, this updates the dependency to the released version, including using the modified API that resulted from the review feedback.

Fixes #69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants