virtual-scroll: add support for user-provided content wrapper #12183

mmalerba · 2018-07-12T22:22:13Z

jelbourn · 2018-07-12T23:08:36Z

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

@@ -33,6 +35,15 @@ function rangesEqual(r1: ListRange, r2: ListRange): boolean {
 }


+@Directive({


Add JsDoc class description?

jelbourn · 2018-07-12T23:10:11Z

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

-
-  /** The raw string version of the rendered content transform. */
-  private _rawRenderedContentTransform: string;
+  /** The the rendered content transform. */


This description just echoes the name now- expand to describe what it's for?

jelbourn · 2018-07-12T23:24:20Z

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

@@ -1,11 +1,14 @@
+<!-- Place the ng-content in a template that we can conditionally render in different places. -->
+<ng-template #rawContent><ng-content></ng-content></ng-template>


Per offline discussion, we can change this to just use straight <ng-content> with a selector for [cdkVirtualScrollContent]

crisbeto · 2018-07-13T16:13:27Z

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

  position: absolute;
  top: 0;
  left: 0;
  will-change: contents, transform;
 }

-.cdk-virtual-scroll-orientation-horizontal .cdk-virtual-scroll-content-wrapper {
+.cdk-virtual-scroll-orientation-horizontal .cdk-virtual-scroll-content {


Could this selector and the one below be nested under the .cdk-virtual-scroll-orientation-?

crisbeto · 2018-07-13T16:14:12Z

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

+  @ViewChild(CdkVirtualScrollContent, {read: ElementRef}) _viewContentWrapper: ElementRef;
+
+  /** The element in the viewport's content that wraps the rendered content. */
+  @ContentChild(CdkVirtualScrollContent, {read: ElementRef}) _contentContentWrapper: ElementRef;


Type the ElementRef here to ElementRef<HTMLElement> for better type safety.

crisbeto · 2018-07-13T16:14:31Z

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

@@ -107,8 +120,9 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
  /** A list of functions to run after the next change detection cycle. */
  private _runAfterChangeDetection: Function[] = [];

-  constructor(public elementRef: ElementRef, private _changeDetectorRef: ChangeDetectorRef,
-              private _ngZone: NgZone, private _sanitizer: DomSanitizer,
+  constructor(public elementRef: ElementRef,


Also stronger typing for the ElementRef here.

mmalerba · 2018-07-13T17:33:40Z

FYI, I'm going to change some things around. I think the attribute selector isn't actually necessary, users can just place the whole table, dl, etc inside the viewport and have it wrapped by the content wrapper

mmalerba · 2018-07-13T22:31:41Z

Ok, changed it to suggest wrapping the entire table or list instead. This PR includes changes from #12181 so marking it blocked until that goes in

amcdnl

LGTM

jelbourn · 2018-07-16T21:25:54Z

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

@@ -16,10 +16,48 @@ cdk-virtual-scroll-viewport {

 .cdk-virtual-scroll-orientation-horizontal .cdk-virtual-scroll-content-wrapper {
  bottom: 0;
+
+  & > dl,


Add a comment that explains why this block of styles exists?

jelbourn · 2018-07-16T21:26:44Z

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

 }

 .cdk-virtual-scroll-orientation-vertical .cdk-virtual-scroll-content-wrapper {
  right: 0;
+
+  & > dl,


Could this block be captured in a mixin instead of repeating it?

jelbourn · 2018-07-16T21:40:27Z

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

-  private _rawRenderedContentTransform: string;
+  /**
+   * The the rendered content transform, used to move the rendered portion of the content to the
+   * correct place in the viewport.


How about

The CSS transform applied to the rendered subset of items so that they appear within the bounds of the visible viewport

jelbourn · 2018-07-16T21:41:49Z

src/cdk-experimental/scrolling/virtual-scroll.md

+can be placed inside. To enable virtual scrolling over these type of elements, place the elements in
+their proper parent, and then wrap the whole thing in a `cdk-virtual-scroll-viewport`. Be careful
+that the parent does not introduce additional space (e.g. via `margin` or `padding`) as it may
+interfere with the scrolling.


"may interfere" -> "will interfere"?

mmalerba · 2018-07-17T00:07:51Z

comments addressed, PTAL

jelbourn

LGTM

ngbot · 2018-07-17T17:38:23Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
status "ci/circleci: build" is failing
status "continuous-integration/travis-ci/pr" is failing

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.

josephperrott · 2018-07-18T15:56:17Z

@mmalerba It looks like this has lint errors with the latest push.

mmalerba · 2018-07-18T18:21:39Z

whoops, should be fixed now

angular-automatic-lock-bot · 2019-09-09T06:03:25Z

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 pr: merge safe labels Jul 12, 2018

mmalerba assigned amcdnl, jelbourn and crisbeto Jul 12, 2018

mmalerba requested review from amcdnl, jelbourn and crisbeto July 12, 2018 22:22

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

jelbourn reviewed Jul 12, 2018

View reviewed changes

crisbeto reviewed Jul 13, 2018

View reviewed changes

mmalerba force-pushed the vs-attr branch 2 times, most recently from d6d2830 to 209b578 Compare July 13, 2018 22:29

mmalerba added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Jul 13, 2018

amcdnl approved these changes Jul 15, 2018

View reviewed changes

mmalerba force-pushed the vs-attr branch from 209b578 to f60c6fc Compare July 16, 2018 16:09

mmalerba mentioned this pull request Jul 16, 2018

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

Merged

jelbourn reviewed Jul 16, 2018

View reviewed changes

mmalerba force-pushed the vs-attr branch from f60c6fc to e947cb7 Compare July 17, 2018 00:07

mmalerba added 2 commits July 17, 2018 09:30

virtual-scroll: add support for user-provided content wrapper

12be786

address comments

cbfacdd

mmalerba force-pushed the vs-attr branch from e947cb7 to cbfacdd Compare July 17, 2018 16:31

mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Jul 17, 2018

jelbourn approved these changes Jul 17, 2018

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 17, 2018

josephperrott removed the action: merge The PR is ready for merge by the caretaker label Jul 18, 2018

fix type issue

661eda7

mmalerba force-pushed the vs-attr branch from 485db25 to 661eda7 Compare July 18, 2018 18:21

mmalerba added the action: merge The PR is ready for merge by the caretaker label Jul 18, 2018

josephperrott added target: patch This PR is targeted for the next patch release and removed target: master only labels Jul 18, 2018

josephperrott merged commit 7301fa0 into angular:master Jul 18, 2018

josephperrott pushed a commit that referenced this pull request Jul 18, 2018

virtual-scroll: add support for user-provided content wrapper (#12183)

12f672b

josephperrott pushed a commit that referenced this pull request Jul 18, 2018

virtual-scroll: add support for user-provided content wrapper (#12183)

bdab139

crisbeto mentioned this pull request Jul 21, 2018

Cannot use *cdkVirtualFor on a <ng-container> when viewport is autosize. #12307

Closed

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

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

		@@ -33,6 +35,15 @@ function rangesEqual(r1: ListRange, r2: ListRange): boolean {
		}


		@Directive({

		@@ -1,11 +1,14 @@
		<!-- Place the ng-content in a template that we can conditionally render in different places. -->
		<ng-template #rawContent><ng-content></ng-content></ng-template>

virtual-scroll: add support for user-provided content wrapper #12183

virtual-scroll: add support for user-provided content wrapper #12183

Uh oh!

Conversation

mmalerba commented Jul 12, 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!

mmalerba commented Jul 13, 2018

Uh oh!

mmalerba commented Jul 13, 2018

Uh oh!

amcdnl left a comment

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 Jul 17, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Jul 17, 2018

Uh oh!

josephperrott commented Jul 18, 2018

Uh oh!

mmalerba commented Jul 18, 2018

Uh oh!

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

Uh oh!

Uh oh!