feat(select): integrate with Angular forms #1655
Conversation
googlebot
added
the
cla: yes
kara
force-pushed
the
select-forms
branch
7 times, most recently
from
053408a
to
d6d5ca9
Compare
kara
force-pushed
the
select-forms
branch
2 times, most recently
from
9c72f58
to
1c52a54
Compare
| @@ -46,9 +50,9 @@ export class MdOption { | |||
| } | |||
|
|
|||
| /** Selects the option. */ | |||
| select(): void { | |||
| select(isUserInput = false): void { | |||
There was a problem hiding this comment.
This is kind of weird, since it's a public method. How about adding a separate _selectViaInteration method that calls select() and does the emit?
There was a problem hiding this comment.
That makes way more sense. Good call.
| @@ -25,6 +25,10 @@ md-select { | |||
| [dir='rtl'] & { | |||
| transform-origin: right top; | |||
| } | |||
|
|
|||
| [aria-required=true] &::after { | |||
| content: '*'; | |||
There was a problem hiding this comment.
Add a TODO here to double-check the accessibility of this.
|
|
||
| it('should set the view value from the form', () => { | ||
| let value = fixture.debugElement.query(By.css('.md-select-value')); | ||
| expect(value).toBeNull(); |
There was a problem hiding this comment.
FYI, don't forget that you can add assertion descriptions to the matchers so that the failures are more clear.
(optional)
There was a problem hiding this comment.
Ack, I always forget about that
| writeValue(value: any): void { | ||
| if (!this.options) { return; } | ||
|
|
||
| this.options.forEach((option: MdOption) => { |
There was a problem hiding this comment.
If you do this with a for of, you could break out as soon as you find the corresponding option.
There was a problem hiding this comment.
this.options is a QueryList, so you'd have to use toArray() first.
kara
force-pushed
the
select-forms
branch
2 times, most recently
from
95bdea5
to
94c1a38
Compare
|
@jelbourn Comments addressed |
| fixture.detectChanges(); | ||
|
|
||
| value = fixture.debugElement.query(By.css('.md-select-value')); | ||
| expect(value.nativeElement.textContent).toContain('Pizza', |
There was a problem hiding this comment.
Prefer breaking at the higher syntactic level, e.g.:
expect(value.nativeElement.textContent)
.toContain('Pizza', `Expected trigger to be populated by the control's new value.`);(general rule-of-thumb that makes code more readable in all languages)
There was a problem hiding this comment.
Yeah, that does look better. I'll reformat.
|
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. |
This PR adds basic forms integration.
Still TODO for forms:
r: @jelbourn