fix(select): allow option with undefined or null value to clear selection #3141
Conversation
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
3110/select-clear
branch
from
14913ce
to
d6fe305
Compare
|
Is this change coming on beta3? |
|
@crisbeto Sorry for the late review on this. It looks good, but probably needs some reworking / rebasing since select has had some changes. Can you update? |
crisbeto
force-pushed
the
3110/select-clear
branch
from
d6fe305
to
aa0725d
Compare
|
Sorry about this one @kara, I must've missed the notification. I've rebased it and I had to redo it to match the new setup. Can you take another look? |
src/lib/select/select.spec.ts
Outdated
| @Component({ | ||
| selector: 'reset-values-select', | ||
| template: ` | ||
| <md-select placeholder="Food" [formControl]="control" [required]="isRequired"> |
There was a problem hiding this comment.
Seems like you're not using the required binding in the tests. Remove to keep it simple?
src/lib/select/select.spec.ts
Outdated
| selector: 'reset-values-select', | ||
| template: ` | ||
| <md-select placeholder="Food" [formControl]="control" [required]="isRequired"> | ||
| <md-option *ngFor="let food of foods" [value]="food.value"> |
There was a problem hiding this comment.
We should also test this setup, since it will be common as well (where value binding is not added at all):
<md-option>None</md-option>
src/lib/select/select.spec.ts
Outdated
| isRequired: boolean; | ||
|
|
||
| @ViewChild(MdSelect) select: MdSelect; | ||
| @ViewChildren(MdOption) options: QueryList<MdOption>; |
There was a problem hiding this comment.
It doesn't look like you're using this options ViewChildren query. Remove?
src/demo-app/select/select-demo.html
Outdated
| @@ -6,6 +6,8 @@ | |||
|
|
|||
| <md-select placeholder="Drink" [color]="drinksTheme" [(ngModel)]="currentDrink" [required]="drinksRequired" [disabled]="drinksDisabled" | |||
| [floatPlaceholder]="floatPlaceholder" #drinkControl="ngModel"> | |||
| #drinkControl="ngModel"> | |||
There was a problem hiding this comment.
Looks like a rebase error here. #drinkControl="ngModel"> has been duplicated.
src/lib/select/select.ts
Outdated
| if (option.value == null) { | ||
| this._clearSelection(); | ||
| this._onChange(option.value); | ||
| this.change.emit(new MdSelectChange(this, option.value)); |
There was a problem hiding this comment.
Rather than duplicating the change event logic from propagateChanges, seems like we could tweak propagateChanges to accept an optional value arg. I think it'd help make the conditional more readable.
private _onSelect(option) {
if (this.multiple) {
...
} else if (option.value == null) {
this._clearSelection();
this._propagateChanges(option.value);
} else {
this._clearSelection(option);
this._selectionModel.select(option);
}
...
}
private _propagateChanges(value?: any): void {
let valueToEmit = Array.isArray(this.selected) ? this.selected.map(option => option.value)
: (this.selected ? this.selected.value : value);
...
}
…tion Allows for options, with a value of `null` or `undefined`, to clear the select. This is similar to the way the native select works. Fixes angular#3110. Fixes angular#2634.
crisbeto
force-pushed
the
3110/select-clear
branch
from
aa0725d
to
c6fce37
Compare
|
Addressed the feedback @kara. |
kara
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Allows for options with a value of
nullorundefinedto clear the select. This is similar to the way the native select works.Fixes #3110.
Fixes #2634.