feat(cdk/table) scrollable table body #20414

MichaelJamesParsons · 2020-08-25T18:10:01Z

Changes

Added an internal API for overriding the DOM structure rendered by the CdkTable.
Added an experimental directive to enable scrollable body content for flex tables only.

Caveats

Sticky columns are not supported for tables with scrollable body content. This will be addressed in a follow-up pull request.

src/cdk/table/table.ts

andrewseguin

Biggest thing for me is the API taking in the height - I would prefer that users don't need to pass this via their template, and it doesn't seem easy to have an auto type maxheight that matches the container

andrewseguin · 2020-08-25T20:45:55Z

...ollable-table-body/mat-scrollable-table-body-flex/mat-scrollable-table-body-flex-example.css

@@ -0,0 +1,10 @@
+/**


I expect this can be deleted - these tag names won't show up in the example and they already apply flex styles

andrewseguin · 2020-08-25T20:50:55Z

src/cdk/table/table.ts

  /** Adds native table sections (e.g. tbody) and moves the row outlets into them. */
-  private _applyNativeTableSections() {
+  private _applyNativeTableSections(): DocumentFragment {


Rename to reflect that this won't apply anything, but instead create the standard document fragment for the native table sections, e.g. _getNativeTableSections()

andrewseguin · 2020-08-25T21:10:07Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+   * Show a scroll bar if the table's body exceeds this height. The height may be specified with
+   * any valid CSS unit of measurement.
+   */
+  @Input('scrollableBody')


I struggled a bit to get this case working: I want my table to expand in height to fit the content of the container, and I don't want to have to set a maxHeight. Is that possible to do with this?

I think the desired behavior will vary for each app:

Fixed height (e.g. 200px, 60vh, calc(100vh - OFFSET_FROM_TOP))

Fill height of immediate parent element.

Fill remaining space in a higher level parent element (e.g immediate parent is a card and the table should fill the remaining space on the page).

The first use case is covered. IMO, the other two should be supported in another directive.

What are the options for letting people set this via CSS? Turning it into a TS API makes it harder to e.g. use media queries, your Sass layout constants, etc.

andrewseguin · 2020-08-25T21:11:08Z

src/cdk/table/table-layout-strategy.ts

+export type _TableLayout = DocumentFragment | null;
+
+/** Interface for a service that constructs the DOM structure for a {@link CdkTable}. */
+export interface _TableLayoutStrategy {


I went back and forth on whether the API should just be getLayout, or what you have. I think what you have is fine - it's clear that there are two very separate layout types for the table and they should both be considered separately

I also went back and forth on this and eventually chose this API for a few reasons:

All implementations of a single getLayout() method would contain the same boilerplate code:

if (/* native table */) { /* native layout */ } else { /* flex layout */ }

The explicit methods make the intent clearer and prevent erroneously returning the wrong layout for the table type.

Some strategies may want to fall back on the default layout (e.g. scrollable body strategy is only supported for flex layouts, so the default layout is used for native tables).

andrewseguin · 2020-08-25T21:14:05Z

src/cdk/table/table.ts

+    }
+
+    if (!layout && this._isNativeHtmlTable) {
+      layout = this._applyNativeTableSections();


It seems a little off to me that we use null as a way to signal to the table that it should fall back on a default. Should there not just be a default layout strategy that is always used, and that contains the native table document fragment builder?

sgtm. I extracted the default layout into its own _DefaultTableLayoutStrategy. LMK what you think.

crisbeto · 2020-08-26T07:26:00Z

src/cdk-experimental/config.bzl

@@ -6,6 +6,7 @@ CDK_EXPERIMENTAL_ENTRYPOINTS = [
    "menu",
    "listbox",
    "popover-edit",
+    "scrollable-table-body",


I'm not sure whether this should be in its own separate module since it's very closely coupled to the table. Maybe we can have material-experimental/table that exports a MatScrollableTableBody module? I believe that would allow build tools to remove the code if it isn't used, while grouping under table.

Yeah, I'd also prefer this. Alternatively a tertiary entry-point under material(-experimental)/table/ could work too.

crisbeto · 2020-08-26T07:27:56Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+
+/** A directive that enables scrollable body content for flex tables. */
+@Directive({
+  selector: 'cdk-table[scrollableBody], mat-table[scrollableBody]',


It seems weird to be mixing CDK and Material selectors within the same module. Generally we have the CDK as the base class and the Material variant extends it.

+1, we would need to separate these

crisbeto · 2020-08-26T07:29:55Z

src/cdk/table/table.ts

-      protected readonly _viewRepeater: _ViewRepeater<T, RenderRow<T>, RowContext<T>>) {
+      protected readonly _viewRepeater: _ViewRepeater<T, RenderRow<T>, RowContext<T>>,
+      @Optional() @Inject(_TABLE_LAYOUT_STRATEGY)
+      private readonly _layoutStrategy: _TableLayoutStrategy|null) {


While the |null here is correct, I believe that ViewEngine will throw an error for it. You might have to make the parameter type non-null, declare a separate nullable property and assign to it:

private _layoutStrategy: _TableLayoutStrategy|null; constructor(@Optional() @Inject(_TABLE_LAYOUT_STRATEGY) layoutStrategy: _TableLayoutStrategy) { this._layoutStrategy = layoutStrategy; }

Can we make this optional instead? Similar to this PR?

private readonly _layoutStrategy?: _TableLayoutStrategy

Yes, if I remember correctly optional parameters shouldn't throw.

crisbeto · 2020-08-26T07:31:03Z

...ollable-table-body/cdk-scrollable-table-body-flex/cdk-scrollable-table-body-flex-example.css

+/**
+ * Add basic flex styling so that the cells evenly space themselves in the row.
+ */
+cdk-row, cdk-header-row, cdk-footer-row {


It might be better to use the CSS classes for these selectors since they'll automatically work if we switch the example to use a table-based mat-table.

+1, we prefer using css classes for styling over element selectors, and in examples we always prefix styled with example- to make it clear which styles come from the component and which come from the example.

crisbeto · 2020-08-26T07:32:03Z

...llable-table-body/cdk-scrollable-table-body-flex/cdk-scrollable-table-body-flex-example.html

@@ -0,0 +1,33 @@
+<button mat-button color="primary" (click)="addRow()">Add row</button>
+<button mat-button color="primary" (click)="removeRow()">Remove row</button>
+<button mat-button color="primary" (click)="nextMaxHeight()">Change max height</button>


nit: I don't think you're supposed to have multiple primary buttons next to each other.

Ah, I did that out of habit. My team's UX mocks tend to have primary buttons next to each other.

I updated the template to align with the table demo (raised, default buttons).

crisbeto · 2020-08-26T07:32:36Z

...rollable-table-body/cdk-scrollable-table-body-flex/cdk-scrollable-table-body-flex-example.ts

+    return this.data;
+  }
+
+  disconnect() {}


Should the data stream be completed here?

jelbourn · 2020-09-01T19:27:12Z

src/cdk/table/table-layout-strategy.ts

+    new InjectionToken<_TableLayoutStrategy>('_TableLayoutStrategy');
+
+
+export class _DefaultTableLayoutStrategy implements _TableLayoutStrategy {


I prefer to name classes for what they do, rather than how they're used (e.g. "Default" here). How about something like _StandardTableLayoutStrategy?

jelbourn · 2020-09-01T19:28:24Z

src/cdk/table/table-layout-strategy.ts

+/** Interface for a service that constructs the DOM structure for a {@link CdkTable}. */
+export interface _TableLayoutStrategy {
+  /** Constructs the DOM structure for a native table. */
+  getNativeLayout(table: CdkTable<any>): DocumentFragment;


I believe all of the any in this file should be unknown

(similar elsewhere where you don't know/care about the generic type)

jelbourn · 2020-09-01T19:33:38Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+  readonly bodyCssClass = 'cdk-table-scrollable-table-body';
+  readonly footerCssClass = 'cdk-table-scrollable-table-footer';
+
+  constructor(@Inject(DOCUMENT) private readonly _document: any) {


Rather than making the injected _document a class member with private, you can create a separate property with type Document so that the types are correct for uses throughout the file.

(we have to inject it as any otherwise SSR breaks due to how TypeScript decorators output)

jelbourn · 2020-09-01T19:35:13Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+   * Returns the DOM structure for a native table. Scrollable body content is not supported for
+   * native tables. Return `null` to use the default {@link CdkTable} native table layout.


Maybe you discussed this with @andrewseguin and @mmalerba, but what's the background on this only supporting flexbox-based table? We want to strongly encourage people to use the native table, so not supporting that for virtual-scroll seems like a problem to me.

jelbourn · 2020-09-01T19:37:21Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+
+    for (const section of sections) {
+      const element = this._document.createElement('div');
+      element.classList.add(section.selector);


selector should be cssClass since you're treating is as such here

jelbourn · 2020-09-01T19:38:30Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+
+/** A directive that enables scrollable body content for flex tables. */
+@Directive({
+  selector: 'cdk-table[scrollableBody], mat-table[scrollableBody]',


+1, we would need to separate these

jelbourn · 2020-09-01T19:42:57Z

src/cdk-experimental/scrollable-table-body/scrollable-table-body-layout.ts

+   * Show a scroll bar if the table's body exceeds this height. The height may be specified with
+   * any valid CSS unit of measurement.
+   */
+  @Input('scrollableBody')


What are the options for letting people set this via CSS? Turning it into a TS API makes it harder to e.g. use media queries, your Sass layout constants, etc.

jelbourn · 2020-09-01T19:43:32Z

...ollable-table-body/cdk-scrollable-table-body-flex/cdk-scrollable-table-body-flex-example.css

+/**
+ * Add basic flex styling so that the cells evenly space themselves in the row.
+ */
+cdk-row, cdk-header-row, cdk-footer-row {


+1, we prefer using css classes for styling over element selectors, and in examples we always prefix styled with example- to make it clear which styles come from the component and which come from the example.

andrewseguin · 2022-04-22T11:55:25Z

@MichaelJamesParsons Would you still like to try and land this change?

angular-automatic-lock-bot · 2025-01-11T00:11:55Z

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

MichaelJamesParsons requested review from andrewseguin, devversion, jelbourn and mmalerba as code owners August 25, 2020 18:10

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 25, 2020

andrewseguin reviewed Aug 25, 2020

View reviewed changes

src/cdk/table/table.ts Outdated Show resolved Hide resolved

andrewseguin reviewed Aug 25, 2020

View reviewed changes

crisbeto reviewed Aug 26, 2020

View reviewed changes

MichaelJamesParsons force-pushed the table-scrollable-body branch 2 times, most recently from 6200db9 to 92fc5d5 Compare August 27, 2020 17:46

jelbourn reviewed Sep 1, 2020

View reviewed changes

garrettld mentioned this pull request Jan 25, 2021

bug(table): Sticky header flickering #21576

Open

MichaelJamesParsons mentioned this pull request Jan 26, 2021

feat(cdk/table): virtual scroll directive for tables #21708

Closed

MichaelJamesParsons added 3 commits January 30, 2021 09:55

feat(cdk/table) scrollable table body

45efdc5

cleanup

9eee000

cleanup

e430a4c

MichaelJamesParsons force-pushed the table-scrollable-body branch from a69813b to e430a4c Compare August 10, 2021 20:54

devversion removed their request for review August 18, 2021 12:58

andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021

andrewseguin self-assigned this Dec 11, 2024

andrewseguin closed this Dec 11, 2024

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 11, 2025

		new InjectionToken<_TableLayoutStrategy>('_TableLayoutStrategy');


		export class _DefaultTableLayoutStrategy implements _TableLayoutStrategy {

		* Returns the DOM structure for a native table. Scrollable body content is not supported for
		* native tables. Return `null` to use the default {@link CdkTable} native table layout.

feat(cdk/table) scrollable table body #20414

feat(cdk/table) scrollable table body #20414

Conversation

MichaelJamesParsons commented Aug 25, 2020

Changes

Caveats

andrewseguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Apr 22, 2022

angular-automatic-lock-bot bot commented Jan 11, 2025