-
Notifications
You must be signed in to change notification settings - Fork 107
fixes #81, #104 #105
base: master
Are you sure you want to change the base?
fixes #81, #104 #105
Conversation
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). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
CLAs look good, thanks! |
} else { | ||
value = selectedItem.label || selectedItem.textContent.trim(); | ||
label = selectedItem.label || selectedItem.textContent.trim(); | ||
value = selectedItem[this.attrForSelected] || label; |
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.
@morficus
Would you consider the following: value = selectedItem[this.contentElement.attrForSelected] || label;
?
The reason being that attr-for-selected
stays on the paper-menu/paper-listbox
contained in the content section. This is useful since paper-dropdown-menu
doesn't have any way of setting the selected item. Selection must be done on the paper-listbox
, which means that one may have to duplicate the attr-for-selected
on the paper-listbox
.
It would enable the following:
eg.)
let _bar = 0;
....
<paper-dropdown-menu>
<paper-listbox selected="{{_bar}}" attr-for-selected="foo" class="dropdown-content">
<paper-item foo="0">Zero</paper-item>
<paper-item foo="10">Ten</paper-item>
<paper-item foo="20">Twenty</paper-item>
</paper-listbox>
</paper-dropdown-menu>
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.
This is useful since
paper-dropdown-menu
doesn't have any way of setting the selected item. Selection must be done on thepaper-listbox
Very good point about the selection staing with in the list-element it self 👍 I'll make the change and update the PR
adding new `attrForSelected` attribute to specify an attribute whose value to use as this elements `value`
@cdata Could you please review? |
I have left some feedback on this design on #104 |
adding new
attrForSelected
attribute to specify an attribute whose value to use as this elementsvalue