virtual-scroll: add support for `scrollToOffset` and `scrollToIndex` #12272

mmalerba · 2018-07-18T21:17:47Z

Note: scrollToIndex currently does not work with the AutoSizeVirtualScrollStrategy. Support for this will be added in a future PR.

This PR is based off #11498 by @amcdnl. I removed the lazy option from #11498 to avoid bloating the API. I'm not sure how common the use case is, and its relatively simple to implement on top using the current APIs.

mmalerba · 2018-07-18T21:19:58Z

CLA justification - I asked @amcdnl if its ok to take over from his PR, he said it was ok.

amcdnl

LGTM

amcdnl · 2018-07-22T13:52:17Z

I removed the lazy option from #11498 to avoid bloating the API. I'm not sure how common the use case is, and its relatively simple to implement on top using the current APIs.

Sad to see that not get included. Its relatively simple and adds a big value imo.

Looks good otherwise.

mmalerba · 2018-07-23T17:25:01Z

@jelbourn @crisbeto what do you guys think of the lazy option? worth making part of the API?

crisbeto · 2018-07-23T18:23:56Z

I wasn't part of the previous PR so I don't remember what it was about. What does the lazy option do?

mmalerba · 2018-07-23T19:42:35Z

If the specified index is already in the viewport, it doesn't bother scrolling (if lazy were not specified it would scroll that element to the top of the viewport)

crisbeto · 2018-07-23T19:48:45Z

To me it sounds like a very narrow use case. If we provide a method like scrollToElement(index) then people should be able to do it themselves, if they need it.

naveedahmed1 · 2018-07-23T20:19:39Z

Should be helpful in certain cases.

jelbourn

LGTM aside from a last couple of nits, add merge-ready when ready.

for the lazy thing, I agree that we should keep the API simpler for now and see how it goes once released

jelbourn · 2018-07-24T17:27:52Z

src/cdk-experimental/scrolling/fixed-size-virtual-scroll.ts

@@ -77,6 +77,13 @@ export class FixedSizeVirtualScrollStrategy implements VirtualScrollStrategy {
  /** @docs-private Implemented as part of VirtualScrollStrategy. */
  onRenderedOffsetChanged() { /* no-op */ }

+  /** Scroll to the offset for the given index. */
+  scrollToIndex(index: number, behavior: ScrollBehavior) {


It should still have void, though, since dgeni can't use the inferred types. Also missing @param JsDocs

jelbourn · 2018-07-24T17:28:43Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

@@ -245,7 +246,28 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
    }
  }

-  /** Sets the scroll offset on the viewport. */
+  /**  Scrolls to the offset on the viewport. */
+  scrollToOffset(offset: number, behavior: ScrollBehavior = 'auto') {


Add @param JsDocs?

googlebot · 2018-07-24T18:13:51Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-24T21:27:42Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@amcdnl

…12272) Note: `scrollToIndex` currently does not work with the `AutoSizeVirtualScrollStrategy`. Support for this will be added in a future PR. This PR is based off #11498 by @amcdnl. I removed the `lazy` option from #11498 to avoid bloating the API. I'm not sure how common the use case is, and its relatively simple to implement on top using the current APIs.

naveedahmed1 · 2018-10-23T18:07:33Z

Any update on scrollToOffset and scrollToIndex support with AutoSizeVirtualScrollStrategy?

angular-automatic-lock-bot · 2019-09-09T23:02:49Z

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.}

amcdnl and others added 7 commits May 24, 2018 13:49

virtual scroll: implement scrollTo functions

868bbf9

fix(scroll): fix scrolling not working all the time

a31a4c8

chore: pr nit

017a131

chore: throw exception for offset getter

2527306

chore: better checks for scroll smooth

05c29b9

Merge branch 'master' into scroll-to

dce1cad

tweak API slightly and add demo

38487f7

mmalerba assigned amcdnl, jelbourn and crisbeto Jul 18, 2018

mmalerba requested review from amcdnl, jelbourn and crisbeto July 18, 2018 21:17

mmalerba requested a review from devversion as a code owner July 18, 2018 21:17

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Jul 18, 2018

mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 18, 2018

mmalerba added pr: merge safe target: patch This PR is targeted for the next patch release and removed pr: merge safe labels Jul 18, 2018

update BUILD file

91bcd58

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 18, 2018

mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 18, 2018

mmalerba added 2 commits July 18, 2018 15:48

Merge remote-tracking branch 'upstream/master' into vs-scroll-to

5a63174

fix duplicate template ref name

0947381

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 18, 2018

angular deleted a comment from googlebot Jul 19, 2018

amcdnl approved these changes Jul 22, 2018

View reviewed changes

amcdnl mentioned this pull request Jul 22, 2018

virtual scroll: implement scrollTo functions #11498

Closed

jelbourn approved these changes Jul 24, 2018

View reviewed changes

jelbourn added the pr: lgtm label Jul 24, 2018

mmalerba added 2 commits July 24, 2018 10:58

Merge remote-tracking branch 'upstream/master' into vs-scroll-to

54705af

address comments

496571b

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 24, 2018

fix aot

983e03b

mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 24, 2018

mmalerba added action: merge The PR is ready for merge by the caretaker pr: merge safe and removed pr: merge safe labels Jul 24, 2018

mmalerba merged commit 6a0b8f7 into angular:master Jul 25, 2018

mmalerba mentioned this pull request Jul 25, 2018

virtual-scroll: emit the currently scrolled to index #12381

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

virtual-scroll: add support for `scrollToOffset` and `scrollToIndex` #12272

virtual-scroll: add support for `scrollToOffset` and `scrollToIndex` #12272

Uh oh!

mmalerba commented Jul 18, 2018

Uh oh!

mmalerba commented Jul 18, 2018

Uh oh!

amcdnl left a comment

Uh oh!

amcdnl commented Jul 22, 2018

Uh oh!

mmalerba commented Jul 23, 2018

Uh oh!

crisbeto commented Jul 23, 2018

Uh oh!

mmalerba commented Jul 23, 2018

Uh oh!

crisbeto commented Jul 23, 2018

Uh oh!

naveedahmed1 commented Jul 23, 2018

Uh oh!

jelbourn left a comment

Uh oh!

jelbourn Jul 24, 2018

Uh oh!

jelbourn Jul 24, 2018

Uh oh!

googlebot commented Jul 24, 2018

Uh oh!

googlebot commented Jul 24, 2018

Uh oh!

naveedahmed1 commented Oct 23, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 9, 2019

Uh oh!

Uh oh!

virtual-scroll: add support for scrollToOffset and scrollToIndex #12272

virtual-scroll: add support for scrollToOffset and scrollToIndex #12272

Uh oh!

Conversation

mmalerba commented Jul 18, 2018

Uh oh!

mmalerba commented Jul 18, 2018

Uh oh!

amcdnl left a comment

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Jul 22, 2018

Uh oh!

mmalerba commented Jul 23, 2018

Uh oh!

crisbeto commented Jul 23, 2018

Uh oh!

mmalerba commented Jul 23, 2018

Uh oh!

crisbeto commented Jul 23, 2018

Uh oh!

naveedahmed1 commented Jul 23, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

jelbourn Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 24, 2018

Uh oh!

googlebot commented Jul 24, 2018

Uh oh!

naveedahmed1 commented Oct 23, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 9, 2019

Uh oh!

Uh oh!

virtual-scroll: add support for `scrollToOffset` and `scrollToIndex` #12272

virtual-scroll: add support for `scrollToOffset` and `scrollToIndex` #12272