stencil-community / stencil-router

A simple router for Stencil apps and sites
https://stenciljs.com/
MIT License
188 stars 55 forks source link

scrollTopOffset being applied on state-change with same route #104

Open rayniervandam opened 4 years ago

rayniervandam commented 4 years ago

Stencil version:

@stencil/core@1.5.3
@stencil/router@1.0.1

I'm submitting a ... (check one with "x") [ x ] bug report [ ] feature request [ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or https://stencil-worldwide.slack.com

Current behavior:

When setting a value for scrollTopOffset on stencil-route-switch, the route will apply the offset when simply updating the state of a matched component.

As far as I can tell it is caused by the following line in route.tsx https://github.com/ionic-team/stencil-router/blob/228e1f3888759d651aa436ea3bfe5201e5872803/packages/router/src/components/route/route.tsx#L42

Which says that as long as a route is defined as part of a switch/group, isGrouped will always be true

This in turn makes the following code always break the function:

   if (!newLocation || isGrouped) {
      return;
    }

And therefore the previousMatch variable is never set. This leads to matchesAreEqual always failing, as it will compare against an undefined value, triggering the scrolling to top https://github.com/ionic-team/stencil-router/blob/228e1f3888759d651aa436ea3bfe5201e5872803/packages/router/src/components/route/route.tsx#L76

Expected behavior:

When not changing the active route node, the scrollTopOffset should not be applied and snap back to the top of the window

Steps to reproduce:

You can use the default example and add some @State() variables to the landing page:

<stencil-router>
  <stencil-route-switch scrollTopOffset={1}>
    <stencil-route url="/" component="landing-page" exact={true} />
    <stencil-route url="/demos" component="demos-page" />
    <stencil-route url="/other" component="other-page" />
    <stencil-route component="page-not-found" />
  </stencil-route-switch>
</stencil-router>

When changing the state it will scroll back top without having changed the route

Note: scrollTopOffset is 1, because 0 is falsy and makes the router think the value hasn't been set per the following line: https://github.com/ionic-team/stencil-router/blob/228e1f3888759d651aa436ea3bfe5201e5872803/packages/router/src/components/switch/switch.tsx#L87

Other information:

As a current hack, disabling line 44 to 46 of route.tsx seems to fix the issue, without any visible side effects, but I might be overlooking something

adam-rocska commented 3 years ago

+1

adam-rocska commented 3 years ago

I don't know if the root cause & stuff are legit, but the bug report sure as hell is.

adam-rocska commented 3 years ago

@rayniervandam : Thanks for the hack. It works for my case as well.