virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` #10290

mmalerba · 2018-03-06T19:26:01Z

progress toward #10113

crisbeto · 2018-03-06T19:33:05Z

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

@@ -122,6 +128,9 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
      this._ngZone.run(() => {
        this._renderedRangeSubject.next(this._renderedRange = range);
        this._changeDetectorRef.markForCheck();
+        this._ngZone.runOutsideAngular(() => this._ngZone.onStable.pipe(take(1)).subscribe(() => {


It seems a little weird to run outside Angular and then waiting for the zone to stabilize. What about flipping them around like:

ngZone.onStable.pipe(take(1)).subscribe(() => { ngZone.runOutsideAngular(() => onContentCentered()); });

Wouldn't subscribing within the zone cause another change detection when the zone stabilizes?

Does NgZone patch into the observable subscriptions? AFAIK it only does browser APIs.

@jelbourn I was worried about that too, but I tested by creating a component that did:

ngAfterContentChecked() { this.ngZone.onStable.subscribe(() => {}) }

It didn't go into a death spiral, so it looks like @crisbeto's suggestion would work.

To me it seems clearer to have it the way it is though. It makes it obvious that I don't want any change detection as a result of this subscription.

crisbeto · 2018-03-06T19:33:35Z

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

+  /** Measure the combined size of all of the rendered items. */
+  measureRenderedContentSize() {
+    return this.orientation === 'horizontal' ?
+        this._contentWrapper.nativeElement.offsetWidth :


Pull out the this._contentWrapper.nativeElement into a variable?

crisbeto · 2018-03-06T19:34:08Z

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

@@ -179,9 +186,13 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
  }

  /** Update the viewport's total content size. */
-  private _updateTotalContentSize() {
+  private _updateTotalContentSize(renderedContentSize) {


This one's missing a type.

jelbourn · 2018-03-07T15:16:29Z

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

      this._renderContentForOffset(this._viewport.measureScrollOffset());
    }
  }

+  /** Called when the range of items rendered in the DOM has changed. */
+  onContentRendered() {


Name this function for what it does instead of when it's called?

I see these more as hooks for someone implementing a VirtualScrollStrategy, therefore I think it makes sense to name them based on when they're called. What it actually does will depend on the particular concrete strategy

Would it make sense to then break the content of this function out into another function that gets called here? With the current name/behavior, it's not immediately clear what the code is meant to do without jumping around the file.

jelbourn · 2018-03-07T15:21:23Z

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

@@ -122,6 +128,9 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
      this._ngZone.run(() => {
        this._renderedRangeSubject.next(this._renderedRange = range);
        this._changeDetectorRef.markForCheck();
+        this._ngZone.runOutsideAngular(() => this._ngZone.onStable.pipe(take(1)).subscribe(() => {


Wouldn't subscribing within the zone cause another change detection when the zone stabilizes?

mmalerba · 2018-03-07T22:12:31Z

Comments addressed PTAL

jelbourn · 2018-03-07T22:15:55Z

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

@@ -129,6 +126,17 @@ export class AutoSizeVirtualScrollStrategy implements VirtualScrollStrategy {
    this._addBufferPx = addBufferPx;
  }

+  /**
+   * Checks the size of the currently rendered content and uses it tp update the estimated item size


"tp" -> "to"

jelbourn

LGTM

…y` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts

…y` (angular#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts

* 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

angular-automatic-lock-bot · 2019-09-08T11:02:08Z

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

virtual-scroll: add onContentRendered hook to VirtualScrollStrategy

c9b60ce

mmalerba added the pr: needs review label Mar 6, 2018

mmalerba assigned amcdnl, jelbourn and crisbeto Mar 6, 2018

mmalerba requested review from amcdnl, jelbourn and crisbeto March 6, 2018 19:26

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

crisbeto reviewed Mar 6, 2018

View reviewed changes

jelbourn reviewed Mar 7, 2018

View reviewed changes

jelbourn approved these changes Mar 7, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Mar 7, 2018

address comemnts

fe2c325

mmalerba force-pushed the vs branch from 11ab12e to fe2c325 Compare March 7, 2018 22:17

mmalerba merged commit 733e86e into angular:virtual-scroll Mar 7, 2018

mmalerba added a commit that referenced this pull request Mar 7, 2018

virtual-scroll: add onContentRendered hook to `VirtualScrollStrateg…

919a899

…y` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts

mmalerba added a commit that referenced this pull request Mar 9, 2018

virtual-scroll: add onContentRendered hook to `VirtualScrollStrateg…

7d3be19

…y` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts

mmalerba added a commit that referenced this pull request Mar 19, 2018

virtual-scroll: add onContentRendered hook to `VirtualScrollStrateg…

13a99f3

…y` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts

mmalerba added a commit to mmalerba/components that referenced this pull request Apr 5, 2018

virtual-scroll: add onContentRendered hook to `VirtualScrollStrateg…

f04b470

…y` (angular#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts

angular-automatic-lock-bot bot unassigned amcdnl Sep 8, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` #10290

virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` #10290

Uh oh!

mmalerba commented Mar 6, 2018

Uh oh!

crisbeto Mar 6, 2018

Uh oh!

jelbourn Mar 7, 2018

Uh oh!

crisbeto Mar 7, 2018

Uh oh!

mmalerba Mar 7, 2018

Uh oh!

crisbeto Mar 6, 2018

Uh oh!

crisbeto Mar 6, 2018

Uh oh!

jelbourn Mar 7, 2018

Uh oh!

mmalerba Mar 7, 2018

Uh oh!

jelbourn Mar 7, 2018

Uh oh!

jelbourn Mar 7, 2018

Uh oh!

mmalerba commented Mar 7, 2018

Uh oh!

jelbourn Mar 7, 2018

Uh oh!

jelbourn left a comment

Uh oh!

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

Uh oh!

Uh oh!

virtual-scroll: add onContentRendered hook to VirtualScrollStrategy #10290

virtual-scroll: add onContentRendered hook to VirtualScrollStrategy #10290

Uh oh!

Conversation

mmalerba commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` #10290

virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` #10290