Skip to content

fix(scrolling): update virtual scroll viewport size on resize #18058

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 29, 2019
Merged

fix(scrolling): update virtual scroll viewport size on resize #18058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/cdk/scrolling/virtual-scroll-viewport.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,15 @@ import {
Directive,
ViewContainerRef
} from '@angular/core';
import {async, ComponentFixture, fakeAsync, flush, inject, TestBed} from '@angular/core/testing';
import {
async,
ComponentFixture,
fakeAsync,
flush,
inject,
TestBed,
tick,
} from '@angular/core/testing';
import {animationFrameScheduler, Subject} from 'rxjs';


Expand Down Expand Up @@ -74,6 +82,17 @@ describe('CdkVirtualScrollViewport', () => {
expect(viewport.getViewportSize()).toBe(500);
}));

it('should update the viewport size when the page viewport changes', fakeAsync(() => {
finishInit(fixture);
spyOn(viewport, 'checkViewportSize').and.callThrough();

dispatchFakeEvent(window, 'resize');
fixture.detectChanges();
tick(20); // The resize listener is debounced so we need to flush it.

expect(viewport.checkViewportSize).toHaveBeenCalled();
}));

it('should get the rendered range', fakeAsync(() => {
finishInit(fixture);

Expand Down
29 changes: 26 additions & 3 deletions src/cdk/scrolling/virtual-scroll-viewport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,20 @@ import {
ViewChild,
ViewEncapsulation,
} from '@angular/core';
import {animationFrameScheduler, asapScheduler, Observable, Subject, Observer} from 'rxjs';
import {
animationFrameScheduler,
asapScheduler,
Observable,
Subject,
Observer,
Subscription,
} from 'rxjs';
import {auditTime, startWith, takeUntil} from 'rxjs/operators';
import {ScrollDispatcher} from './scroll-dispatcher';
import {CdkScrollable, ExtendedScrollToOptions} from './scrollable';
import {CdkVirtualForOf} from './virtual-for-of';
import {VIRTUAL_SCROLL_STRATEGY, VirtualScrollStrategy} from './virtual-scroll-strategy';

import {ViewportRuler} from './viewport-ruler';

/** Checks if the given ranges are equal. */
function rangesEqual(r1: ListRange, r2: ListRange): boolean {
Expand Down Expand Up @@ -142,18 +149,33 @@ export class CdkVirtualScrollViewport extends CdkScrollable implements OnInit, O
/** A list of functions to run after the next change detection cycle. */
private _runAfterChangeDetection: Function[] = [];

/** Subscription to changes in the viewport size. */
private _viewportChanges = Subscription.EMPTY;

constructor(public elementRef: ElementRef<HTMLElement>,
private _changeDetectorRef: ChangeDetectorRef,
ngZone: NgZone,
@Optional() @Inject(VIRTUAL_SCROLL_STRATEGY)
private _scrollStrategy: VirtualScrollStrategy,
@Optional() dir: Directionality,
scrollDispatcher: ScrollDispatcher) {
scrollDispatcher: ScrollDispatcher,
/**
* @deprecated `viewportRuler` parameter to become required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just mark this @breaking-change, not @deprecated since the change is making it required rather than removing it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve been marking these as deprecated in other places, because it’s something standard that’ll show up in people’s tooling, whereas breaking-change is something we made up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I'm not sure if this will show up the way we want though, it will probably show viewportRuler as a deprecated param. What might work better is TS's method overloading. We can mark the old signature deprecated and the new one not deprecated.

Obviously we already have this same pattern all over the code base, but it might be nice to start doing it the way I suggested going forward, I'll leave it up to you

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point that we should be using overloads instead, but I realized that Ivy won't generate the correct code if the constructor is overloaded with a different number of parameters. I've submitted a PR to fix the compiler here angular/angular#34590 so until that gets released I'll keep it as is.

* @breaking-change 11.0.0
*/
@Optional() viewportRuler?: ViewportRuler) {
super(elementRef, scrollDispatcher, ngZone, dir);

if (!_scrollStrategy) {
throw Error('Error: cdk-virtual-scroll-viewport requires the "itemSize" property to be set.');
}

// @breaking-change 11.0.0 Remove null check for `viewportRuler`.
if (viewportRuler) {
this._viewportChanges = viewportRuler.change().subscribe(() => {
this.checkViewportSize();
});
}
}

ngOnInit() {
Expand Down Expand Up @@ -188,6 +210,7 @@ export class CdkVirtualScrollViewport extends CdkScrollable implements OnInit, O
// Complete all subjects
this._renderedRangeSubject.complete();
this._detachedSubject.complete();
this._viewportChanges.unsubscribe();

super.ngOnDestroy();
}
Expand Down
3 changes: 2 additions & 1 deletion tools/public_api_guard/cdk/scrolling.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ export declare class CdkVirtualScrollViewport extends CdkScrollable implements O
orientation: 'horizontal' | 'vertical';
renderedRangeStream: Observable<ListRange>;
scrolledIndexChange: Observable<number>;
constructor(elementRef: ElementRef<HTMLElement>, _changeDetectorRef: ChangeDetectorRef, ngZone: NgZone, _scrollStrategy: VirtualScrollStrategy, dir: Directionality, scrollDispatcher: ScrollDispatcher);
constructor(elementRef: ElementRef<HTMLElement>, _changeDetectorRef: ChangeDetectorRef, ngZone: NgZone, _scrollStrategy: VirtualScrollStrategy, dir: Directionality, scrollDispatcher: ScrollDispatcher,
viewportRuler?: ViewportRuler);
attach(forOf: CdkVirtualForOf<any>): void;
checkViewportSize(): void;
detach(): void;
Expand Down