Scroll position replacement?

[taion]

https://github.com/rackt/react-router/blob/master/UPGRADE_GUIDE.md#scrolling says we're planning on implementing something before 1.0.0-final. Should we? Not ideal to have users do #2144 (comment).

[knowbody]

that would be cool

[timdorr]

I'd say leave it until after 1.0.0. It's nice to have and improves user experience, but is not critical for functionality.

What would be really cool is if we can use sort sort of middleware to achieve this. That is, we enhance the Router to support it, rather than hacking it in there. Extensibility like that would enable all sorts of other things too.

[knowbody]

oh yeah! this is what I mentioned to @mjackson the other day. Having middleware in place would allow us to build any plugins for React Router

[taion]

I'm on the fence here. I'd forgotten that scroll position wasn't in place. I feel like it's important enough for anything tagged a 1.0.0 release to have this sort of management as part of a complete routing solution, but it might not be worth delaying release further.

[knowbody]

middleware is a breaking change right?

@taion depends when we are releasing. But then the question is, do we want to implement the scrolling now and then change it when we implement middleware?

[DanielSundberg]

I totally understand if this can't be done before 1.0.0.

However, my opinion as a user of react-router, is that I would like this kind of functionality to be part of the core package. React-router is one of more than 20 modules I depend on, I would like to get this working without having to learn about yet another module and plugin architecture.

[timdorr]

If users can implement their own scrolling behavior via transition hooks, then they can wait until after 1.0.0 for us to implement our own solution.

[taion]

I'm 👎 on middleware for this. I can think of two okay ways to do this - let me get some PoCs up to demonstrate.

[timdorr]

I'm not saying we wait on a middleware implementation to provide scrolling behavior. We can build that in and then rip it out after we get some sort of middleware/mixin/plugin/whateverin solution in place.

But what I am saying is we don't need scrolling behavior for 1.0.0. Users have the ability to implement it themselves. It would be a different story entirely if the functionality wasn't even possible. I say we close this and release 1.0.0 right now rather than wait on an enhancement.

[taion]

One of the ways was dumb. I PRed the other way as a PoC on #2471.

[LDMFD]

If I may chime in, please consider this. I'm about to launch about 4 months of work that uses react-router 1.0.0-rc1. This is a public-facing website for a sales business that has long vertically scrolling pages - it is out of the question to launch with broken scrolling behaviour.

The reason I ended up using 1.0.0-rc1 is essentially that I was struggling with React's server-side rendering, and I ended up getting it working with rc1. I kinda wish I'd stuck with 0.13 but launching a new front-end with a nearly-outdated version of a lib isn't ideal either.

I'm implementing #2144's comment now and crossing my fingers it's going to work - but if there is an officially supported and documented method for preserving expected browser behaviour, this would be massively appreciated.

[taion]

@darreng We're discussing this with some code strawmen on #2471. The solution on #2144 will mostly work, but there will be quirks with restoring scroll position on Firefox. That said, v0.13.x had bugs with this too. We have what I think is a better solution on #2471, but it's not clear if we want this to be a blocker for the final release, or if we're okay deferring this to 1.1 or something.

[timdorr]

@darreng Even we end up deferring the scroll changes to 1.1 (which will come quickly after 1.0, I'm sure), the solution @taion proposed is layer on top of the createHistory API and doesn't require internal changes. So, you can pull it into your local code for now and make use of it in 1.0. You could even do that with rc4 right now if you want. It's very optional code. (which is why I said let it wait until after 1.0 😄)

[kjs3]

I'm fine to use the hacky workaround in the short-term but I would consider this incredibly important. In particular scrolling/jumping to url fragments is one of the oldest and most fundamental parts of HTML. At least when using browserHistory.

Here's my slightly modified hack btw. #2144 (comment)

[taion]

We're still hammering it out - try out the useStandardScrollBehavior enhancer from the PR if you're feeling adventurous; I'd love to get feedback on whether it's e.g. usable/works as expected, or if it's still massively glitchy.

[LDMFD]

I've pulled in useStandardScrollBehavior():

var history = require('history/lib/createBrowserHistory');
ReactDOM.render(
    <Router history={useStandardScrollBehavior(history)()}>

(is this correct?)

I found one case in which is doesn't work (in Chrome at least):

1- scroll down a long page
2- click a link - page 2 will be correctly scrolled to the top
3- click Back
4- click the same or any other link without scrolling
5- observe the new page is not scrolled to the top

If after Step 3, a scroll of any distance is performed, the bug does not occur.

(I found this bug when testing nicolashery's #2144 comment solution too).

Any ideas?

Thank you!

[taion]

@darreng Let's consolidate discussion on #2471.

[ryanflorence]

I actually started on scrolling yesterday, but I don't think we need it for 1.0, we can add it in 1.1 or whenever its ready, its easy enough in apps to just scroll to the top in didMount/didUpdate for problem pages until a robust solution is finished.

[LDMFD]

That breaks the UI pretty badly sometimes, e.g. a master-detail pattern with a long list of products each linking to a detail page. If the user goes back from the product detail page to the list, they've lost their position in the page and will be instantly frustrated. That's a 'Class A' for me - the business is an online store.

[timdorr]

@darreng it looks like @taion will build this as an addon library for now. We can refine the code independently of router, which I really like about the implementation so far.

Since work is already started on this, I'm going to close this issue and let attention focus wherever the code ends up (hopefully within @rackt shortly!)

[LDMFD]

👍

[timdorr]

As promised: https://github.com/rackt/scroll-behavior

[taion]

https://github.com/rackt/scroll-behavior/releases/tag/v0.1.0

[LDMFD]

Amazing, thanks guys!

I'll get a test-case together for that bug I found as soon as I've got time (I'm new to Node so it's a bit foggy..)

[taion]

@cachvico Would mind opening this as an issue against the scroll-behavior repo? I see it now; not sure how to fix it and I don't see how this wouldn't have been an issue with v0.13.x.

[LDMFD]

@taion do you have a test-case handy? I didn't put one together yet.

[taion]

Not yet - I can reproduce manually. I just want that issue on scroll-behavior so I can track this.