Skip to content

Commit

Permalink
[changed] Don't update scroll if only query has changed
Browse files Browse the repository at this point in the history
Previously, the only way to opt out of scroll updates for a route would be by using `ignoreScrollBehavior`.
This, however, made it hard to implement arguably the most common use case: resetting scroll when `params` change and preserving it when only `query` changes.

This commit completely disables scroll updates when only `query` has changed.
This provides a reasonable default behavior and leaves `ignoreScrollBehavior` for more complicated cases.

If you'd rather keep the old behavior and reset scroll on query changes, you can either promote `query` variables to be route `params` or reset scroll position yourself in response to `query` changes in route handler's `componentDidUpdate`.

Fixes remix-run#432, remix-run#439
  • Loading branch information
gaearon committed Nov 26, 2014
1 parent 0dce2e9 commit 5fe6c08
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 7 deletions.
40 changes: 33 additions & 7 deletions modules/__tests__/Router-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,8 @@ describe('Router.run', function () {
<Route handler={Foo} path='/feed' />
<Route handler={Foo} path='/discover' />
</Route>
<Route path='/search' handler={Foo} ignoreScrollBehavior />
<Route path='/search/:q' handler={Foo} ignoreScrollBehavior />
<Route path='/users/:id/posts' handler={Foo} />
<Route path='/about' handler={Foo} />
</Route>
);
Expand Down Expand Up @@ -406,25 +407,50 @@ describe('Router.run', function () {
});

it('calls updateScroll when no ancestors ignore scroll although source and target do', function () {
TestLocation.push('/search');
TestLocation.push('/search/foo');
expect(didUpdateScroll).toBe(true);
});

it('calls updateScroll when source is same as target and does not ignore scroll', function () {
TestLocation.push('/about?page=2');
it('calls updateScroll when route does not ignore scroll and only params change', function () {
TestLocation.replace('/users/3/posts');
didUpdateScroll = false;

TestLocation.push('/users/5/posts');
expect(didUpdateScroll).toBe(true);
});

it('calls updateScroll when route does not ignore scroll and both params and query change', function () {
TestLocation.replace('/users/3/posts');
didUpdateScroll = false;

TestLocation.push('/users/5/posts?page=2');
expect(didUpdateScroll).toBe(true);
});

it('does not call updateScroll when route does not ignore scroll but only query changes', function () {
TestLocation.replace('/users/3/posts');
didUpdateScroll = false;

TestLocation.push('/users/3/posts?page=2');
expect(didUpdateScroll).toBe(false);
});

it('does not call updateScroll when common ancestor ignores scroll', function () {
TestLocation.push('/discover');
expect(didUpdateScroll).toBe(false);
});

it('does not call updateScroll when source is same as target and ignores scroll', function () {
TestLocation.push('/search');
it('does not call updateScroll when route ignores scroll', function () {
TestLocation.replace('/search/foo');
didUpdateScroll = false;

TestLocation.push('/search?q=test');
TestLocation.push('/search/bar');
expect(didUpdateScroll).toBe(false);

TestLocation.replace('/search/bar?safe=0');
expect(didUpdateScroll).toBe(false);

TestLocation.replace('/search/whatever');
expect(didUpdateScroll).toBe(false);
});
});
Expand Down
7 changes: 7 additions & 0 deletions modules/mixins/Scrolling.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
var invariant = require('react/lib/invariant');
var canUseDOM = require('react/lib/ExecutionEnvironment').canUseDOM;
var getWindowScrollPosition = require('../utils/getWindowScrollPosition');
var Path = require('../utils/Path');

function shouldUpdateScroll(state, prevState) {
if (!prevState) {
return true;
}

var path = state.path;
var routes = state.routes;
var prevPath = prevState.path;
var prevRoutes = prevState.routes;

if (Path.withoutQuery(path) === Path.withoutQuery(prevPath)) {
return false;
}

var sharedAncestorRoutes = routes.filter(function (route) {
return prevRoutes.indexOf(route) !== -1;
});
Expand Down

0 comments on commit 5fe6c08

Please sign in to comment.