Closed TimvdLippe closed 8 years ago
After a little extra consideration, it seems like we can't just patch _valueToItem
: IronMultiSelectableBehavior
will map values from selectedValues
without a matching item to the default item. So, the default item can be added (multiple times, potentially) to a selection which already contained valid values.
I think you should test / filter selected
and selectedValues
in _updateSelected
of both, and if selected
/ selectedValues
have no matching items, then select the item matching defaultSelected
(if it exists).
@bicknellr Ah I did not consider the Multi-selector at all, I do not know the codebase of this element that well ;)
Since you self-assigned this PR, does that mean you take it over or shall I update the PR with your feedback? (both options are fine by me, just want to prevent doing double work)
default
may not be the clearest term here, as it connotes that this is the element that will be selected when no deliberate selection was made. One option that may be clearer is fallback
.
The convention is that the assignee for a PR is the person that's gonna review it.
@TimvdLippe no worries, that's what we're here for!
I have updated the code to also work with multiselectable. However, I did also change the workings of defaultSelected
. It will now not work as fallback, but explicitly update the selected value(s).
The initial reason we did not want to update the selected value, was to not force the router to be updated. However, I later realised a developer can more quickly decide what the behavior should by switching between one-way and two-way bindings. Therefore now the default value is actually set and therefore I also think the default-selected
naming is again more valid.
Renamed the property and also properly fix the timing issue. The issue was that on attached
the actual this.items
array is populated. Only then will _valueToIndex
correctly return the index. If the call is made before this.items
exist, it would falsely notify that there was no selection made.
Following from @rictic's comment, what's the general expectation for the behavior when the element is created / ready? Does this property imply that the default item is selected at the start if selectedValues
is empty or only contains invalid values? I think it should, but does anyone want to propose that it shouldn't / have an opposing use case?
@bicknellr Currently there is no selection made until attached
is called. The check if it should update is here. The selected
property is left intact up to this point. Therefore it will not have any default behavior until the element is ready and a selection is made. This does however mean that if the selection ismade and up to that point there was no valid selection, the fallbackSelection will be used.
(recap from slack discussion) Not updating selectedValues
is preferable given that this is the user writable way to set multiple selected values and does not currently happen. (selectedItems
will inherently be filtered.) Last thing here is a check / tests for when fallbackSelected
itself doesn't reference an item. The observer internals handle this for IronSelectableBehavior since it won't fire the next time it's set to fallbackSelected
. Though, for IronMultiSelectableBehavior, there needs to be an explicit check because the fallback behavior sets selectedValues
to a new array.
@bicknellr Up for review again :tada:
I've tried it out and played around with it and it seems really nice! I've got no complaints anymore with anything you've written here but I did notice a bug that's a result of something I didn't catch when reviewing #115 and this new PR kind of brings to light: this line actually shouldn't be there anymore since we're now listening for changes to selectedValues.splices
instead of just selectedValues
. This wasn't really an issue until now, because selectedValues
never changed during that function. If you have multi
and only the default item is selected and then you try to deselect the default item; you're actually able to do so because that line will force the default item to be deselected even though your code - which runs in either the push
or slice
right before - is trying to keep it selected (since nothing would be otherwise). Delete that line and LGTM.
Thanks!
Add support for the
defaultSelected
property. As discussed with @rictic theselected
value is not updated, to prevent the property notifying and possibly update the URL of the website.Added corresponding documentation and testcase to verify correctness of the implementation. Also the
attr-for-selected.html
testsuite was not inindex.html
and was not executed. I fixed that issue too.Fixes #116