[changed] Remove preserveScrollPosition, add scrollStrategy

[gaearon]

Note: This PR lacks docs

This removes support for preserveScrollPosition due to its unforunate naming.
Instead, we introduce three scrolling strategies:

• none: Router doesn't scroll the window when routes change
• scrollToTop (default): Router always scrolls to top when routes change
• imitateBrowser: Router tries to act like browser acts with server-rendered pages: it scrolls to top when clicking on links, but tries to restore position when navigating back and forward

You can only specify these on <Routes /> only.
Per-route overrides are not supported, but you can supply a custom strategy object.

This also fixes #252.

Migration path:

The default changed from what corresponded to imitateBrowser, to scrollToTop.
If router's server-rendered scrolling imitation worked well for you, you must now specify it explicitly:

// before
<Routes>
<Routes preserveScrollPosition={false}>

// after
<Routes scrollStrategy='imitateBrowser'>

If you wish router to not try to manage scrolling, you must opt out:

// before
<Routes preserveScrollPosition={true}>

// after
<Routes scrollStrategy='none'>

Also, as a third option, you may now use the simple scrollToTop strategy.

[gaearon]

Actually now that #252 is fixed here, I don't mind having imitateBrowserScrolling as the default..

[ryanflorence]

I'd personally rather we use None until we know that imitateBrowserScrolling is solid (it might be already, but we don't know that yet, right?)

My reasoning is that a bug with this code is going to be pretty much impossible for most people to realize its us doing it.

When we know its solid, I whole-heartedly want it to be the default.

@mjackson, thoughts?

[ryanflorence]

Also, great work :D

[mjackson]

@gaearon This is really fantastic work. Thank you so much!

Unfortunately I've got a branch going right now that removes the RouteStore (it's for server-side rendering) that is going to conflict with this work. :/ After our discussion yesterday I didn't think it would, but turns out removing RouteStore changes quite a few things in the code.

No worries though. I say let's go ahead and merge this and then I'll do the merge manually.

I'll try not to break your stuff ;)

[gaearon]

Thank you a lot of merging it quickly.

