Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

Data should be reset when the pattern no longer matches. #220

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jishi9
Copy link

@jishi9 jishi9 commented Dec 12, 2017

The current behavior is problematic, particularly when combined with <iron-pages>

For example:

<app-route
    route="{{route}}"
    pattern="/:page"
    data="{{routeData}}"
</app-route>

<iron-pages selected="{{routeData.page}}"
            attr-for-selected="page"
            fallback-selection="default">
  <div page="a">PageA</div>
  <div page="b">PageB</div>
  <div page="default">Default page</div>
</iron-pages>

The current behavior:
When visiting http://foo/ we land at the default page.
Switching the URL to http://foo/b we land at Page B
Switching the URL back to http://foo/ (e.g. by clicking the browser's back button) , we still remain on Page B, when the expected behavior is to go back to the default page.

@cdata
Copy link
Contributor

cdata commented Jan 31, 2018

Agreed, the current behavior is problematic. Unfortunately, the current behavior is also working as designed. The stated intention of the designer was to avoid causing deep trees of state to thrash as routes become activated and deactivated. It is a nice idea, but in practice should probably be a secondary quality to "state changes the way I expect..." but I digress.

Given that this element already has had its major release, could you please propose this change in a way that retains backwards compatibility with default behavior? In other words, we can consider this change more seriously if it is "enabled" by an optional boolean attribute on the route.

@cdata
Copy link
Contributor

cdata commented Jan 31, 2018

/cc @e111077 who is the owner of this element

@e111077
Copy link
Contributor

e111077 commented Jan 31, 2018

@cdata has a good grasp of this. This causes a lot thrashing down the tree, and then causes a lot of reported issues that they cannot keep state on their app / flashing of content after page change.

Though, this is also a common feature request that we've seen.

I'll be more-inclined to hide this behind a flag e.g. clear-data-on-inactive. That defaults to false / is opt-in.

Also want to cc @rictic initial designer of this element as he was the one to initially make this decision.

Copy link
Contributor

@cdata cdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @e111077 on this - let's implement this change behind a flag. Please update the PR add an attribute flag to toggle this behavior!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants