feat(select): Implement compareWith so custom comparators can be used. #4540
Conversation
googlebot
added
the
cla: yes
ppham27
force-pushed
the
compareWith
branch
3 times, most recently
from
b576c36
to
854954a
Compare
|
@ppham27 Hey would you mind rebasing this? |
|
Rebased. Screenshot tests are failing because of the elevation change in #5361. |
ppham27
force-pushed
the
compareWith
branch
3 times, most recently
from
7fbf9cf
to
ac4278e
Compare
|
@kara Is this still being considered? |
src/demo-app/select/select-demo.html
Outdated
| @@ -95,5 +95,32 @@ | |||
| </md-card> | |||
| </div> | |||
|
|
|||
| <div *ngIf="showSelect"> | |||
There was a problem hiding this comment.
Let's leave this select out of an ngIf. We only really need one select inside an ngIf to test that they work together, and it makes it more difficult to manually test.
src/lib/select/select.ts
Outdated
| get compareWith() { return this._compareWith; } | ||
| set compareWith(fn: (o1: any, o2: any) => boolean) { | ||
| if (typeof fn !== 'function') { | ||
| throw new TypeError( |
There was a problem hiding this comment.
Can you move this error into select-errors.ts?
There was a problem hiding this comment.
Done. Updated tests, too.
src/lib/select/select.spec.ts
Outdated
| compareByReference(f1: any, f2: any) { return f1 === f2; } | ||
|
|
||
| setFoodByCopy(newValue: {value: string, viewValue: string}) { | ||
| this.selectedFood = Object.assign({}, newValue); |
There was a problem hiding this comment.
Can you use our object.extend utility here from core/utils? Object.assign won't pass our internal tests.
There was a problem hiding this comment.
Done. Is the spread operator ({...newValue}) not used?
There was a problem hiding this comment.
The spread operator isn't supported in many browsers. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
There was a problem hiding this comment.
Spread operator should be fine for the demo.
There was a problem hiding this comment.
TypeScript will transpile the spread operator into something that can run everywhere.
src/lib/select/select.spec.ts
Outdated
| fixture.detectChanges(); | ||
| })); | ||
|
|
||
| it('should have a selection', () => { |
There was a problem hiding this comment.
Nit: Can you wrap these two tests in a describe that says 'when comparing by value`, so it's clear what comparator is being used?
src/lib/select/select.spec.ts
Outdated
| expect(instance.select.selected).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should not update the selection when changing the value', async(() => { |
There was a problem hiding this comment.
Nit: I think this test name could be misleading. Selects comparing by reference should still update the selection, generally speaking. It's only when you manually copy over every ngModelChange that you see this behavior. Maybe: "should not update the selection if value is copied on change" ?
src/demo-app/select/select-demo.html
Outdated
| <md-card-content> | ||
| <md-select placeholder="Drink" [color]="drinksTheme" | ||
| [ngModel]="currentDrinkObject" | ||
| (ngModelChange)="setDrinkObjectByCopy($event)" |
There was a problem hiding this comment.
Hmm, this might be a bit odd to use for manual testing because when comparing by reference, it looks broken. I think it might be clearer if you use a regular two-way binding on ngModel, then have a button that says "REASSIGN DRINKS" or "COPY DRINKS" to test whether the value clears.
There was a problem hiding this comment.
Its appearing broken was done on purpose. The intent was to convey when it's necessary to use [compareWith]. I could add a short caption saying this. I am perhaps using the demo app incorrectly, though. Will await further comment before making a change.
There was a problem hiding this comment.
I get the intent, but for other team members manually testing the select, I can see this being confusing. I'd prefer the button.
There was a problem hiding this comment.
Okay, that makes sense to me. I added a REASSIGN DRINK BY COPY button and used a two-way binding. Let me know if this is what you had in mind.
Fixes Issue angular#2250 and Issue angular#2785. Users can supply a custom comparator that tests for equality. The comparator can be changed dynamically at which point the selection may change. If the comparator throws an exception, it will log a warning in developer mode but will be swallowed in production mode.
A two-way binding is used, and there's now a `REASSIGN DRINK BY COPY` button that will clear the display value when comparing by reference.
kara
added
pr: lgtm
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. |
Fixes Issue #2250 and Issue #2785. Users can supply a custom comparator that tests for equality. The comparator can be changed dynamically at which point the selection may change. If the comparator throws an exception, it will log a warning in developer mode but will be swallowed in production mode.