Fixed issue where scrolling was preserved when navigating

[trulyronak]

If you were all the way scrolled down on the documentation page and clicked on a request, you'd nothing because you’re scrolled down (from before) - I made it scroll you back to the top or something after you click on a route (well really whenever you navigate to a page)

Added Scroll to Top Component to fix this, following stack overflow.

[JaapRood]

Great issue identified @trulyronak, thanks! Your fix is a bit too impactful, though, to merge.

The react-router documentation you link to as part explains how to scroll back to to the top. It also, though, explains why they're not making that the generic behaviour for every app that uses react router, while they very easily could.

The problem, here, is a UX one: how we need the scroll state to behave when navigating is highly contextual. When navigating to a new page, going to the top might be fine, but what if we're navigating backwards using the browser navigation? Shouldn't we go back to the point where we scrolled down? What if the address we're navigating back to had an "anchor" tag as part of it, do we go back to the anchor?

None of these questions are easy to answer, evidenced by the various approach browsers take. Even they can't agree!

Then there's app or feature specific answers to these questions, too. In the Testing Dashboard page, where we list on the left side a list of captures, upon navigating we automatically scroll to the one selected and displayed on the right. That's very important to make sure the user can see which report out of the list they have navigated to, and establish context for how to react to that testing report.

That's not to say that scrolling to the top can't be useful. Instead of applying it app-wide, we'll want each route to be in control of what happens. Scrolling to the top when navigating forwards can even be the default, but a rendered must have a reasonable way of disabling it and implementing it's own logic.

What I would try as a first place to try and implement this is the Page component. Every route (should) use this to render it's "page". A prop passed to it could give us interface point we want.

[trulyronak]

That makes a lot of sense - I'll take a look at implementing it via the Page component

[trulyronak]

@JaapRood I think I addressed your concerns in the latest change - I placed the ScrollToPosition component in the Page component. While it defaults to (0,0), you can override the x or y accordingly.

One thing I think that might be useful (I can look into adding this) is maybe making it auto scroll to a specific component. Unsure how I'd go about finding a specific component on the page, but I think it should be doable

[JaapRood]

Thanks for making the changes Ronak, you're taking this in the right direction! With ScrollToPosition now rendered as part of the Page, only routes that render a page will automatically scroll forward.

However, what's missing is a route using Page being able to opt-out of (or opt-in to) this behaviour. I'd expect that to be possible through something like <Page title="Test page" scrollToTop={false}></Page>. The TestingDashboard component might be a good place to test that, as it implements it's own scrolling logic.

Lets for now not worry about making the API so we can scroll to any coordinate: that complicates things significantly both technically and from a usability point of view. A Page automatically scrolling to the top when first rendered seems plenty of an improvement already!

Finally, it might be worth asking ourselves whether this logic needs its own component ScrollToTop. Is it really a piece of code we want to be executing elsewhere, with all the complications of supporting all the additional use cases and contexts that come with this? If so, does it have to be a component? Or would making it right part of the Page component make things a lot simpler?

Don't hesitate to ask for help with any of that 🙂.

[trulyronak]

it might be worth asking ourselves whether this logic needs its own component ScrollToTop

Honestly, when looking at the current Page component (and seeing how it handles the title logic within the component already), I think it's worth just adding the scroll code in the component, as I can't think of another place where I'd use it. There's an argument to be made about abstracting all logic out into its own component, but I think in this case it just makes most sense to keep the behavior all inside the Page component — I went ahead and made that change, but I'm not 100%. on how I did it.

[JaapRood]

Great job, almost there! 🙂

• if scrollToTop comes from the props object, it can be used like <Page scrollToTop={true}></Page> in other components. There's an excellent guide by the ReactJS team about props and how to use them, if you'd like to look that up.
• the scrollToTop must be used inside the useEffect to trigger the window scrolling conditionally. The second parameter [pathname && scrollToTop] is monitored to see whether the effect must be run. Any change in that value causes the effect to be ran again. For more details, see this guide to how useEffect works.

[trulyronak]

adjusted it! since the default behavior is not to scroll to the top, I also specified for DocsPage to pass in scrollToTop

[JaapRood]

Honestly, when looking at the current Page component (and seeing how it handles the title logic within the component already), I think it's worth just adding the scroll code in the component, as I can't think of another place where I'd use it.

I totally agree! 👍 Even if something can be it's own function or component, doesn't mean you should. As soon as something is it's own function that can be called from elsewhere, other code will eventually use it. 9 out of 10 times that code has slightly different requirements, making it hard to make changes to it in the future. Unless some code is particularly error-prone, complicated or you have a really good understanding of what it should and shouldn't do, inlining is the more pragmatic and durable choice.

[JaapRood]

Nice work! Thanks for putting all that effort in and sticking with it 🎉👍!