fix(virtual-scroll): emit on viewChange inside the NgZone
#12873
Conversation
mmalerba
added
the
P2
googlebot
added
the
cla: yes
src/cdk/scrolling/virtual-for-of.ts
Outdated
| @SkipSelf() private _viewport: CdkVirtualScrollViewport) { | ||
| @SkipSelf() private _viewport: CdkVirtualScrollViewport, | ||
| /** The Angular zone. */ | ||
| ngZone: NgZone) { |
There was a problem hiding this comment.
Should this be optional with a breaking change for next major?
There was a problem hiding this comment.
virtual scroll isn't released yet so its ok
src/cdk/scrolling/virtual-for-of.ts
Outdated
| @@ -166,14 +167,16 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy | |||
| /** The set of available differs. */ | |||
| private _differs: IterableDiffers, | |||
| /** The virtual scrolling viewport that these items are being rendered in. */ | |||
| @SkipSelf() private _viewport: CdkVirtualScrollViewport) { | |||
| @SkipSelf() private _viewport: CdkVirtualScrollViewport, | |||
| /** The Angular zone. */ | |||
There was a problem hiding this comment.
This comment doesn't contribute much.
| this.dataStream.subscribe(data => { | ||
| this._data = data; | ||
| this._onRenderedDataChange(); | ||
| }); | ||
| this._viewport.renderedRangeStream.pipe(takeUntil(this._destroyed)).subscribe(range => { | ||
| this._renderedRange = range; | ||
| this.viewChange.next(this._renderedRange); | ||
| ngZone.run(() => this.viewChange.next(this._renderedRange)); |
There was a problem hiding this comment.
You should be able to write a test for this. Something like:
const spy = jasmine.createSpy('something');
renderedRangeStream.subscribe(() => spy(NgZone.isInAngularZone()));
expect(spy).toHaveBeenCalledWith(true);
|
test added |
| @@ -166,14 +167,15 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy | |||
| /** The set of available differs. */ | |||
| private _differs: IterableDiffers, | |||
| /** The virtual scrolling viewport that these items are being rendered in. */ | |||
| @SkipSelf() private _viewport: CdkVirtualScrollViewport) { | |||
| @SkipSelf() private _viewport: CdkVirtualScrollViewport, | |||
| ngZone: NgZone) { | |||
There was a problem hiding this comment.
I left this before, but I think it got lost between commits: shouldn't we make this an optional parameter with a breaking change for the next major?
There was a problem hiding this comment.
virtual scroll isn't released yet, so it's not necessary
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
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. |
fixes #12869