-
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): don't scroll panel when option is visible #4905
fix(autocomplete): don't scroll panel when option is visible #4905
Conversation
f04e716
to
7d4f46e
Compare
@jelbourn could I get this assigned? 😄 |
7d4f46e
to
b693409
Compare
@kara I would really appreciate if you could review this! I think it makes keyboard scrolling feel more natural. |
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.
Looks good; mostly comment readability nits. Being especially nitpicky because scrolling/positioning is pretty complicated.
@@ -315,15 +315,30 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy { | |||
/** | |||
* Given that we are not actually focusing active options, we must manually adjust scroll | |||
* to reveal options below the fold. First, we find the offset of the option from the top | |||
* of the panel. The new scrollTop will be that offset - the panel height + the option | |||
* height, so the active option will be just visible at the bottom of the panel. | |||
* of the panel. If that offset if below the fold, the new scrollTop will be the offset - |
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.
Typo: if below the fold
-> is below the fold
// Scroll up to reveal selected option above the panel | ||
if (optionOffset < panelTop) { | ||
this.autocomplete._setScrollTop(optionOffset); | ||
return; |
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.
If you used an if-else
conditional, you could avoid using the early return here and on line 340. I think it might make it clearer that the two cases are mutually exclusive with fewer lines.
const newScrollTop = | ||
const panelTop = this.autocomplete._getScrollTop(); | ||
|
||
// Scroll up to reveal selected option above the panel |
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.
Nit: above the panel
-> scrolled above the panel top
When first reading the comment, I thought you meant literally above the panel element (not scrolled out of view).
return; | ||
} | ||
|
||
// Scroll down to reveal selection option below the panel |
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.
Typo: selection option below the panel
-> selected option scrolled below the panel bottom
* height, so the active option will be just visible at the bottom of the panel. | ||
* of the panel. If that offset if below the fold, the new scrollTop will be the offset - | ||
* the panel height + the option height, so the active option will be just visible at the | ||
* bottom of the panel. If that offset is above the top of the panel, the new scrollTop |
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.
Nit: above the top of the panel
-> above the top of the visible panel
* of the panel. If that offset if below the fold, the new scrollTop will be the offset - | ||
* the panel height + the option height, so the active option will be just visible at the | ||
* bottom of the panel. If that offset is above the top of the panel, the new scrollTop | ||
* will become the offset. If that offset is visible within the panel, the scrollTop is not |
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.
Nit: visible within the panel
-> visible within the panel already
.
|
||
// Expect no scrolling to have occurred. Still showing bottom of 6th option. | ||
expect(scrollContainer.scrollTop) | ||
.toEqual(32, `Expected panel to not scroll back.`); |
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.
Nit: The failure message is a bit vague. It seems like you should move the guidance in the comment to the failure message.
@kara changes made! |
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
…#4905) * fix(autocomplete): don't scroll panel if option is visible * Clean up comments & indent by 4 on line continuations
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. |
When scrolling down to an option well below the fold and then scrolling back up a couple of options, it feels jarring to have the selected option "stick" to the bottom of the panel. This fix keeps the panel from scrolling when the selected option is fully visible.
cc @kara
Before
After