Skip to content
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.

Sign up for GitHub

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

Add selectedPropertyValue to dynamically set property #148

Closed
wants to merge 1 commit into from
Closed

Add selectedPropertyValue to dynamically set property #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion iron-selectable.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,15 @@
value: null
},

/**
* The value of the property to set when an item is selected. The name
* of the property is this.selectedAttribute.
*/
selectedPropertyValue: {
type: Object,
value: null
},

/**
* Default fallback if the selection based on selected with `attrForSelected`
* is not found.
Expand Down Expand Up @@ -154,7 +163,8 @@
observers: [
'_updateAttrForSelected(attrForSelected)',
'_updateSelected(selected)',
'_checkFallback(fallbackSelection)'
'_checkFallback(fallbackSelection)',
'_updatePropertyValue(selectedItem, selectedAttribute, selectedPropertyValue)'
],

created: function() {
Expand Down Expand Up @@ -336,10 +346,19 @@
if (this.selectedAttribute) {
this.toggleAttribute(this.selectedAttribute, isSelected, item);
}
if (this.selectedPropertyValue) {
item[this.selectedAttribute] = this.selectedPropertyValue;
}
Copy link

Choose a reason for hiding this comment

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

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);
    }
}

this._selectionChange();
this.fire('iron-' + (isSelected ? 'select' : 'deselect'), {item: item});
},

_updatePropertyValue: function(selectedItem, selectedAttribute, propertyValue) {
if (selectedAttribute && propertyValue) {
selectedItem[selectedAttribute] = propertyValue;
Copy link

Choose a reason for hiding this comment

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

This crashes on first load as selectedItem is undefined.
Works with if (selectedItem && ...) on the line above.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Not sure if I understand.. the view's selectedPropertyValue would be updated on _applySelection.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

Ah ya.. yup you're correct, it would break that :(

}
},

_selectionChange: function() {
this._setSelectedItem(this._selection.get());
},
Expand Down
17 changes: 17 additions & 0 deletions test/selected-attribute.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,23 @@
assert.isTrue(s.children[4].hasAttribute('myattr'));
});

test('custom selectedAttribute with value', function() {
var value = { foo: 'bar' };
s.selectedPropertyValue = value
// set selectedAttribute
s.selectedAttribute = 'myattr';
// check selected attribute (should not be there)
assert.isFalse(s.children[4].hasAttribute('myattr'));
// set selected
s.selected = 4;
// now selected attribute should be there
assert.isTrue(s.children[4].hasAttribute('myattr'));
assert.deepEqual(s.children[4]['myattr'], value);

s.selectedPropertyValue = 'foo';
assert.deepEqual(s.children[4]['myattr'], 'foo');
});

});

suite('changing attrForSelected', function() {
Expand Down