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

Controlling scroll behavior when clicking a <Link> #810

Closed
MadLittleMods opened this issue Feb 10, 2015 · 13 comments
Closed

Controlling scroll behavior when clicking a <Link> #810

MadLittleMods opened this issue Feb 10, 2015 · 13 comments

Comments

@MadLittleMods
Copy link

I am trying to use Route.ignoreScrollBehavior on each route to stop the jump-to-top effect when clicking a <Link> element. But it doesn't seem to do anything because on each page change, the scroll position moves to the top.

Full snippet here
Route.ignoreScrollBehavior

<Route name="all" handler={TodoApp} ignoreScrollBehavior />

Tried this from something I saw in the changelog

<Route name="all" handler={TodoApp}  scrollBehaviour="none" />

I tried setting the scrollBehaviour in Router.create but that doesn't seem to do anything. I also found this confusing because the documentation isn't clear on what to set the value as.

I found some documentation saying you can use strings. I assume this is actually for adding the scrollBehaviour property to a <Route> itself which is deprecated and removed. Strings don't work anyway because it complains about it not having a updateScrollPosition property.

  • 'browser'
  • 'scrollToTop'
  • 'none'

I assume that it wants to use the modules/behaviors that are exported. In the tests, the behaviours are used.

var { ImitateBrowserBehavior, ScrollToTopBehavior } = Router;

Router.create({
    routes: routes,
    scrollBehaviour: ImitateBrowserBehavior // or `ScrollToTopBehavior`
});

Related to issue #432.

@agundermann
Copy link
Contributor

The string API is gone, you only have the two scrollBehaviors for Router.create and the option to add ignoreScrollBehavior to your <Route>s.

I think your problem is that the browser itself does the scrolling. With ignoreScrollBehavior, you just tell react-router to stay out of scrolling, but not that it should override the browser's default behavior.

Sadly, due to browser inconsistencies and the lack of an API, I don't think there's a good way to make a robust PreserveScrollPositionBehavior or something like that without browser sniffing.. Even the current behaviors aren't perfect in that way.

An option could be to relocate the scrollbar using css overflow, but that could also disable some wanted browser features such as "Scroll to top".

Edit: Oops, didn't notice you were talking only about Links, not back button. Ignore my post ;)

@gaearon
Copy link
Contributor

gaearon commented Feb 10, 2015

I am trying to use Route.ignoreScrollBehavior on each route to stop the jump-to-top effect when clicking a element. But it doesn't seem to do anything because on each page change, the scroll position moves to the top.

It works within the route and its children. Applying it to leaf routes only makes transitions within each of them (e.g. param changes) ignoring scroll, but transitions between one and another of them will still reset scroll.

What you want is to put ignoreScrollPosition on the parent route of all routes you want to have this behavior when transitioning from or to each other. For example:

// a->a, ignore scroll
// b->b, ignore scroll
// a->b or b->a, reset scroll
<Route>
  <Route name='a' ignoreScrollBehavior />
  <Route name='b' ignoreScrollBehavior />
</Route>

// a->a, ignore scroll
// b->b, ignore scroll
// a->b or b->a, ignore scroll
<Route ignoreScrollBehavior>
  <Route name='a' />
  <Route name='b' />
</Route>

@MadLittleMods
Copy link
Author

@gaearon I tried it both ways before posting. Trying it again, putting ignoreScrollBehavior on the parent, still causes a scroll-to-top in Chrome and Firefox.

By placing it on individual routes, I thought any transition to that route would not cause a change. It would be nice to have some of that granularity as a feature (although not needed in this case).

@gaearon
Copy link
Contributor

gaearon commented Feb 10, 2015

Any chance you're not immediately rendering the whole content? In this case page will collapse and there's no way for us to restore position afterwards.

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2015

By placing it on individual routes, I thought any transition to that route would not cause a change. It would be nice to have some of that granularity as a feature (although not needed in this case).

I'm afraid this would feel very arbitrary and too easy to misuse. It's really about “zones” in your app where scrolling doesn't switch, not individual routes. Route nesting allows us exactly that.

@MadLittleMods
Copy link
Author

@gaearon When you click a <Link> it renders a new TodoApp with a different filter prop, per transition (check full snippet). I am open to fixing this if this is the real problem.

I also just put the project on GitHub so we can discuss this easier. The only thing that doesn't work in the GitHub version is that you can't directly visit a filtered url.

Live Demo

@gaearon
Copy link
Contributor

gaearon commented Feb 12, 2015

As far as I can see, this is your problem: https://github.com/MadLittleMods/react-flux-todomvc/blob/master/js/app.js#L13

Each time you switch filter, you render a different class.
React bails out of reconciliation in this case and throws away the DOM: http://facebook.github.io/react/docs/reconciliation.html#different-node-types

You can probably change it to always use TodoApp and instead tweak render:

run(function (Handler, state) {
  var lastRoute = state.routes[state.routes.length - 1];
  React.render(<Handler filter={lastRoute.name} />, document.getElementById('todoapp'));
})

@MadLittleMods
Copy link
Author

@gaearon In reality the run callback would have to be a lot more complicated to support the filters that way. This does solve the problem and I have updated the GitHub project with these changes.

Looking at previous issues, etc when you could just pass props directly on the <Route>, this would of been much easier. The reasoning I found for removing that feature was confusion but I think that would be great to add back. I ran into this use case right away.

How are others solving this problem more elegantly?

Link to source

run(function (Handler, state) {
    var filter = TodoConstants.TODO_FILTER_ALL;

    var lastRoute = state.routes[state.routes.length - 1];
    if(lastRoute === 'all') {
        filter = TodoConstants.TODO_FILTER_ALL;
    }
    else if(lastRoute === 'active') {
        filter = TodoConstants.TODO_FILTER_ACTIVE;
    } 
    else if(lastRoute === 'completed') {
        filter = TodoConstants.TODO_FILTER_COMPLETED;
    }

    React.render(<Handler filter={filter} />, document.getElementById('todoapp'));
});

My goal: I can change the filter onClick of the link but I want the benefit of the url changing so someone could get back to their exact spot by sharing the url (even without the same tasks).

@gaearon
Copy link
Contributor

gaearon commented Feb 12, 2015

The reason for removal was that, unlike React normal props, those wouldn't be kept in sync. In fact, there would be no way to update them once set. This confused many people.

Why not use a param instead? Have one route, make filter a route parameter, and pass it as either a prop to Handler (state.params.filter) or read it inside TodoApp from this.getParams().filter using State mixin.

@gaearon
Copy link
Contributor

gaearon commented Feb 12, 2015

As you can see, you're now doing the rendering, not the router. So we can't just pass something as a prop. You need to do it yourself.

@MadLittleMods
Copy link
Author

@gaearon Params are good for lots of situations but sometimes having it directly apart of the URL instead of a GET parameter is better.

For example, many sites use the same structure I am trying to implement when showing tagged content:

http://stackoverflow.com/questions/tagged/react
http://stackoverflow.com/questions/tagged/javascript
http://www.deviantart.com/tag/valentinesday
http://www.deviantart.com/tag/red

@gaearon
Copy link
Contributor

gaearon commented Mar 4, 2015

Closing since this original issue is resolved. (And turned out to be unrelated to scroll behaviors.)

@teameh
Copy link
Contributor

teameh commented Aug 25, 2016

Currently the router doesn't manage scroll positions any more. Try https://github.com/taion/react-router-scroll if you stumble upon this issue while finding a solution for your scroll problem.

@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

No branches or pull requests

4 participants