feat(table): enable multiple data rows #10964

andrewseguin · 2018-04-23T16:24:00Z

Introduces a new table input multiTemplateDataRows which allows the table to have multiple data rows per data object.

Old paradigm

Each data object passed to the table is rendered as a data row. Since the mapping is 1:1, the data array itself can be used with the IterableDiffer to determine which rows need to change.

New paradigm

Each data object passed to the table may be rendered by one or more rows. The IterableDiffer cannot use the data array itself anymore since it will not correctly understand add/remove/move operations in the rows.

Instead, the IterableDiffer will be diffed with a DataRow object that represents a pair containing a data object and row template. When the data changes, the list of DataRow objects will be constructed and diffed to get the list of changes.

Caveats for discussion

The row context previously contained an index value that represented both the row index and data object index (they were the same. Now, that index value represents the index of the data. To get the row index, there is an additional context property called rowIndex.

Should it be swapped such that index is the row index and the data index can be stored in dataIndex?

Similarly, the trackBy function receives an index value. Previously the index matched both the data and row. Now, its explicitly the data index.

Should trackBy receive the row index?

jelbourn · 2018-04-26T00:49:33Z

For the index stuff: I think that, when in multiple-row mode, you should have separate renderIndex and dataIndex properties for both the context and trackBy; I wouldn't make either just index because it will always be ambiguous. However, when in the standard mode, it should stay just index that that will by far be the most common case.

jelbourn · 2018-04-26T00:51:43Z

src/cdk/table/table.ts

@@ -98,6 +99,16 @@ export const CDK_TABLE_TEMPLATE = `
 */
 abstract class RowViewRef<T> extends EmbeddedViewRef<CdkCellOutletRowContext<T>> { }

+/**
+ * Set of properties that represent a single rendered row. Acts as the row's identity for the
+ * `IterableDiffer`.


Can you expand this to explain more of the why this is necessary? Should also explicitly say something like "There will be one DataRow instance per ____"

We also should avoid making this part of the public API.

Added more to the comment, not sure whether its too verbose.

jelbourn · 2018-04-26T00:52:32Z

src/cdk/table/table.ts

+   * `CdkRowDef<T>` object. With the two keys, the cache points to a `DataRow<T>` object that
+   * contains the pair.
+   */
+  private _cachedDataRowsMap = new Map<T, Map<CdkRowDef<T>, DataRow<T>>>();


Can this use WeakMap? It doesn't look like you need to iterate over it.

Unfortunately not. T may be a primitive value which would be an invalid WeakMap key.

jelbourn · 2018-04-26T00:54:03Z

src/cdk/table/table.ts

+   * `CdkRowDef<T>` object. With the two keys, the cache points to a `DataRow<T>` object that
+   * contains the pair.
+   */
+  private _cachedDataRowsMap = new Map<T, Map<CdkRowDef<T>, DataRow<T>>>();


Why can't this just be a map of T -> DataRow<T>?

Each T may map into multiple DataRow<T> objects since each object represents a pair of data and row template.

This setup allows us to uniquely identify these pairs so that they can be re-used if they show up again between renders. This is necessary for the IterableDiffer to understand the correct operations, e.g. when a pairing is moved.

jelbourn · 2018-04-26T00:57:52Z

src/cdk/table/table.ts

+   * defined in the table, or otherwise the default row which does not have a when predicate.
+   */
+  @Input()
+  get enableRowMultiplex(): boolean { return this._enableRowMultiplex; }


Okay, so I refreshed my memory about multiplexers, and they actually do the opposite of what we want here: they take multiple input signals and output one. The opposite of that is "demuxing", and I think enableRowDemux is a bit too jargony for this API.

How about just multiTemplateRows?

Perhaps it should be multiTemplateDataRows since I intend to allow multiple header and footer rows which would not depend on this input.

jelbourn · 2018-04-26T01:02:14Z

src/cdk/table/table.ts

@@ -262,8 +306,9 @@ export class CdkTable<T> implements CollectionViewer, OnInit, AfterContentChecke
      this._applyNativeTableSections();
    }

-    // TODO(andrewseguin): Setup a listener for scrolling, emit the calculated view to viewChange
-    this._dataDiffer = this._differs.find([]).create(this._trackByFn);
+    this._dataDiffer = this._differs.find([]).create((_i: number, dataRow: DataRow<T>) => {


Add comment explaining what is happening here? It's not quite clear to me

jelbourn · 2018-04-26T01:05:46Z

src/cdk/table/table.ts

+   * row definitions. If the previous list already contained a particular pair, it should be reused
+   * so that the differ equates their references.
+   */
+  private _getDataRows(): DataRow<T>[] {


Having both _getDataRows and _getDataRowsForData is confusing

Agreed, struggled coming up with a better name for these. One returns the list of all the DataRow objects to render, while one returns the list of DataRow objects for a particular data object.

Any ideas?

I think _getAllRenderRows and _getRenderRowsForData would be good

andrewseguin · 2018-04-26T18:39:27Z

Making the index and naming changes; will add pr: needs review label when I pushed the latest

andrewseguin · 2018-04-26T21:00:45Z

Ready for review

jelbourn · 2018-04-26T21:04:49Z

src/cdk/table/row.ts

+  /** Index location of the rendered row that this cell is located within. */
+  renderIndex?: number;
+
+  /** Index of the data object in the provided data array. */


Remove index?

jelbourn · 2018-04-26T21:07:24Z

src/cdk/table/table.spec.ts

  dataSource: FakeDataSource = new FakeDataSource();
  columnsToRender = ['column_a', 'column_b', 'column_c'];
+  isIndex1Columns = ['index1Column'];
+  hasC3Columns = ['c3Column'];


These names are pretty cryptic

Renamed to columnsForIsIndex1Row and columnsForHasC3Row. Not totally sure thats better :)

jelbourn · 2018-04-26T21:10:44Z

src/cdk/table/table.ts

+ */
+export interface RenderRow<T> {
+  data: T;
+  index: number;


dataIndex or renderIndex?

Renamed to dataIndex

jelbourn · 2018-04-26T21:11:48Z

src/cdk/table/table.ts

+  get multiTemplateRows(): boolean { return this._multiTemplateRows; }
+  set multiTemplateRows(v: boolean) {
+    this._multiTemplateRows = coerceBooleanProperty(v);
+    if (!!this._rowOutlet.viewContainer.length) {


!! is unnecessary in conditionals

jelbourn · 2018-04-26T21:13:51Z

src/cdk/table/table.ts

+    // respective `RenderRow` object which is the pair of `data` and `CdkRowDef`.
+    for (let i = 0; i < this._data.length; i++) {
+      let data = this._data[i];
+      const dataRowsForData = this._getRenderRowsForData(data, i, prevCachedRenderRows.get(data));


renderRowsForData

jelbourn · 2018-04-26T21:16:43Z

src/cdk/table/table.ts

-  private _insertRow(rowData: T, index: number) {
-    const rowDef = this._getRowDef(rowData, index);
-    const context: CdkCellOutletRowContext<T> = {$implicit: rowData};
+  private _insertRow(dataRow: RenderRow<T>, index: number) {


renderRow

jelbourn · 2018-04-26T21:18:31Z

src/cdk/table/row.ts

+  odd?: boolean;
+}
+
+/** Context provided to the row cells when `multiTemplateDataRows` is true */


Expand to say something like

This is the same as CdkCellOutletRowContext, except that the single `index` is replaced with `renderIndex` and `dataIndex`.

jelbourn · 2018-04-26T21:19:49Z

src/cdk/table/table.ts

+ * @docs-private
+ */
+export interface RowContext<T>
+    extends CdkCellOutletMultiRowContext<T>, CdkCellOutletRowContext<T> { }


You don't want to extend both, you want a union type

export type RowContext = CdkCellOutletMultiRowContext | CdkCellOutletRowContext;

Tried that but typescript didn't like it. Perhaps I'm doing something incorrectly?

Here's a typescript playground that shows what I tried: https://www.typescriptlang.org/play/#src=interface%20A%20%7B%0D%0A%20%20%20%20a1%3A%20number%3B%0D%0A%20%20%20%20a2%3A%20number%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20B%20%7B%0D%0A%20%20%20%20b1%3A%20number%3B%0D%0A%20%20%20%20b2%3A%20number%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20ExtendedInterface%20extends%20A%2C%20B%20%7B%20%7D%3B%0D%0Alet%20value%3A%20ExtendedInterface%3B%0D%0Avalue.a1%20%3D%201%3B%0D%0Avalue.b1%20%3D%201%3B%0D%0A%0D%0Atype%20UnionInterface%20%3D%20A%20%7C%20B%3B%0D%0Alet%20unionValue%3A%20UnionInterface%3B%0D%0AunionValue.a1%20%3D%201%3B%0D%0AunionValue.b1%20%3D%201%3B

jelbourn · 2018-04-26T21:20:26Z

src/cdk/table/table.ts

@@ -620,4 +755,11 @@ export class CdkTable<T> implements CollectionViewer, OnInit, AfterContentChecke
      this._elementRef.nativeElement.appendChild(element);
    }
  }
+
+  /** Forces a re-render of the data rows. */


Expand this comment with information about when it's appropriate to call this function?

jelbourn · 2018-04-26T21:21:02Z

src/lib/table/table.spec.ts

@@ -56,6 +56,23 @@ describe('MatTable', () => {
        ['Footer A', 'Footer B', 'Footer C'],
      ]);
    });
+
+    it('should create a table with row multiplex', () => {


~~multiplex~~

andrewseguin · 2018-04-26T21:42:29Z

Thanks, should have done a better job of renaming dataRow to renderRow. Forgot to find/replace that lowercase one version

andrewseguin · 2018-04-26T21:56:36Z

Note to caretaker, will require BUILD change to add ":cdk_coercion" to the cdk_table BUILD rule.

See change 194450307 for patch.

andrewseguin · 2018-04-27T14:29:48Z

src/cdk/table/table.ts

+
+    // For each data object, get the list of rows that should be rendered, represented by the
+    // respective `RenderRow` object which is the pair of `data` and `CdkRowDef`.
+    for (let i = 0; i < this._data.length; i++) {


Need to check/handle if empty

andrewseguin · 2018-04-27T22:44:26Z

Removing pr: needs review label. Found that having homogeneous data was not correctly being handled between diffs due to caching a RenderRow for each data object rather than per rendered row.

Writing up some tests and will add the label and a comment when ready again.

andrewseguin · 2018-04-30T21:02:35Z

Ready again for review. Added docs on the table's implementation process for rendering rows. Renamed the input to multiTemplateDataRows since the input will only affect data rows

jelbourn · 2018-04-30T22:01:29Z

src/cdk/table/table.ts

+  }
+
+  /**
+   * Returns a list of `RenderRow<T>` for the provided data object and any `CdkRowDef` objects that


Prefer "Gets" to "Returns` in descriptions

jelbourn · 2018-04-30T22:03:59Z

src/cdk/table/table.ts

   * predicate that returns true with the data. If none return true, return the default row
   * definition.
   */
-  _getRowDef(data: T, i: number): CdkRowDef<T> {
-    if (this._rowDefs.length == 1) { return this._rowDefs[0]; }
+  _getRowDefs(data: T, i: number): CdkRowDef<T>[] {


Still make this more specific than i?

jelbourn · 2018-04-30T22:07:41Z

src/cdk/table/table.spec.ts

+        ]);
+      });
+
+      it('should have the correct data and row indicies when data is homogeneous', () => {


"Homogeneous" doesn't really communicate the same thing as "Data contains multiple occurrences of the same object instance"

andrewseguin · 2018-04-30T22:35:11Z

Thanks - made the changes

jelbourn

LGTM

This reverts commit 0e466ee.

…lar#11102)

angular-automatic-lock-bot · 2019-09-08T21:02:20Z

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

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

andrewseguin requested a review from jelbourn April 23, 2018 16:24

andrewseguin added the pr: needs review label Apr 23, 2018

andrewseguin requested review from mmalerba and tinayuangao April 23, 2018 16:24

andrewseguin force-pushed the table-eval-when-each-emit branch 2 times, most recently from f3870e0 to 52834b2 Compare April 23, 2018 22:53

jelbourn reviewed Apr 26, 2018

View reviewed changes

andrewseguin force-pushed the table-eval-when-each-emit branch from 52834b2 to 9fa7f08 Compare April 26, 2018 16:46

andrewseguin removed the pr: needs review label Apr 26, 2018

andrewseguin added the pr: needs review label Apr 26, 2018

jelbourn reviewed Apr 26, 2018

View reviewed changes

andrewseguin added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 26, 2018

andrewseguin commented Apr 27, 2018

View reviewed changes

andrewseguin removed the pr: needs review label Apr 27, 2018

andrewseguin force-pushed the table-eval-when-each-emit branch 2 times, most recently from d64a8ff to 75ced9a Compare April 30, 2018 17:37

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

andrewseguin force-pushed the table-eval-when-each-emit branch from 75ced9a to fe56e56 Compare April 30, 2018 17:41

andrewseguin removed request for mmalerba and tinayuangao April 30, 2018 21:06

jelbourn reviewed Apr 30, 2018

View reviewed changes

andrewseguin added 9 commits April 30, 2018 15:32

feat(table): enable multiple data rows

75ae402

comments

1c72b4a

fix tests

b9df4c9

update

459d76c

gracefully handle null data

42e7472

handle homogeneous data; need to write tests

2f9c2b0

rename input; add docs for rendering

65f3604

remove trackby

09958b0

comments

075d680

andrewseguin force-pushed the table-eval-when-each-emit branch from 373bfa9 to 075d680 Compare April 30, 2018 22:35

adjust another test name

44cec3b

jelbourn approved these changes May 1, 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 May 1, 2018

josephperrott requested review from josephperrott and removed request for josephperrott May 1, 2018 22:34

josephperrott merged commit 0e466ee into angular:master May 2, 2018

josephperrott added a commit that referenced this pull request May 2, 2018

Revert "feat(table): enable multiple data rows (#10964)"

950dc7d

This reverts commit 0e466ee.

josephperrott mentioned this pull request May 2, 2018

Revert "feat(table): enable multiple data rows" #11102

Merged

crisbeto pushed a commit to crisbeto/material2 that referenced this pull request May 2, 2018

Revert "feat(table): enable multiple data rows (angular#10964)" (angu…

645edc5

…lar#11102)

andrewseguin mentioned this pull request May 2, 2018

feat(table): enable multiple data rows #11116

Merged

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

feat(table): enable multiple data rows #10964

feat(table): enable multiple data rows #10964

Uh oh!

Conversation

andrewseguin commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Apr 26, 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!

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!

andrewseguin commented Apr 26, 2018

Uh oh!

andrewseguin commented Apr 26, 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!

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!

Choose a reason for hiding this comment

Uh oh!

andrewseguin commented Apr 26, 2018

Uh oh!

andrewseguin commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewseguin commented Apr 27, 2018

Uh oh!

andrewseguin commented Apr 30, 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!

andrewseguin commented Apr 30, 2018

andrewseguin commented Apr 23, 2018 •

edited

Loading