fix(select): deselect old options when programmatically setting value #4658
Conversation
googlebot
added
the
cla: yes
|
Off the top of my head, I can't think of anything that it might break, but let me play around with it a bit more tomorrow. |
crisbeto
left a comment
crisbeto
left a comment
There was a problem hiding this comment.
Logic seems fine, just a couple of nitpicks.
| throw getMdSelectNonArrayValueError(); | ||
| } | ||
|
|
||
| this._clearSelection(); |
There was a problem hiding this comment.
nit: Can you add a new line after this one?
src/lib/select/select.spec.ts
Outdated
|
|
||
| options = | ||
| overlayContainerElement.querySelectorAll('md-option') as NodeListOf<HTMLElement>; | ||
| expect(options[0].classList).not.toContain('mat-selected'); |
There was a problem hiding this comment.
Could you add a description to the expectations in the test? It makes it easier to parse if one of them fails. E.g. expect(options[0].classList).not.toContain('mat-selected', 'Expected first option not to be selected');.
|
@crisbeto changes have been made |
|
Seems like there are a couple of linter errors. The Closure failure should be fixed by #4709. |
crisbeto
added
pr: lgtm
blocked
|
@willshowell the two PRs got merged. Can you rebase from master? |
willshowell
force-pushed
the
fix/select-option-styling
branch
from
2cadd47 to
885db05
Compare
crisbeto
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. |
4 participants
Fixes #4476
This may be naive regarding selection change events, but existing tests pass and it fixes the issue.
cc @crisbeto