Skip to content

virtual-scroll: add update method #11210

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

virtual-scroll: add update method #11210

wants to merge 8 commits into from

Conversation

amcdnl
Copy link
Contributor

@amcdnl amcdnl commented May 7, 2018

Adds a method to force resize of the viewport. This is required when the container height/width changes.

@amcdnl amcdnl self-assigned this May 7, 2018
@amcdnl amcdnl requested a review from mmalerba May 7, 2018 20:27
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 7, 2018
mmalerba
mmalerba reviewed May 7, 2018
View reviewed changes
src/cdk-experimental/scrolling/virtual-scroll-viewport.ts Outdated
/** Update the viewport dimensions and re-render. */
updateViewport() {
const viewportEl = this.elementRef.nativeElement;
this._viewportSize = this.orientation === 'horizontal' ?
Copy link
Contributor

Choose a reason for hiding this comment

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

since this also appears in the Promise above, can we extract it into a separate _measureViewportSize method

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts Outdated
@@ -137,6 +137,15 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
this._detachedSubject.complete();
}

/** Update the viewport dimensions and re-render. */
updateViewport() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: updateViewportSize to be a little more specific about the purpose

@amcdnl
Copy link
Contributor Author

amcdnl commented May 7, 2018

@mmalerba - Addressed feedback

mmalerba
mmalerba reviewed May 7, 2018
View reviewed changes
src/cdk-experimental/scrolling/virtual-scroll-viewport.ts Outdated
@@ -102,8 +102,7 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
ngOnInit() {
const viewportEl = this.elementRef.nativeElement;
Promise.resolve().then(() => {
this._viewportSize = this.orientation === 'horizontal' ?
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can remove viewportEl and use this.elementRef.nativeElement when listening below

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts Outdated
@@ -303,4 +299,11 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
}
return this._forOf.measureRangeSize(range, this.orientation);
}

/** Measure the viewport size. */
_measureViewportSize() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make this private

@amcdnl
Copy link
Contributor Author

amcdnl commented May 7, 2018

@mmalerba - done :)

mmalerba
mmalerba reviewed May 12, 2018
View reviewed changes
src/cdk-experimental/scrolling/virtual-scroll-viewport.ts Outdated
/** Update the viewport dimensions and re-render. */
updateViewportSize() {
this._measureViewportSize();
this._scrollStrategy.onDataLengthChanged();
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird to be calling onDataLengthChanged here since the data length did not change. Can you just leave a TODO so I remember to clean this up later. I'm going to add logic for handling content resize, this can probably call whatever method I create for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mmalerba
Copy link
Contributor

Also now that there are tests, it would be good to add a test for this case too.

@amcdnl
Copy link
Contributor Author

amcdnl commented May 13, 2018

@mmalerba - done :)

mmalerba
mmalerba reviewed May 13, 2018
View reviewed changes
src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts Outdated
// TODO(mmalerba): Add test that it corrects the initial render if it didn't render enough,
// once it actually does that.
});
expect((viewport._renderedContentTransform as any).changingThisBreaksApplicationSecurity)
Copy link
Contributor

Choose a reason for hiding this comment

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

did this get copied down here by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - sorry.

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts Outdated
@@ -56,6 +56,20 @@ describe('CdkVirtualScrollViewport', () => {

expect(viewport.getViewportSize()).toBe(testComponent.viewportSize);
}));

it('should update viewport size', fakeAsync(() => {
viewport.elementRef.nativeElement.style['height'] = '300px';
Copy link
Contributor

Choose a reason for hiding this comment

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

testComponent.viewportSize = 300

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

expect(size).toBe(300);

viewport.elementRef.nativeElement.style['height'] = '500px';
Copy link
Contributor

Choose a reason for hiding this comment

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

testComponent.viewportSize = 500

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts Outdated
it('should update viewport size', fakeAsync(() => {
viewport.elementRef.nativeElement.style['height'] = '300px';
viewport.updateViewportSize();
const size = viewport.getViewportSize();
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need to save this, just expect(viewport.getViewportSize()).toBe(300)

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

viewport.elementRef.nativeElement.style['height'] = '500px';
viewport.updateViewportSize();
const newSize = viewport.getViewportSize();
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need to save in variable

@amcdnl amcdnl mentioned this pull request May 16, 2018
Merged
@amcdnl
Copy link
Contributor Author

amcdnl commented May 16, 2018

@mmalerba - Done :)

mmalerba
mmalerba reviewed May 16, 2018
View reviewed changes
src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts Outdated
@@ -492,7 +502,6 @@ describe('CdkVirtualScrollViewport', () => {

// TODO(mmalerba): Add test that it corrects the initial render if it didn't render enough,
// once it actually does that.
});
Copy link
Contributor

Choose a reason for hiding this comment

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

accidental change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, thats what happens when I merge via Github haha

@amcdnl amcdnl changed the title feat(scroll): add update method virtual-scroll: add update method May 17, 2018
@mmalerba mmalerba changed the base branch from virtual-scroll to master May 22, 2018 19:16
@mmalerba mmalerba changed the base branch from master to virtual-scroll May 22, 2018 19:16
amcdnl added a commit to amcdnl/material2 that referenced this pull request May 23, 2018
@amcdnl
virtual-scroll: add update method angular#11210
0826773
@amcdnl amcdnl mentioned this pull request May 23, 2018
Merged
@amcdnl
Copy link
Contributor Author

amcdnl commented May 23, 2018

Closing in favor of #11476

@amcdnl amcdnl closed this May 23, 2018
@amcdnl amcdnl deleted the force-resize branch May 23, 2018 15:45
mmalerba pushed a commit that referenced this pull request May 25, 2018
@amcdnl @mmalerba
virtual-scroll: add update method (#11476)
2a68edb 
Supersedes #11210
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mmalerba mmalerba mmalerba left review comments

Assignees

@amcdnl amcdnl

Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amcdnl @mmalerba @googlebot