feat(autocomplete): add autocomplete panel toggling #2452
Conversation
googlebot
added
the
cla: yes
kara
force-pushed
the
autocomplete-pr
branch
4 times, most recently
from
127d3a5
to
7e5ad05
Compare
kara
force-pushed
the
autocomplete-pr
branch
2 times, most recently
from
5ca9cec
to
3ab5fad
Compare
| } | ||
|
|
||
| /** Destroys the autocomplete suggestion panel. */ | ||
| destroyPanel(): void { |
There was a problem hiding this comment.
Should this be an internal method? (Would the user ever have a reason to call it?)
There was a problem hiding this comment.
Hmm, someone could possibly call it if they knew that the autocomplete wouldn't be used anymore for some reason and they had a ton of options. But I don't really think it's that useful. Removing. We can always add to the API later if needed
| private _overlayRef: OverlayRef; | ||
| private _portal: TemplatePortal; | ||
| private _panelOpen: boolean = false; | ||
| private _closeWatcher: Subscription; |
There was a problem hiding this comment.
_panelClosures? I'd like to avoid the term "watchers" to avoid confusion with the Angular 1 concept.
Also add a description comment for this since it's not immediately obvious what it's for.
| * This method will close the panel if it receives a selection event from any of the options | ||
| * or a click on the backdrop. | ||
| */ | ||
| private _watchForClose() { |
There was a problem hiding this comment.
Different name that doesn't use watch?
| * the backdrop click into one observable. | ||
| */ | ||
| private _getOptionObs(): Observable<any>[] { | ||
| return this.autocomplete.options.map((option) => option.onSelect); |
There was a problem hiding this comment.
Don't need parens around option
| * of their selection events. This map will be used to merge the option events and | ||
| * the backdrop click into one observable. | ||
| */ | ||
| private _getOptionObs(): Observable<any>[] { |
There was a problem hiding this comment.
Make this a getter called optionSelections? Then the description is just something like
/** Stream of autocomplete option selections. */
| */ | ||
| private _watchForClose() { | ||
| // TODO(kara): add tab event watcher when adding keyboard events | ||
| this._closeWatcher = Observable.merge(...this._getOptionObs(), this._overlayRef.backdropClick()) |
There was a problem hiding this comment.
Won't this get stale is more options are added while the panel is open?
There was a problem hiding this comment.
This is addressed in the next PR, #2516, when filtering is added (and the options are often changing)
| @Input('mdAutocomplete') autocomplete: MdAutocomplete; | ||
|
|
||
| constructor(private _element: ElementRef, private _overlay: Overlay, | ||
| private _vcr: ViewContainerRef) {} |
|
@jelbourn Comments addressed! |
|
|
||
| if (!this._overlayRef.hasAttached()) { | ||
| this._overlayRef.attach(this._portal); | ||
| this._panelClosingSub = this.panelClosingActions.subscribe(() => this.closePanel()); |
|
|
||
| // remove body padding to keep consistent cross-browser | ||
| document.body.style.padding = '0'; | ||
| document.body.style.margin = '0'; |
There was a problem hiding this comment.
I don't think these styles are necessary any more since 38700e0
There was a problem hiding this comment.
Oh right! Forgot to update.
| dispatchEvent('focus', trigger); | ||
| fixture.detectChanges(); | ||
|
|
||
| const backdrop = <HTMLElement>overlayContainerElement.querySelector('.cdk-overlay-backdrop'); |
There was a problem hiding this comment.
Prefer as HTMLElement
(here and elsewhere)
| @import '../style/menu-common'; | ||
| @import '../a11y/a11y'; | ||
|
|
||
| @mixin md-option() { |
There was a problem hiding this comment.
Add comment explaining what this mixin is for (i.e., used by both select and autocomplete)
|
@jelbourn Updated |
|
LGTM |
jelbourn
added
pr: lgtm
action: merge
kara
force-pushed
the
autocomplete-pr
branch
5 times, most recently
from
17d809d
to
5e42f71
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. |
The autocomplete is a WIP. To keep PRs small, this PR only implements the panel toggling behavior. Future PRs will implement forms integration, positioning, and keyboard events.
r: @jelbourn