[added] preserveScrollPosition Route/Routes props

[ryanflorence]

When a page’s content is long and you transition
to a new route, the scroll position persists
and its super dumb.

This mixin goes on a route handler that you know
has long content and scrolls the window up when
you leave the page.

Not a bad idea to put this on every route handler

closes #121

[ryanflorence]

I initially put this on willTransitionTo, but the entire issue arises when the previous page has long content, so it seems to make sense to do it on the page that causes the problem you want to mitigate, rather than on every potential victim.

[ryanflorence]

I am hesitant to start dabbling in inheritance/object composition in the router.

I wish React dealt with static methods like instance methods.

@spicyj is there some way that React handles mixins with statics?

[sophiebits]

It looks like duplicate statics keys currently create chained functions when both properties are functions, but this behavior doesn't make a ton of sense to me – I'd expect it to give an error. Filed facebook/react#1947.

[mjackson]

@spicyj Thanks for digging in further. I'd expect it to give an error too.

Sounds like we should probably just use a <Route preserveScrollOffset> prop that defaults to false.

[ryanflorence]

I'm going to need some convincing to put it on the <Route/> config :\

[mjackson]

Ok. Then put it in componentWillUnmount instead?

[ryanflorence]

If it goes on componentWillUnmount, and we quit adding keys to our route handlers and instead use our new data stuff + componentWillReceiveProps to make sure transitions from users/1 to users/2 work correctly, then we won't get the scrolling.

[ryanflorence]

do not be fooled by my avatar, I am a real person who uses github.

[ryanflorence]

@spicyj this is what happens for life-cycle hooks though, I actually like that they do it for statics, its the exact behavior I want/expected here :) I say we merge this and if react ever changes the behavior of static mixin methods, then we do something else.

[sophiebits]

I know we do it for lifecycle methods, but that's partly because those don't return a value (shouldComponentUpdate isn't merged). We can talk about if the current behavior is more desirable, but I believe it's a mistake and was intended to throw an error.

[ryanflorence]

Made it a Route and Routes property.

Switched it to scrolling on transition to. It is hard to think about use-cases that don't want to scroll so maybe I'm wrong. I say we ship this and if we're wrong, switch it or add two options, preserverScrollOnTransitionTo and ...From

[ryanflorence]

I had this stuff in one conditional, but I feel like this reads nicer.

[mjackson]

Looks good! 👍

[sophiebits]

I didn't test this yet but if it doesn't already, we should make this restore your previous scroll position properly when pressing back in your browser.

[ryanflorence]

Ah! I thought if that but forgot to add it

Sent from my iPhone

On Aug 7, 2014, at 4:59 PM, Ben Alpert [email protected] wrote:

I didn't test this yet but if it doesn't already, we should make this restore your previous scroll position properly when pressing back in your browser.

—
Reply to this email directly or view it on GitHub.

[iamrandys]

Ahhh, works great guys! Was painless to incorporate and looks really good. Felt so good ripping out my scrollTo code.