feat(table): add optional footer row #10330

andrewseguin · 2018-03-08T22:09:15Z

Adds an optional footer row to the table. Since header row has similar logic, a refactoring of the table code was necessary to avoid duplicating work.

Note that the table.spec.ts was reworked as well to remove a lot of boilerplate (most tests begin by setting up a fixture, component, table element, etc.). Added a utility function at the top of the file to handle this.

Closes #7548

jelbourn · 2018-03-08T22:30:15Z

src/cdk/table/cell.ts

@@ -8,12 +8,17 @@

 import {ContentChild, Directive, ElementRef, Input, TemplateRef} from '@angular/core';

+/** Base class for a cell definition. Captures a column's cell template definition. */
+export interface BaseCellDef {


Typically you don't use "Base" for interfaces. Just CellDef or CellDefinition?

jelbourn · 2018-03-08T23:37:12Z

src/cdk/table/cell.ts

+})
+export class CdkFooterCell {
+  constructor(columnDef: CdkColumnDef, elementRef: ElementRef) {
+    elementRef.nativeElement.classList.add(`cdk-column-${columnDef.cssClassFriendlyName}`);


Create a base class with the constructor so that all three directives don't duplicate it? Similar for the mat versions

jelbourn · 2018-03-08T23:39:44Z

src/cdk/table/table.md

-the column a name. Each column definition then further defines both a header-cell template
-(`cdkHeaderCellDef`) and a data-cell template (`cdkCellDef`).
+the column a name. Each column definition then further defines a header-cell template
+(`cdkHeaderCellDef`), data-cell template (`cdkCellDef`), and footer-cell 


"...and an optional footer-cell template"

See comment below

jelbourn · 2018-03-08T23:40:24Z

src/cdk/table/table.md

+the column a name. Each column definition then further defines a header-cell template
+(`cdkHeaderCellDef`), data-cell template (`cdkCellDef`), and footer-cell 
+template (`cdkFooterCellDef`). Note that each of these cell definitions need to be present if they
+are used by their respective row definition. 


Note that each of these cell definitions need to be present if they are used by their respective row definition.

It's not clear to me what this means

Removed the sentence and clarified the previous one a bit.

Each column definition can contain a header-cell template
(cdkHeaderCellDef), data-cell template (cdkCellDef), and footer-cell
template (cdkFooterCellDef).

Technically they are all optional, but it depends on which row def uses it. E.g. if a header row def uses the column, it assumes a header cell def is present in the column

jelbourn · 2018-03-08T23:48:12Z

src/cdk/table/table.ts

+ * @docs-private
+ */
+@Directive({selector: '[footerRowPlaceholder]'})
+export class FooterRowPlaceholder {


Change "Placeholder" to "Outlet" in this PR since we're adding a new one?

jelbourn · 2018-03-08T23:54:51Z

src/cdk/table/table.ts

@@ -214,6 +238,14 @@ export class CdkTable<T> implements CollectionViewer, OnInit, AfterContentChecke
   */
  @ContentChild(CdkHeaderRowDef) _headerRowDef: CdkHeaderRowDef;

+  /**
+   * Template definition used as the header container. By default it stores the footer row


"header" -> "footer"

jelbourn · 2018-03-08T23:55:58Z

src/cdk/table/table.ts

-    // Make sure that the user has at least added a header row or row def.
-    if (!this._headerRowDef && !this._rowDefs.length) {
+    // Make sure that the user has at least added header, footer, or data row def.
+    if (!this._headerRowDef && !this._footerRowDef && !this._rowDefs.length) {


I don't think the presence of a footer should negate the need for a header or rows.

Currently the only requirement is that the table must have defined either amat-header-row, mat-row. If you do not have either, then an error is thrown. If you do not have one of them, they just are not rendered.

This continues the check - if you do not have any rows defined, an error is thrown. Otherwise, anything excluded just is not rendered.

So you would say it's valid to have a footer row template defined with no header and no data row templates?

Yes mostly for symmetry. I've seen requests for tables that have only a header row and some requests to hide the header row. Doing the same treatment for footer

Some context: #7604

jelbourn · 2018-03-09T00:07:59Z

src/cdk/table/table.ts

+   * of where to place the new row template in the placeholder.
+   */
+  private _renderRow(
+      placeholder: any, rowDef: BaseRowDef, context: any = {}, index = 0) {


Is the context not CdkCellOutletRowContext?

jelbourn · 2018-03-09T00:11:44Z

src/demo-app/table/footer/footer-table.scss

+mat-table {
+  height: 500px;
+  overflow: auto;
+}


Add a demo- class around these?

jelbourn · 2018-03-09T23:36:50Z

You should sync your client as well

andrewseguin · 2018-03-09T23:41:24Z

Synced

andrewseguin · 2018-03-14T17:40:19Z

Ready for review

jelbourn · 2018-03-14T23:22:34Z

src/cdk/table/table.ts

+   * of where to place the new row template in the outlet.
+   */
+  private _renderRow(
+      outlet: any, rowDef: BaseRowDef, context: CdkCellOutletRowContext<T> = {}, index = 0) {


outlet should be able to be more specific than any

Done - added a base class for the row outlets

jelbourn · 2018-03-29T23:33:49Z

Will need to be rebased after the native <table> change goes in; let me know when it's ready

andrewseguin · 2018-03-30T20:18:33Z

Rebased

jelbourn · 2018-03-30T20:22:38Z

src/demo-app/table/footer/footer-table.html

@@ -0,0 +1,16 @@
+<h3>Table with Footer</h3>


Did you want to change this one to a native table?

jelbourn · 2018-03-30T20:23:49Z

src/cdk/table/table.ts

@@ -43,21 +50,34 @@ import {
  getTableUnknownDataSourceError
 } from './table-errors';

+export interface RowOutlet {


Add description

jelbourn · 2018-03-30T20:24:45Z

src/cdk/table/table.ts

@@ -66,8 +86,9 @@ export class HeaderRowPlaceholder {
 * material library.
 */
 export const CDK_TABLE_TEMPLATE = `
-  <ng-container headerRowPlaceholder></ng-container>
-  <ng-container rowPlaceholder></ng-container>`;
+  <ng-container headerRowOutlet></ng-container>


Just noticed this, but this constant should be @docs-private

jelbourn · 2018-03-30T20:28:31Z

src/cdk/table/table.ts

+    // header row def change so that the content check will render the header row.
+    if (this._footerRowDef) {
+      this._footerRowDefChanged = true;
+    }


Could this check (and the header one above) be replaced with

this._footerRowDefChanged = !!this._footerRowDef;

Or should it not be always set to false when _footerRowDef is undefined?

jelbourn · 2018-03-30T20:30:27Z

src/cdk/table/table.ts


-    this._getCellTemplatesForRow(row).forEach(cell => {
+    this._getCellTemplates(rowDef).forEach(cellTemplate => {


It was like this before, but this would probably be better as a for of just to avoid the extra function invocations in the critical path.

jelbourn · 2018-03-30T20:41:45Z

src/cdk/table/table.ts

-    tbody.appendChild(this._rowPlaceholder.elementRef.nativeElement);
+    thead.appendChild(this._headerRowOutlet.elementRef.nativeElement);
+    tbody.appendChild(this._rowOutlet.elementRef.nativeElement);
+    tfoot.appendChild(this._footerRowOutlet.elementRef.nativeElement);


You can avoid some repetition:

const sections = [ {tag: 'thead', outlet: this._headerRowOutlet}, {tag: 'tbody', outlet: this._rowOutlet}, {tag: 'tfoot', outlet: this._footerRowOutlet}, ]; for (const section of sections) { const element = document.createElement(section.tag); element.appendChild(section.outlet.elementRef.nativeElement); this._elementRef.nativeElement.appendChild(element); }

andrewseguin · 2018-03-30T21:17:00Z

Cool thanks for the review -- good suggestions

jelbourn

LGTM

mmalerba · 2018-04-18T22:04:13Z

@andrewseguin needs rebase, re-apply merge ready when done

angular-automatic-lock-bot · 2019-09-08T19:02:36Z

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

andrewseguin added pr: needs review target: minor This PR is targeted for the next minor release labels Mar 8, 2018

andrewseguin requested a review from jelbourn March 8, 2018 22:09

andrewseguin requested a review from amcdnl as a code owner March 8, 2018 22:09

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

jelbourn reviewed Mar 9, 2018

View reviewed changes

jelbourn reviewed Mar 14, 2018

View reviewed changes

andrewseguin force-pushed the table-footer branch 2 times, most recently from 3fda997 to a749798 Compare March 26, 2018 17:27

This was referenced Mar 27, 2018

[Table] Add a footer row (e.g. totals) #7548

Closed

Footer in md-table #6570

Closed

andrewseguin force-pushed the table-footer branch from a749798 to 98e2e45 Compare March 30, 2018 17:33

jelbourn reviewed Mar 30, 2018

View reviewed changes

jelbourn approved these changes Mar 30, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Mar 30, 2018

andrewseguin force-pushed the table-footer branch from 3a194d1 to b121be8 Compare March 30, 2018 21:51

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

andrewseguin force-pushed the table-footer branch from b121be8 to 6dda130 Compare April 19, 2018 16:28

andrewseguin added the action: merge The PR is ready for merge by the caretaker label Apr 19, 2018

feat(table): add optional footer row

84642b2

andrewseguin force-pushed the table-footer branch from 6dda130 to 84642b2 Compare April 20, 2018 16:44

andrewseguin merged commit 6df3709 into angular:master Apr 23, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(table): add optional footer row #10330

feat(table): add optional footer row #10330

andrewseguin commented Mar 8, 2018

jelbourn Mar 8, 2018

jelbourn Mar 8, 2018

jelbourn Mar 8, 2018

andrewseguin Mar 9, 2018

jelbourn Mar 8, 2018

andrewseguin Mar 9, 2018

jelbourn Mar 8, 2018

jelbourn Mar 8, 2018

jelbourn Mar 8, 2018

andrewseguin Mar 9, 2018

jelbourn Mar 9, 2018

andrewseguin Mar 9, 2018 •

edited

Loading

jelbourn Mar 9, 2018

jelbourn Mar 9, 2018

jelbourn commented Mar 9, 2018

andrewseguin commented Mar 9, 2018

andrewseguin commented Mar 14, 2018

jelbourn Mar 14, 2018

andrewseguin Mar 20, 2018

jelbourn commented Mar 29, 2018

andrewseguin commented Mar 30, 2018

jelbourn Mar 30, 2018

jelbourn Mar 30, 2018

jelbourn Mar 30, 2018

jelbourn Mar 30, 2018

jelbourn Mar 30, 2018

jelbourn Mar 30, 2018

andrewseguin commented Mar 30, 2018

jelbourn left a comment

mmalerba commented Apr 18, 2018

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


		this._getCellTemplatesForRow(row).forEach(cell => {
		this._getCellTemplates(rowDef).forEach(cellTemplate => {

feat(table): add optional footer row #10330

feat(table): add optional footer row #10330

Conversation

andrewseguin commented Mar 8, 2018

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 Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Mar 9, 2018

andrewseguin commented Mar 9, 2018

andrewseguin commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Mar 29, 2018

andrewseguin commented Mar 30, 2018

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 Mar 30, 2018

jelbourn left a comment

Choose a reason for hiding this comment

mmalerba commented Apr 18, 2018

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

andrewseguin Mar 9, 2018 •

edited

Loading