-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix(autocomplete): close panel when options list is empty #2834
Conversation
d1a0f7a
to
bd8dbdb
Compare
bd8dbdb
to
a0bf7cc
Compare
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
@@ -131,7 +134,7 @@ export class MdAutocompleteTrigger implements AfterContentInit, ControlValueAcce | |||
* A stream of actions that should close the autocomplete panel, including | |||
* when an option is selected and when the backdrop is clicked. | |||
*/ | |||
get panelClosingActions(): Observable<any> { | |||
get panelClosingActions(): Observable<MdOptionSelectEvent | 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.
What's your reasoning for doing | null
here since we haven't been doing explicit null yet?
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.
Just thought we should start so that it's more explicit. It's not super important to change in this PR, so I can revert for now.
@@ -149,6 +152,11 @@ export class MdAutocompleteTrigger implements AfterContentInit, ControlValueAcce | |||
return this._keyManager.activeItem as MdOption; | |||
} | |||
|
|||
/** The initial list of autocomplete options, as soon as the zone has stabilized */ | |||
get initialOptionList(): Observable<QueryList<MdOption>> { |
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.
I don't think this is really the initial list of options- it's just whatever the next list of options are once the zone stabilizes; the fact that you call it on initialization is separate from what you're getting. So you could change this to _getStableOptions
and then in the subscribe function do
let initialOptions = this._getStableOptions();`
Also should be private or internal.
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's a good point. Will do.
@@ -149,6 +152,11 @@ export class MdAutocompleteTrigger implements AfterContentInit, ControlValueAcce | |||
return this._keyManager.activeItem as MdOption; | |||
} | |||
|
|||
/** The initial list of autocomplete options, as soon as the zone has stabilized */ | |||
get initialOptionList(): Observable<QueryList<MdOption>> { | |||
return this._zone.onStable.first().map(() => this.autocomplete.options); |
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 why you need to do this
fixture.detectChanges(); | ||
|
||
fixture.whenStable().then(() => { | ||
// Filter down the option list so no options match the value |
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.
"so" => "such that"
a0bf7cc
to
0914f29
Compare
@jelbourn Comments addressed. |
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. |
Ready for review.
r: @jelbourn @andrewseguin