Add selectedPropertyValue to dynamically set property #148
Conversation
|
Ping to @bicknellr. |
| this._selectionChange(); | ||
| this.fire('iron-' + (isSelected ? 'select' : 'deselect'), {item: item}); | ||
| }, | ||
|
|
||
| _updatePropertyValue: function(selectedItem, selectedAttribute, propertyValue) { | ||
| if (selectedAttribute && propertyValue) { | ||
| selectedItem[selectedAttribute] = propertyValue; |
There was a problem hiding this comment.
This crashes on first load as selectedItem is undefined.
Works with if (selectedItem && ...) on the line above.
There was a problem hiding this comment.
I believe another issue will raise its head here..
Assuming we are using it for the purpose of fixing the subroute issue then we don't want the selectedPropertyValue to be set on pages unless they are the expected page for the route.
Assuming subroute is bound to selectedPropertyValue..
The problem happens because when the route is changed (and subroute triggers the selectedPropertyValue change) the selectedItem hasn't been updated to the new page yet.. so the _updatePropertyValue will be triggered with the new selectedPropertyValue (subroute) but still with the old page still being the selectedItem!
So the new value is set on the old page, which isn't desired.
Note: the new value will also be set correctly on the new page when the selectedItem is changed.
Instead we could just toss out the _updatePropertyValue observer entirely. This fixes the issue I have raised above, and the value will be set correctly in _applySelection anyway, though this also means any changes to selectedPropertyValue require a page change before they will be reflected down to the pages accordingly - though for my purposes this isn't really a problem.
There was a problem hiding this comment.
That would break subviews though wouldn't it? We would have app-location intercepting the new route which would be passed down through subroute, but then the view never gets notified of a page change because the view's route didn't get updated.
There was a problem hiding this comment.
Not sure if I understand.. the view's selectedPropertyValue would be updated on _applySelection.
There was a problem hiding this comment.
Yes but _applySelection would only be called on change of the selected item, if you have an app-route inside your view binding to the route property and then handling other views, how would it get updates for when the page changes?
There was a problem hiding this comment.
Ah ya.. yup you're correct, it would break that :(
| @@ -336,10 +346,19 @@ | |||
| if (this.selectedAttribute) { | |||
| this.toggleAttribute(this.selectedAttribute, isSelected, item); | |||
| } | |||
| if (this.selectedPropertyValue) { | |||
| item[this.selectedAttribute] = this.selectedPropertyValue; | |||
| } | |||
There was a problem hiding this comment.
Seems like the assignment of the value should be contingent on if selectedAttribute exists, and also probably needs to check if isSelected and if !isSelected then set to null (or something?).. eg:
if (this.selectedAttribute) {
if (this.selectedPropertyValue) {
item[this.selectedAttribute] = isSelected ? this.selectedPropertyValue : null;
}
else {
this.toggleAttribute(this.selectedAttribute, isSelected, item);
}
}
|
@TimvdLippe Now that the Polymer Summit is over, would it be possible to get this some attention sometime soon? |
|
I would like to get @bicknellr's opinion first, before fixing the last bugs. Thanks for testing this PR out! |
|
This sounds useful! 👍 Thanks @TimvdLippe! |
|
This PR is very old and I am no longer pursuing this change. Therefore I am closing this PR. |
Please see PolymerElements/app-route#164 for the motivation of this PR. In general, it is not possible to dynamically change the value of
selected-attributeand alike. Therefore this PR introduces a new property that does so. Since attribute values can not cope with objects (which was the original usecase) I decided to dircectly set it as property instead.Fixes PolymerElements/app-route#164