[feat] Always scroll to top on router change

[brendanfalkowski]

Description

On router changes, the page should always scroll to top.

This also removes prior attempts to manage scrolling-to-top per route. This should be a universal behavior.

Related Issue

Closes #2289.

Acceptance

Verification Stakeholders

Specification

Verification Steps

1. Go to any page.
2. Scroll down.
3. Click a link.
4. The page should scroll up to top.

Screenshots / Screen Captures (if appropriate)

Checklist

• I have updated the documentation accordingly, if necessary.
• I have added tests to cover my changes, if necessary.

[brendanfalkowski]

I removed window.scrollTo from the PDP since that is handled by the global routing behavior.

I did not remove window.scrollTo from the category/search areas because some changes like sorting don't invoke the router. These inline calls are still needed.

[PWAStudioBot]

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against a51749e

[jimbo]

@brendanfalkowski Thanks for contributing this, it makes sense to me.

I was concerned that the browser wouldn't be able to preserve scroll position for a history entry if we did this, but after trying your branch locally, it seems to work fine. That is, going forward or backward in browser history seems to return the page to the scroll position, which is good.

I'm inclined to say that the ScrollToTop component should be a custom hook instead, though. Typically a component should return an element, or at least render an element under certain conditions. Here, it's only being used as a vessel for a side effect, a case in which we normally just use a custom hook.

Would this work?

const Routes = () => {
  useScrollToTop()

  return (
    <Suspense fallback={fullPageLoadingIndicator}>
      <Switch />
    </Suspense>
  )
}

[sirugh]

Now that we're migrated to function components this should be a peregrine hook, useScrollTop. It could even be useScrollTopOnChange and take a parameter like pathname which it uses in the effect. Then any component could configure when it should change such as useScrollTopOnChange(user.isSignedIn), etc.

Could also have useScrollTopOnMount that scrolls only on mount of a component.

[sirugh]

I just saw Jimmy said basically the same thing a few days ago 😆

[sirugh]

This is a nice change, and should obviate a lot of repeated code throughout the app. That said, please make this a custom Peregrine hook :)

Naming of the hook is up for debate but I don't think we should hard code the variable (pathname in your component) into the hook as other components may want to use the behavior on some different change. I would actually prefer it be a hook that accepts a variable on which to trigger the event. For example:

const Routes = () => {
  const { pathname } = useLocation();
  useScrollTopOnChange(pathname);

  return (...)
}

As always, we'd be happy to make these changes if you are occupied with more important things :) Let us know!

[sirugh]

Some other examples I saw out on the web included also observing search. I'm not sure if we want to implement that here or in the components that actually care about search such as pagination or the search/filter components.

[supernova-at]

I think it makes sense to do it here. Those components are changing search but scrolling is a page-level concern; I don't think they should know / care / control the scrolling.

[jimbo]

Edit: I wouldn't scroll to top when search params change. Yes, scrolling is a page-level concern, but so is changing the URL—even search.

It's reasonable for a component that changes location, which is global state, to have an opinion on whether the page should scroll. Also, on the web we try to minimize the number of overflowing containers we have, so it ends up being fairly common for a component to scroll the window.

Anyway, the issue I see with scroll-to-top on search change is that we can't really disable that behavior on a case-by-case basis. We'd have to accept that it'll happen in every case where we ever change search...and I don't have that level of confidence. In contrast, I do have that level of confidence when it comes to pathname.

Thoughts?

[sirugh]

We'd have to accept that it'll happen in every case where we ever change search...and I don't have that level of confidence. In contrast, I do have that level of confidence when it comes to pathname.

To be specific, the Router component can confidently control scrolling on path changes but not search changes. That said, other components such as the Category or Search pages should be able to utilize the new hook to handle scrolling to top as they do already.

// CURRENT
const Category = props => {
    const { search } = useLocation();
    const filtersFromSearch = getFilters(search);
    useEffect(() => {
        runFilterQuery()
        window.scrollTop();
    }, [filtersFromSearch])    
}
// PROPOSED
const Category = props => {
    const { search } = useLocation();
    const filtersFromSearch = getFilters(search);
    useScrollTopOnChange(filtersFromSearch);
    useEffect(() => {
        runFilterQuery()
    }, [filtersFromSearch])
}

[supernova-at]

Approved, and I'd also add search.

[supernova-at]

I think it makes sense to do it here. Those components are changing search but scrolling is a page-level concern; I don't think they should know / care / control the scrolling.

[devops-pwa-codebuild]

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2309.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.65

I made changes.