Several enhancements for StandardScroll

[jondkoon]

• Adds ability to ignore scroll behavior by passing ignoreScroll with
  location state
• Adds ability to restore scroll by passing restoreScroll with
  location state
• When routing to the current location scroll to top
• RAF around scrollTo that was necessary for restoreScroll functionality

[jondkoon]

Will add tests and documentation as well, but wanted to start a conversation about the changes first

[taion]

For ignoreScroll, couldn't that be handled in user space with shouldUpdateScroll? i.e. just check nextLocation.state.ignoreScroll or something.

[jondkoon]

It could. Seems like a feature that would be nice to have out of the box though.

[jondkoon]

I thought a visual might help explain the necessity of the enhancements.

In the screenshot you'll notice a top bar that allows you to switch between your home timeline and notifications. I have added restoreScroll to the links so that when switching between these timelines your scroll position is restored. Additionally when you are on the home timeline and you want to scroll to the top you can tap the home icon.

You'll also notice a "SegmentedControl" on the profile that allows you to switch between Tweets, Media, and Likes. For these links I have added ignoreScroll because I don't wan't the scroll position to change since the content is only changing for the bottom half of the screen and a scroll to the top would be jarring.

[taion]

I like where you're going with this.

• For restoreScroll, can you instead just scrollPosition on the state? It's not an officially documented API hook, but it seems like it'd be easier to just re-use the existing codepath, i.e. overwrite the remembered scroll position. If that doesn't work, we should make it work.
• I think it would be better to handle ignoreScroll via shouldUpdateScroll as already implemented; your use case makes sense, but I don't think we need to have hard-coded support here, when it looks pretty straightforward to implement in userspace.
• Can you explain what you wanted to do with positionFromPath? Is this a hook for working with hash history without a query key?
• Lastly, I'd been thinking about the RAF stuff. Maybe we should just move this into a separate utility that's common to all the scroll behaviors.

Thanks again for the PR!

[jondkoon]

Thanks for the review @taion

• For restoreScroll you need positionFromPath because it needs to handle the case where you have pushStated to a path you have already visited. In this case the state will not have a scrollPosition entry. This is actually the main case it needs to handle as StandardScroll already handles popstate pretty well.
• I removed ignoredScroll from the patch and moved it into user land in my code base
• I created a scrollTo utility that uses RAF. This also should remove the need for a setTimeout in useScrollToTop. I did not do any manual testing of this, but it should work.

Let me know when the changes are good and I'll work on the tests and documentation.

[taion]

I see what you mean. That makes sense, thanks. Give me a moment to think about this. I'll get back to you shortly.

[taion]

Do you think it would make sense if shouldUpdateScroll could return an [x, y] array? In which case we then scroll to that position, which would also let you build out the functionality for preserving scroll position by path?

You don't need to amend your PR – I just had a separate idea for how to update scroll position that might address some long-standing issues like #15, but I need to experiment with it.

How urgently do you need a release? It might take me a day or two.

[jondkoon]

@taion I actually had the same thought about shouldUpdateScroll. Not only would it afford implementing scroll preservation in user space, but scroll to top when visiting the same route.

I need a solution in the next day or so, but don't rush because of me. I can use my fork to unblock myself.

I'm also interested in authoring the patch if you want to share your plans for #15

[taion]

Would you be willing to amend this PR to just update shouldUpdateScroll to allow returning that 2-tuple, then, and exclude the RAF-related bit?

I don't want to waste your time with my hairbrained ideas for #15. The idea – and I have no clue if it works – is to actual listen for the scroll event caused by our attempt to restore the scroll position (is there one?), and keep trying to scroll to the right position every frame until that scroll event fires, except if I get the same scroll position back two frames in a row, in which case I give up (because maybe I'm at the end of the page or something and I can't get to where I want to be, because the page is shorter now).

[jondkoon]

@taion I'll take a shot at that. I might need the RAF stuff for restore scroll (not really sure why honestly). Do you expect wrapping window.scrollTo in RAF will cause problems?

[taion]

I'm saying that I think what I have is more reliable than RAF. I'll make some time to look at it later today.

For future API consideration, are you willing to share whether you're using this with React Router, or whether you're using this on its own?

[jondkoon]

Something more reliable sounds like a win.

We are using this with React Router. I was originally going to fork this internally, but then realized that there will likely be some churn on this module and react router integration, so I wanted to make sure the features we need for our app will carry on.

I'm working on the patch in a new branch now. It's code complete just need to write the tests. Should have it for you shortly.

[jondkoon]

Closing in favor of #53

[jondkoon]

So this is what I ended up with implementing these features in user land. It can be further simplified once the RAF alternative lands: https://gist.github.com/jondkoon/e81c956674655d3cae625e985f749d81

I don't like that I have to use readState in order to get the scrollPosition. Have any ideas for alternatives?

[taion]

Are the data not all on location.state?

[jondkoon]

Doesn't seem to be. I was surprised by that as well. Double checking...

[jondkoon]

It is strangely inconsistently available

[taion]

What sorts of transitions were these? Push or pop?

[jondkoon]

Fresh page load -> push -> push

[taion]

Ah, sorry! I see now. Yes, for the previous state, to avoid weird stuff with mutating the location object, we don't update location.state in-place.

You'll want to always use readState for the previous state (if you want to see the scroll position), and always use location.state for the new state.

[jondkoon]

Gotcha. I think I can probably get away with just looking at new state for my case. Thanks!

[taion]

Implemented the more robust scroll handling on #56.