-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate active property to tail #125
Propagate active property to tail #125
Conversation
This is working for me when merged with PolymerElements/app-route:master and it resolves the problem with tail routes I had encountered. |
👍 Any chance of getting this merged soon? 🙏 I simply cannot get routes and subroutes to work properly with observers in my project. |
Given the amount of issues generated each week regarding this issue, will this pull request land any time soon @rictic? |
Opinions by @rictic? :) |
@TimvdLippe In general I think we like this change. I have tried to use this branch in an app, and noticed at least one condition where |
@TimvdLippe Please consider the following repro case based on this PR: http://output.jsbin.com/paxake |
@TimvdLippe After testing this out a little, I think I may disagree with the following design axiom:
I don't think that a child route should have the ability to activate itself by poking its own data. Could you describe a use case that justifies such behavior? |
@cdata Really glad there is finally a positive response from the team! I will be taking a look at the repro soonTM (when I have time this weekend).
An example usecase is this store page: https://github.com/TimvdLippe/polymer-lazy-application/blob/master/app/store/store-page.html In this page, we try to update a route using databinding. Whenever you select a category in the
Without this change, the above workflow does not work. However, an existing workflow does work (and therefore makes it very inconsistent):
Note the only difference is the trailing slash after |
I guess the use case sounds fine, but nothing about it seems to require that an inactive route can activate itself via changes to its |
@cdata Well, whenever the databindings change the route needs to be somehow updated. However, if you do not check for the active property it could suddenly produce incorrect active states on its own. #77 contains a different solution by considering both This pullrequest solves the same problem as #77, but in a different manner and in my eyes a lot more intuitive. I have tried to come up with a better solution, but was not able to in all the months since #77 was originally opened. Given the amount of users validating the behavior and the problems it solves, I personally don't think the current design is any magic or counter intuitive. |
It's not about magic, but I do think intuitiveness is important. If a route is not active, then the UI that is bound to the |
Just to remind us where we are in the discussion: I think we agree that the Separately, I think we disagree with the design detail that a child route should be able to self-activate via manipulation of its own |
@cdata, @rictic, I have a use case that is pretty simple. The documentation for app-route, as well as several write ups, leads developers to believe we can use app-route to delegate out routing of certain parts of the app to their respective owners. An example would be this: in app entry point:
in
This should produce the following available urls: /dash When on the /trxns page, selecting a trxn should set the url to something like /trxns/1. It should be able to activate itself. Right now this does not work (it doesn't change the url) and is misleading, especially when |
20d11e5
to
ec55999
Compare
@cdata The issue with reactivating should now be fixed. I also reverted the change with activating the route based on data changes as per our Slack discussion. We discussed the usecase I described and also was pointed out by @runia1 . This usecase will be fixed in a separate PR to not prolonge this PR anymore 😄 |
I am the guy from #158 ... please have a look at my use case if you have any doubts about the usefulness of this PR! |
Can we - please - have this PR (submitted on 5th of July) merged into master ? |
Just having a real struggle without this PR. Any news about the merge guys? |
@@ -236,9 +237,8 @@ | |||
|
|||
__resetProperties: function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps rename to __deactivate
?
If we do end up going to this PR, perhaps we should provide a public resetProperties
or clearProperties
helper function that clears data
and tail
. This function not called by __deactivate
, so the user can choose the functionality. By changing the path and tail, that should eventually do the same as this other proposed solution.
Thoughts @cdata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e111077 In previous discussions with @rictic we concluded that resetting tail
would significantly impact performance of children. Since you reset the tail
data, this means that all children will recalculate based on the data. In big applications this results in a waterfall of databinding updates, sometimes resulting in very long frames. This is the exact reason that right now tail
is not altered when resetting a route.
I am okay for adding a property to enable/disable this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, Tim, spoke with Chris on the issue in person about this. I thought that it was a specific use case he was gunning for, but I was mistaken. There is no need to implement that part of my suggestion.
Although, I still believe that __resetPeoperties
should be renamed to __deactivate
because that seems to be it's main purpose now rather than resetting data
, tail
, and __matched
as it did in it's first implementation.
Thoughts?
Please note everyone that if you want to, you can use my version for the time being. To do so execute
|
ec55999
to
23b8a9a
Compare
As this PR was from my fork prior to renaming from |
This pull request passes the same tests as added in #77, but with almost no breaking changes to existing tests. The only breaking change is actually intentional. Whenever wants to update its data, it checks if the parent is active. If so, it can update the data and become active.
Fixes #110
Closes #77