-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(autocomplete): add value support #2516
Conversation
902d72b
to
c6112aa
Compare
c541077
to
30e83a9
Compare
30e83a9
to
24c94ec
Compare
b8cb2df
to
f431e49
Compare
</md-card> | ||
|
||
<md-card> | ||
<div>TD value (currentState): {{ currentState }}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed to "template-driven" for clarity
*/ | ||
private _subscribeToClosingActions(): void { | ||
this.autocomplete.options.changes | ||
.startWith(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment explaining what this does? I don't know what startWith(null)
is for or what switchMap
does
*/ | ||
private _setValueAndClose(event: MdOptionSelectEvent | null): void { | ||
if (event) { | ||
// TODO(kara): revisit animation once floating placeholder is toggle-able |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
probably isn't necessary any more
expect(fixture.componentInstance.stateCtrl.dirty) | ||
.toBe(false, `Expected control to stay pristine if value is set programmatically.`); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be any tests for touched state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be tested by the forms module and/or mdInput. The autocomplete doesn't actually touch those events, so seems duplicative to test here.
78a5be9
to
e21a8e1
Compare
@jelbourn Comments addressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
e21a8e1
to
e4c8a67
Compare
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. |
Autocomplete is WIP. This is a small piece to add value population.
Note: The placeholder animation will still happen when setting the value. This will be fixed in a later PR once floating placeholders are more configurable in input.