Regarding the default.. I set it to scrollToTop because I think it's still better than none but doesn't have a chance of being buggy. (It's too simple.) I don't think we should change it to none as that would give the idea that router doesn't support scrolling behaviors, and user might spend time implementing them herself.

As for my suggestion of making imitateBrowser the default, I agree we don't know it's rock solid, but if you take a look at the method, it's really straightforward. It doesn't match Chrome behavior 100% but it matches in all ways that matter: Back and Forward are preserving, other actions are not. If you'd rather like this strategy be better tested, I suggest we make it default and write tests cases for it?

Finally, do you think scrollBehavior sounds nicer? Should we change names to that?

[iamrandys]

So we can no longer specify that we don't want the browser to scroll on a per route basis? We have a lot of pages that when you transition from a route to a nested route, the page doesn't scroll, but the URL changes using repalceWith. For instance when you have a search form and then display search results on the same page (URL changes for deep linking, no scrolling needed). Once you have selected a search result, you then want to "check-out" and use the standard browser behavior. This was working great.

What do you suggest we accomplish the same behavior using this new setup?

Simplified router for example:

var Router = (
    <Routes location="history">
        <Route handler={App}>
            <Route name="search"     path="/search.html"    handler={SearchPage}     >
                <Route name="select" path="/select.html"    handler={SearchResults} preserveScrollPosition={true}/>
            </Route>
            <Route name="price"      path="/price.html"     handler={PricePage}     />
            <Route name="purchase"   path="/purchase.html"  handler={PurchasePage}  />
            <Route name="confirmed"  path="/confirmed.html" handler={ConfirmedPage} />
        </Route>
    </Routes>
);

[ryanflorence]

A little background: After screwing around with per-route scrolling it wasn't sufficient. Sometimes you wanted to scroll when leaving a route, other times you wanted to scroll when entering a route, and other times it depends on the action (using forward/back buttons v. clicking on a new link).

Its not documented (yet) but you can supply your own scrolling behavior.

<Routes scrollBehavior={YourScrollBehavior}/>

Check out the ones that ship with the router

https://github.com/rackt/react-router/blob/master/modules/behaviors/ImitateBrowserBehavior.js

Put whatever logic you need in there, you may need a second module that your components send data to in componentWillMount and componentWillUnmount so you can decide when to scroll and when not to.

[iamrandys]

Thanks Ryan! I'll work on it and see if I can get and example together on how the old functionality can be implemented with the new router. I have have a few uses cases for it so I'm sure others will want it as well.

On Oct 7, 2014, at 9:49 PM, Ryan Florence [email protected] wrote:

A little background: After screwing around with per-route scrolling it wasn't sufficient. Sometimes you wanted to scroll when leaving a route, other times you wanted to scroll when entering a route.

Its not documented (yet) but you can supply your own scrolling behavior.

Check out the ones that ship with the router

https://github.com/rackt/react-router/blob/master/modules/behaviors/ImitateBrowserBehavior.js

Put whatever logic you need in there, you may need a second module that your components send scroll positions to in componentWillMount and componentWillUnmount so you can decide when to scroll and when not to.

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

[mjackson]

I don't think it should be too difficult to add back the ability to dictate scrolling behavior on a per-route basis. I honestly didn't think anyone would miss it tho.

[gaearon]

Are you sure it was working correctly before?
It's not so simple to me.

Say I have /search that I don't want to scroll when query changes. With per-Route API, I would give it scrollBehavior=none. But I do want to restore the position when I open product from search and hit back. Similarly, I do want to scroll to top if I go to /search from /about.

What I'm trying to say is I can't see how per-Route behaviors could make sense at all. Behavior always depends on both source and target routes, or it won't be consistent.

[mjackson]

Maybe @iamrandys needs a way to just cancel the default scrolling behavior on certain transitions?

[gaearon]

@mjackson

I see where you're going! Indeed, it looks like his use case is to opt out of any scrolling behavior when transitioning in the bounds of a particular route.

Using this example, select -> price and back should be handled normally, but select -> select should probably not cause updateScroll at all:

    <Routes location="history">
        <Route handler={App}>
            <Route name="search" path="/search.html" handler={SearchPage}>
                <Route name="select" path="/select.html" handler={SearchResults} noScroll />
            </Route>
            <Route name="price" path="/price.html" handler={PricePage} />
            <Route name="purchase" path="/purchase.html" handler={PurchasePage}  />
            <Route name="confirmed" path="/confirmed.html" handler={ConfirmedPage} />
        </Route>
    </Routes>

Say we have noScroll (default is false) on a Route that completely opts it out of scrolling behavior when transition is within that route. This will also allow us to support scenarios with tabs:

    <Routes location="history">
        <Route handler={App}>
            <Route name="home" path="/" handler={HomePage}  noScroll>
                <Route name="feed" path="/" handler={FeedPage} />
                <Route name="discover" path="/discover" handler={DiscoverPage} />
            </Route>
            <Route name="profile"  path="/profile" handler={ProfilePage} />
        </Route>
    </Routes>

Say feed and discover are just tabs on one screen. If the tab switcher is near the top of the screen, it's silly to scroll to top when I switch them. Specifying noScroll on parent (HomePage) helps me achieve zero scrolling behavior as long as I'm switching inside HomePage. However, as soon as I click a user's profile, the behavior specified in Routes kicks in.

Does this make sense?

This is much easier to implement than arbitrary nesting of scroll behaviors, and it should cover most (if not all) use cases when you want scroll not to happen.

[gaearon]

There is also an option of using transition hooks and something like transition.cancelScroll() for that but I'm not sure it's a good idea.

[ryanflorence]

noScroll is just when transitioning between descendant routes?

[gaearon]

@rpflorence Yup, descendant or itself (when on a leaf route).

[gaearon]

    <Routes location="history">
        <Route handler={App}>
            <Route name="A" noScroll>
                <Route name="A1" />
                <Route name="A2" />
            </Route>
            <Route name="B" noScroll />
            <Route name="C">
                <Route name="C1" />
                <Route name="C2" />
            </Route>
        </Route>
    </Routes>

Scrolling behavior doesn't get called for:

• A1 -> A1
• A1 -> A2
• A2 -> A1
• A2 -> A2
• B -> B

Scrolling behavior gets called for:

• A1 -> B
• B -> C1
• C1 -> C2
• C2 -> C1
• C1 -> A1
• etc

[ryanflorence]

or itself (when on a leaf route).

I don't get that part.

[gaearon]

^^^ see comment above

[ryanflorence]

ok

[ryanflorence]

:P

[gaearon]

So what I'm suggesting is noScroll opting out a route and all its descendants from any scrolling behavior whatsoever

[ryanflorence]

the tab navigation use-case is a great example.

I have some doubt that I can't quite surface about being a couple levels deep in a noScroll, and wonder if it should only apply to the immediate children

[gaearon]

Can we warn if you define noScroll inside an already noScroll-d route?

If we only apply behavior to immediate children, adding nested routes kinda breaks your app in unexpected ways.

[gaearon]

Another way to explain this is that noScroll on a Route makes it “leaf” as far as scroll behavior is concerned.

[iamrandys]

Scrolling behavior gets called for:

• A1 -> B ** why would this one scroll? B has noScroll
• B -> C1 ** same with this one
• C1 -> C2
• C2 -> C1
• C1 -> A1

[gaearon]

• A1 -> B ** why would this one scroll? B has noScroll
• B -> C1 ** same with this one

Because these are different sections altogether.
It's like you have a page A with Air/Hotel/Car and page B with Search.

If A has noScroll, then switching between Air/Hotel/Car doesn't scroll.
However when you go from Hotel (A2) to Search (B), you want to scroll to top since it's a different page.

Now that you're in Search (B), your path changes every type you type something (you transition to B again). We don't want to scroll to top, so that's why B has noScroll.

But when you go from Search (B) to About Us (C), we want to scroll.

That was my point: noScroll should only be respected when transition occurs inside its scope.

[gaearon]

<Routes location="history">
  <Route handler={App}>
    <Route name="home" path="/" handler={HomePage} noScroll>
      <Route name="feed" path="/" handler={FeedPage} />
      <Route name="discover" path="/discover" handler={DiscoverPage} />
    </Route>
    <Route name="search" path="/search" handler={SearchResultsPage} noScroll />
    <Route name="about" path="/about" handler={AboutPage} />
  </Route>
</Routes>

By default, scroll behavior is performed on all transitions (red arrows).

Routes can create “scrolling realms” for themselves and their descendants by specifying noScroll.
(Purple areas).

Transitions that are fully confined to a single scrolling realm, don't have any scroll behavior performed (blue arrows). If transition is not fully confined to a scrolling realm, scrollBehavior is respected.

[mjackson]

@gaearon Ah, yes. Thank you for the art. That makes a lot of sense.

@iamrandys Does this work for you? The case you're most concerned about works.

[iamrandys]

Ah, that makes sense. I like it. Then I'll only have to add the noScroll in one place. Oh, I really like this. I will be able to remove a few preserveScrollPosition tags. Will be much cleaner.

[gaearon]

@iamrandys And if you need to preserve position between tabs (air | hotel | car) as well as search results, they just need to be placed under same container route that has noScroll.

[iamrandys]

Yes, which makes perfect sense, since they are all inside the same component. And then all of the other pages will be outside the noScroll route. Which again makes perfect since, because they will look like a completely different pages and don't share the same container. It looks like it all works out. Sweet, this is going to be really nice! Working with the react-router has been really fun. Let me know if I can do anything to help.

[mjackson]

@gaearon @iamrandys If either of you are up for making a PR around this, I would really appreciate it!

Any takers? :D

[gaearon]

@mjackson I can definitely try this weekend!

[ryanflorence]

My only requirement is that the PR includes the artwork.