Skip to content

[fixed] Window scrolling #249

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

Merged
merged 1 commit into from
Aug 29, 2014
Merged

[fixed] Window scrolling #249

merged 1 commit into from
Aug 29, 2014

Conversation

mjackson
Copy link
Member

The router now remembers the last window scroll position at various
paths and automatically scrolls the window to match after transitions
complete unless preserveScrollPosition=true is used.

This commit also introduces a flux-style architecture to the high-level
transitionTo/replaceWith/goBack methods.

Fixes #189
Fixes #186

The router now remembers the last window scroll position at various
paths and automatically scrolls the window to match after transitions
complete unless preserveScrollPosition=true is used.

This commit also introduces a flux-style architecture to the high-level
transitionTo/replaceWith/goBack methods.

Fixes #189
Fixes #186
@ryanflorence
Copy link
Member

\o/

I was just thinking last night that we needed to finish this feature.

@ryanflorence
Copy link
Member

AND bringing in a dispatcher/actions. this is fantastic

@ryanflorence
Copy link
Member

Interesting implementation.

In a server-rendered app you only return to scroll positions if you use the back button, but it appears that if I navigate with links from a -> b -> a my position will be returned. Maybe this is better?

@mjackson
Copy link
Member Author

In the few quick tests that I did in Chrome it maintained the scroll position regardless of whether I was using the back/forward buttons or links. Makes for a super simple and predictable implementation.

@ryanflorence
Copy link
Member

Yeah, I think its a novel approach, but you've been pretty adamant about
trying to mimick old, boring, server-rendered app behavior.

On Fri, Aug 29, 2014 at 8:22 AM, Michael Jackson [email protected]
wrote:

In the few quick tests that I did in Chrome it maintained the scroll
position regardless of whether I was using the back/forward buttons or
links. Makes for a super simple and predictable implementation.


Reply to this email directly or view it on GitHub
#249 (comment).

@mjackson
Copy link
Member Author

My initial implementation of this feature used a ScrollStore to store the window's scroll positions and used the dispatcher to know when to record the scroll position (immediately before we change the location). However, I found that I had a circular dependency between ScrollStore and PathStore. PathStore needed to waitFor([ ScrollStore.dispatchToken ]) before it actually changed the location object, and ScrollStore needed to use PathStore.getCurrentPath to know what the path was.

So that was the motivation for including the dispatcher in the first place.

@mjackson
Copy link
Member Author

you've been pretty adamant about trying to mimick old, boring, server-rendered app behavior

I just think it makes the most sense as the default since that's what everyone knows already. Mounting a <Route> isn't like mounting other components. They are entry points. Pages.

ryanflorence added a commit that referenced this pull request Aug 29, 2014
@ryanflorence ryanflorence merged commit 4c26d1a into master Aug 29, 2014
@ryanflorence ryanflorence deleted the window-scrolling branch August 29, 2014 14:32
@ryanflorence
Copy link
Member

I'm still a little unsure about clicking a link and getting the scroll position back.

@mjackson
Copy link
Member Author

Well, let's just see how this feels. I agree there will probably be some fine tuning we still need to do.

@@ -0,0 +1,18 @@
var copyProperties = require('react/lib/copyProperties');
var Dispatcher = require('react-dispatcher');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(require('flux') now!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D Awesome!

@sophiebits
Copy link
Contributor

This also sounds a little odd to me but I'm willing to try it.

@ryanflorence
Copy link
Member

@spicyj I keep wondering what is the difference between clicking a link to users/1 and clicking back to users/1?

The only answer I have is "server-rendered apps won't preserve scroll position."

Doesn't seem like a good enough reason to not preserve scroll position.

@bobeagan
Copy link
Contributor

Clicking a link and going to the last scroll position of that page doesn't make sense to me.

@mjackson
Copy link
Member Author

yeah, me neither. should be pretty easy to make the change tho. just need to observe the source of the action. if it was a click, don't do the scroll thing. if it was popstate, do it.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants