-
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
fix(autocomplete): up arrow should set last item active #2776
Conversation
export const AUTOCOMPLETE_OPTION_HEIGHT = 48; | ||
|
||
/** The total height of the autocomplete panel. */ | ||
export const MD_AUTOCOMPLETE_PANEL_HEIGHT = 256; |
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.
One constant has MD_
and the other doesn't; they should be consistent (I would omit the MD_
since they're really just meant for internal consumption.
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.
Good catch; not sure why I did that.
@@ -37,8 +47,12 @@ export class MdAutocompleteTrigger implements AfterContentInit, OnDestroy { | |||
private _portal: TemplatePortal; | |||
private _panelOpen: boolean = false; | |||
|
|||
/** The subscription to positioning changes in the autocomplete panel. */ | |||
private _panelPositionSub: Subscription; |
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.
_panelPositionSubscription
?
(generally avoiding abbreviations)
@@ -553,5 +676,15 @@ class FakeKeyboardEvent { | |||
preventDefault() {} | |||
} | |||
|
|||
class FakeViewportRuler { |
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.
Isn't there a common FakeviewportRuler
now?
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.
Oh hey. Didn't know about that, but that's way better. Will replace.
7ed5f6e
to
a5444f8
Compare
a5444f8
to
7d8e5e7
Compare
} | ||
|
||
/** Sets the active item to a previous enabled item in the list. */ | ||
setPreviousItemActive(): void { | ||
this._setActiveItemByDelta(-1); | ||
this._activeItemIndex === null && this._wrap ? this.setLastItemActive() | ||
: this._setActiveItemByDelta(-1); |
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.
Need to handle the case when activeItemIndex
is null and wrap is false
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.
(with a unit test)
7d8e5e7
to
3b4836e
Compare
LGTM |
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. |
r: @jelbourn @andrewseguin