Closed leolabs closed 6 years ago
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed, please reply here (e.g. I signed it!
) and we'll verify. Thanks.
I signed the CLA!
CLAs look good, thanks!
I think you should change your tabsize to 2 spaces instead of 4. Also, the tests are failing
Tests are obviously the issue right now (adapting them is not), especially since it seems like the expected behavior of the element is to remain how it is if it isn't active. Maybe someone from the polymer elements team can shine some more light on this.
I checked this approach in my setup with three levels of navigation (/:modelName/:id/:subsection
) and three corresponding app-route
connected with tail="subroute"
approach. It didn't work because the moment when app-route
becomes inactive is different than you expected. Imagine you have such transitions:
The app-route
for /my-model/1/my-subsection
(last in the chain) becomes inactive in the transition 3, but not 1 as you expect. And right afterwards becomes active again. So if you rely in the template on its routeData
, it will be updated only in transition 3 when it is not needed anymore, because it gets new params (:id
and :subsection
) right away as same path is activated :(
This was a feature intentionally reverted while in beta because of computational cost. It would propagate down the entire routing subtree upon each change.
Seeing as there has been repeated requests for this feature, I'd be willing to merge a non-breaking change that hides this feature behind a flag (name I have made up below is crude and I welcome suggestions), so usage would be
<app-route clear-data-on-reset route="{{route}}" data="{{firstDataClearsOnReset}}" tail="{{tail}}">
</app-route>
<app-route clear-data-on-reset route="{{tail}}" data="{{secondDataClearsOnReset}}">
</app-route>
Closing for now since this has gone stale. Reopen with changes if you wish to go this route
I would love this feature, if it has to be a flag, that sounds fine to me!
Should fix #206