virtual-scroll: add logic to correct the scroll position as user move… #11137

mmalerba · 2018-05-03T19:22:39Z

…s toward the top

mmalerba · 2018-05-03T19:31:06Z

For some reason the changes from #11085 are showing up here, I have no idea why since that PR is shown as merged already.

Edit: seems to be fixed now, must've been some sort of glitch

crisbeto

LGTM, left a couple of nits.

crisbeto · 2018-05-03T20:20:15Z

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

+        // If the rendered content offset was specified as an offset to the end of the content,
+        // rewrite it as an offset to the start of the content.
+        if (this._renderedContentOffsetNeedsRewrite) {
+          this._ngZone.onStable.pipe(take(1)).subscribe(() => {


Since you're subscribing to this._ngZone.onStable.pipe(take(1)) in both the if and the else clauses, it might make sense to combine them and do the check inside the subscription.

crisbeto · 2018-05-03T20:21:50Z

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

@@ -144,6 +144,13 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
    }
  }

+  /** Implemented as part of VirtualScrollStrategy. */


nit: should this be @docs-private?

yeah, should mark all of these hook funcions as @docs-private

crisbeto · 2018-05-03T20:23:35Z

src/demo-app/virtual-scroll/virtual-scroll-demo.ts

+  sortBy(prop: 'name' | 'capital') {
+    this.statesObservable.next(this.states.map(s => ({...s})).sort((a, b) => {
+      const aProp = a[prop], bProp = b[prop];
+      return aProp < bProp ? -1 : (aProp > bProp ? 1 : 0);


nit: consider breaking up the nested ternary.

amcdnl · 2018-05-04T14:08:11Z

src/cdk-experimental/scrolling/virtual-for-of.ts

@@ -147,8 +147,15 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
      private _differs: IterableDiffers,
      /** The virtual scrolling viewport that these items are being rendered in. */
      @SkipSelf() private _viewport: CdkVirtualScrollViewport) {
-    this.dataStream.subscribe(data => this._data = data);
-    this._viewport.renderedRangeStream.subscribe(range => this._onRenderedRangeChange(range));
+    this.dataStream.subscribe(data => {


Do you ever need to unsubscribe from there?

For this one it should be ok, since the underlying Subject is completed in the destroy function, but renderedRangeStream I probably should, since that one belongs to the viewport

amcdnl

Looks good overall.

jelbourn · 2018-05-04T16:37:06Z

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

    this._lastRenderedContentSize = viewport.measureRenderedContentSize();
    this._averager.addSample(viewport.getRenderedRange(), this._lastRenderedContentSize);
    this._updateTotalContentSize(this._lastRenderedContentSize);
  }

+  private _checkRenderedContentOffset() {


Add method description?

amcdnl · 2018-05-06T19:20:56Z

Just tested this code in real world app, looks good there.

jelbourn

LGTM

jelbourn · 2018-05-07T19:37:30Z

Looks like build file needs to be updated

* feat(virtual-scroll): fixed size virtual scroll (#9316) * feat(virtual-scroll): fixed size virtual scroll * address some comments * change VirtualScrollStrategy interface a bit * address some more comments * fix lint & build * chore: move virtual-scroll to cdk-experimental (#9974) * virtual-scroll: simplify scroll listener logic (#10102) * virtual-scroll: only move views that need to be moved (#10099) * virtual-scroll: only move views that need to be moved * address comments * virtual-scroll: switch `throttleTime` to `sampleTime` (#10179) * virtual-scroll: switch throttleTime to sampleTime * add comment * virtual-scroll: allow user to pass `Observable<T[]>` (#10158) * virtual-scroll: rename `Range` to `ListRange` to avoid confusion with native `Range` (#10220) * virtual-scroll: add autosize scroll strategy (#10219) * rename fixed size virtual scroll directive * add autosize virtual scroll strategy * add item size estimator class * add logic for jumping rendered content based on scroll position * address comments * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy` (#10504) * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy`. This still has a couple issues that need to be ironed out and it doesn't have the code for correcting the error between the predicted and actual scroll position. (See various TODOs for additional things that need work). * fix lint * address comments * address comments * fix bazel * fix devapp * fix lint * cleanup * virtual-scroll: rewrite offset in terms of "to-top" and fix a bug where items were removed too soon (#10986) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * address comments * virtual-scroll: address amcdnl's feedback (#10988) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * virtual-scroll: address amcdnl's feedback * virtual-scroll: fix updating when data changes and add trackBy demos (#11085) * virtual-scroll: add logic to correct the scroll position as user move… (#11137) * virtual-scroll: add logic to correct the scroll position as user moves toward the top * address comments * fix(scrolling): adds right to fix pushed content (#11192) * fix(scrolling): adds right to fix pushed content * chore: address feedback * chore: address pr feedback * virtual-scroll: add tests for fixed size items (#11255) * virtual-scroll: add tests for fixed size items * address comments * fix bug in fixed size strategy * fix bazel build * virtual-scroll: add tests for `cdkVirtualFor` logic (#11275) * merge fixed size test components into one * add tests for cdkVirtualFor logic * allow undefined to be explicitly passed as trackBy * fix bazel build * address comments * add some basic tests (#11295) * fix lint * virtual-scroll: add e2e tests for autosize scroll strategy (#11345) * set up virtual scroll page in e2e app * add gulp task for e2e:watch * add e2e tests for autosize * address comments * address comments * fix lint and tests * fix bad rebase * fix(scrolling): scrollbar jump when data change on scroll (#11193) * fix(scrolling): data change not updating size * chore: remove todo * chore: more performant fix for jump

naveedahmed1 · 2018-07-20T19:47:57Z

I am using virtual-scroll in a component, when user clicks an item I get the current scroll position
which I get through '_renderedContentOffset' and store it in a singleton service.:

@ViewChild('virtualViewport') public virtualViewport: ElementRef;
.....
this._myService.scrollToOffset = this.virtualViewport['_renderedContentOffset'];

and when user goes back to the list page I set the scroll position of the div.

ngAfterViewInit() {
          let thisRef = this;
          setTimeout(function () {
                    let e = document.getElementById('list');
                    e.scrollTop = thisRef._as.scrollToOffset;
        }, 10);
}

I thought this would work, but it doesn't seem to work since it doesn't set the scroll position to where it was before clicking the item.

Does this indicate that there is some issue with the calculations of scroll position?

mmalerba · 2018-07-23T17:13:18Z

@naveedahmed1 for autosize strategy, the currently displayed data is only an estimation of what should be shown at the current scroll position. Setting it back to the same scroll position later is not guaranteed to show the same data. Once the autosize strategy supports the scrollToIndex method you can use that to go to a specific index in the list.

angular-automatic-lock-bot · 2019-09-09T07:02:51Z

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

mmalerba added the pr: needs review label May 3, 2018

mmalerba assigned amcdnl, jelbourn and crisbeto May 3, 2018

mmalerba requested review from amcdnl, jelbourn and crisbeto May 3, 2018 19:22

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 3, 2018

virtual-scroll: add logic to correct the scroll position as user move…

1932c53

…s toward the top

mmalerba force-pushed the vs2 branch from 7a5a05d to 1932c53 Compare May 3, 2018 19:24

mmalerba added the target: master only label May 3, 2018

crisbeto approved these changes May 3, 2018

View reviewed changes

amcdnl reviewed May 4, 2018

View reviewed changes

amcdnl approved these changes May 4, 2018

View reviewed changes

jelbourn reviewed May 4, 2018

View reviewed changes

mmalerba added 2 commits May 4, 2018 15:45

address comments

644428f

Merge remote-tracking branch 'upstream/virtual-scroll' into vs2

95c4d41

mmalerba force-pushed the vs2 branch from bf832e4 to 95c4d41 Compare May 4, 2018 22:49

jelbourn approved these changes May 7, 2018

View reviewed changes

mmalerba merged commit 604f743 into angular:virtual-scroll May 7, 2018

mmalerba added pr: lgtm and removed pr: needs review labels May 11, 2018

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

virtual-scroll: add logic to correct the scroll position as user move… #11137

virtual-scroll: add logic to correct the scroll position as user move… #11137

Uh oh!

Conversation

mmalerba commented May 3, 2018

Uh oh!

mmalerba commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto May 3, 2018

Choose a reason for hiding this comment

Uh oh!

crisbeto May 3, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba May 4, 2018

Choose a reason for hiding this comment

Uh oh!

crisbeto May 3, 2018

Choose a reason for hiding this comment

Uh oh!

amcdnl May 4, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba May 4, 2018

Choose a reason for hiding this comment

Uh oh!

amcdnl left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn May 4, 2018

Choose a reason for hiding this comment

Uh oh!

amcdnl commented May 6, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented May 7, 2018

Uh oh!

naveedahmed1 commented Jul 20, 2018

Uh oh!

mmalerba commented Jul 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

mmalerba commented May 3, 2018 •

edited

Loading