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

[added] <Route ignoreScrollBehavior /> to opt out of scroll behavior for itself and descendants #388

Merged
merged 1 commit into from
Oct 15, 2014

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Oct 11, 2014

This PR adds ignoreScrollBehavior on Route component as discussed in #326.

It seems to work on my project but I'd like to hear your opinions on how I should approach testing.
Should I unit-test Routes, ScrollContext, or what?


If you specify ignoreScrollBehavior on a route, scroll behavior will not be triggered when transitioning within that route. It will, however, be triggered when transitioning outside this route. This allows us to support a variety of scenarios where you might want scroll behavior to be ignored:

  • Searching, where path is updated every time you change a query
  • Tabs, where you'd rather not scroll to top when switching between them

In the first case, by specifying ignoreScrollBehavior on search route we ensure that transitions from search to search (which is what happens when you type a search query) will not trigger scroll behavior, but transitions from search to home and back will.

In the second case, by specifying ignoreScrollBehavior on home parent route, we say transitions within home (e.g. from feed to discover and back) don't trigger scroll behavior. However if you go from feed to about, scroll behavior will be respected.

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

(I'll draw a better picture later :-)

By default, scroll behavior is performed on all transitions (red arrows). Routes can create “scrolling realms” for themselves and their descendants by specifying ignoreScrollBehavior (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 of Routes is respected.

@ryanflorence
Copy link
Member

I refuse to accept a "better" picture

On Saturday, October 11, 2014, Dan Abramov notifications@github.com wrote:

This PR adds ignoreScrollBehavior on Route component as discussed
#326 (comment) in
#326 #326.

It seems to work on my project but I'd like to hear your opinions on how
I should approach testing
.

Should I unit-test Routes, ScrollContext, or what?

If you specify ignoreScrollBehavior on a route, scroll behavior will not
be triggered when transitioning within that route. It will, however,
be triggered when transitioning outside this route. This allows us to
support a variety of scenarios where you might want scroll behavior to be
ignored:

  • Searching, where path is updated every time you change a query
  • Tabs, where you'd rather not scroll to top when switching between
    them

In the first case, by specifying ignoreScrollBehavior on search route we
ensure that transitions from search to search (which is what happens when
you type a search query) will not trigger scroll behavior, but
transitions from search to home and back will.

In the second case, by specifying ignoreScrollBehavior on home parent
route, we say transitions within home (e.g. from feed to discover and
back) don't trigger scroll behavior. However if you go from feed to about,
scroll behavior will be respected.

(I'll draw a better picture later :-)

https://cloud.githubusercontent.com/assets/810438/4585743/db9431f0-500a-11e4-9a47-b56116e28cf6.jpg

By default, scroll behavior is performed on all transitions (red arrows).
Routes can create “scrolling realms” for themselves and their descendants
by specifying ignoreScrollBehavior (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 of Routes is respected.

You can merge this Pull Request by running

git pull https://github.com/gaearon/react-router noScroll

Or view, comment on, or merge it at:

#388
Commit Summary

  • [added] to opt out of scroll behavior
    for itself and descendants

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#388.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 11, 2014

🎨

@mjackson
Copy link
Member

I think the best way to test this would be to stub out the updateScroll method and make sure it's called/not called as you expect it to be.

For example, you could setup your routes exactly as you have them here, start at the feed, transition to discover, and make sure that updateScroll wasn't called in the transition. Likewise, you could have another test that starts at discover and transitions to search and assert that updateScroll was indeed called. The actual scroll position should be outside the scope of the test as it doesn't really matter.

@gaearon gaearon force-pushed the noScroll branch 2 times, most recently from 63bea87 to a333734 Compare October 12, 2014 14:48
@gaearon
Copy link
Contributor Author

gaearon commented Oct 12, 2014

@mjackson I added some tests, are they OK?

@ryanflorence
Copy link
Member

@mjackson this looks good to me, you 👍 ?

mjackson added a commit that referenced this pull request Oct 15, 2014
[added] <Route ignoreScrollBehavior /> to opt out of scroll behavior for itself and descendants
@mjackson mjackson merged commit 0a1c41e into remix-run:master Oct 15, 2014
@mjackson
Copy link
Member

Thanks @gaearon ! Looks great.

@iamrandys
Copy link

Thanks @gaearon ! You're way too fast! I was going to start working on this, and found it is already done! Looks like I have some time now, if anything else needs to be done.

gaearon added a commit to gaearon/react-router that referenced this pull request Nov 26, 2014
@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
Development

Successfully merging this pull request may close these issues.

4 participants