Skip to content

Conversation

willshowell
Copy link
Contributor

@willshowell willshowell commented Aug 3, 2017

Fixes #6209

This also adds tests for the existing panelClosingActions

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 3, 2017
@willshowell willshowell force-pushed the autocomplete-escape-stream branch from 549b86c to 133b03f Compare August 3, 2017 13:21
@willshowell willshowell force-pushed the autocomplete-escape-stream branch 3 times, most recently from 8d5b0b6 to 6d8da62 Compare August 23, 2017 16:49
@willshowell willshowell force-pushed the autocomplete-escape-stream branch from 6d8da62 to 47088f6 Compare September 11, 2017 20:06
@willshowell
Copy link
Contributor Author

@kara @crisbeto would one of you mind reviewing this? It's really simple and a big help for #3334 (comment).

if (event.keyCode === ESCAPE && this.panelOpen) {
this._resetActiveItem();
this.closePanel();
this._escapeEventStream.next(event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it only fires for escape presses, I don't think it's necessary to pass along the event.

private _closingActionsSubscription: Subscription;

/** Stream of escape keyboard events. */
private _escapeEventStream = new Subject<KeyboardEvent>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also complete this in ngOnDestroy?

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 13, 2017
@willshowell willshowell force-pushed the autocomplete-escape-stream branch 2 times, most recently from e134ecc to c6cd992 Compare September 25, 2017 15:05
@willshowell willshowell force-pushed the autocomplete-escape-stream branch from c6cd992 to b61af4f Compare October 2, 2017 19:11
@kara kara merged commit f4673a5 into angular:master Oct 3, 2017
@willshowell willshowell deleted the autocomplete-escape-stream branch October 3, 2017 03:43
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[autocomplete] panelClosingActions does not include escape key
5 participants