refactor(select): simplify option sync and more robust unit tests #7807
Conversation
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
select-control-provider
branch
2 times, most recently
from
7bd3930
to
b8ce743
Compare
crisbeto
added
pr: needs review
and removed
in progress
src/lib/core/option/option.ts
Outdated
| /** | ||
| * Injection token used to provide the parent control to options. | ||
| */ | ||
| export const MAT_OPTION_PARENT_CONTROL = |
There was a problem hiding this comment.
Hm, the name makes it sounds like we're getting a FormControl instance (and it's form related), when it's really the parent control component. Can we rename to MatOptionParentComponent?
src/lib/core/option/option.ts
Outdated
| @@ -34,6 +36,22 @@ export class MatOptionSelectionChange { | |||
| } | |||
|
|
|||
| /** | |||
| * Describes a parent form control that manages a list options. | |||
There was a problem hiding this comment.
Typo: A list options -> A list of options
| @@ -54,20 +54,6 @@ describe('MatOption component', () => { | |||
| .toBe(0, 'Expected no ripples to show up after click on a disabled option.'); | |||
| }); | |||
|
|
|||
| it('should not show ripples if the ripples are disabled using disableRipple', () => { | |||
There was a problem hiding this comment.
Can you move this test to the select tests rather than removing? e.g. select.disableRipple = true.
Seems like a good one to have, given that select only tests the presence of the property.
crisbeto
force-pushed
the
select-control-provider
branch
from
b8ce743
to
c0c9eb2
Compare
* Currently the select sets the `multiple` and `disableRipple` values on its options manually. In order to avoid "changed after checked" errors we wrap the calls in promises, however this also forces us to have `async` tests which can time out if the browser is out of focus. These changes add a provider that allows for the options to take the value directly from the select. * Refactors some unit tests that have been timing out in Firefox to run in the `fakeAsync` zone instead of the `async` one.
crisbeto
force-pushed
the
select-control-provider
branch
from
c0c9eb2
to
e77785d
Compare
|
I've addressed the feedback @kara, can you take another look? |
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. |
multipleanddisableRipplevalues on its options manually. In order to avoid "changed after checked" errors we wrap the calls in promises, however this also forces us to haveasynctests which can time out if the browser is out of focus. These changes add a provider that allows for the options to take the value directly from the select.fakeAsynczone instead of theasyncone.