feat(data-table): re-render when columns change #4830

andrewseguin · 2017-05-26T19:46:56Z

No description provided.

ErinCoughlan · 2017-05-26T20:08:43Z

src/lib/core/data-table/data-table.spec.ts

+    // Remove column_a and swap column_b/column_c.
+    component.columnsToRender = ['column_c', 'column_b'];
+    fixture.detectChanges();
+    fixture.detectChanges();


Why do we need two detectChanges? Add a comment?

Good question, looks like whatever was needing it before is gone. Just need one now

jelbourn · 2017-05-26T20:43:11Z

src/lib/core/data-table/data-table.ts

  /**
   * Stream containing the latest information on what rows are being displayed on screen.
   * Can be used by the data source to as a heuristic of what data should be provided.
   */
  viewChanged =
      new BehaviorSubject<{start: number, end: number}>({start: 0, end: Number.MAX_VALUE});

+  /** Stream that emits when a row def has a change to its array of columns to render. */
+  columnsChange = new Observable<void[]>();


Name should probably be consistent w/ viewChanged; maybe change viewChanged to just viewChange?

jelbourn · 2017-05-26T20:46:39Z

src/lib/core/data-table/data-table.ts

@@ -112,21 +116,32 @@ export class CdkTable implements CollectionViewer {
    this._columnDefinitions.forEach(columnDef => {
      this._columnDefinitionsByName.set(columnDef.name, columnDef);
    });
+
+    // Get and merge the streams for column changes made to the row defs
+    const rowDefs = this._rowDefinitions.toArray().concat(this._headerDefinition);


Expert mode:

const rowDefs = [...this._rowDefinitions.toArray(), this._headerDefinition];

jelbourn · 2017-05-26T20:49:18Z

src/lib/core/data-table/data-table.ts

-    this.dataSource.connect(this).subscribe((rowsData: any[]) => {
+    const streams = [this.dataSource.connect(this), this.columnsChange];
+    Observable.combineLatest(streams).subscribe((result: any[]) => {
+      console.log('Rendering all rows');


console.log

We can pretend this didn't make it through to review

jelbourn · 2017-05-26T20:50:17Z

src/lib/core/data-table/data-table.ts

    // TODO(andrewseguin): If the data source is not
    //   present after view init, connect it when it is defined.
    // TODO(andrewseguin): Unsubscribe from this on destroy.
-    this.dataSource.connect(this).subscribe((rowsData: any[]) => {
+    const streams = [this.dataSource.connect(this), this.columnsChange];
+    Observable.combineLatest(streams).subscribe((result: any[]) => {


Can this be more specific than any?

I don't think it can be since I cannot type the individual elements of the array.

jelbourn · 2017-05-26T20:50:57Z

src/lib/core/data-table/data-table.ts

    // TODO(andrewseguin): If the data source is not
    //   present after view init, connect it when it is defined.
    // TODO(andrewseguin): Unsubscribe from this on destroy.
-    this.dataSource.connect(this).subscribe((rowsData: any[]) => {
+    const streams = [this.dataSource.connect(this), this.columnsChange];


More specific name than streams?

Going with renderChange in that it causes render changes? Naming is hard

ErinCoughlan

Suggestion for future/best practice, otherwise, LGTM

ErinCoughlan · 2017-05-30T19:01:41Z

src/demo-app/data-table/data-table-demo.ts

+  toggleColorColumn() {
+    let colorColumnIndex = this.propertiesToDisplay.findIndex((col: string) => col === 'color');
+    if (colorColumnIndex == -1) {
+      this.propertiesToDisplay.push('color');


Possibly later, but I'm always cautious about passing strings around because I'm great at mistyping them. Perhaps a best practice would be to define a columns type for the demo:

type columns = 'color'|'progress'; // etc.

And then we'll have a little bit of type safety with the propertiesToDisplay. It would also make us less likely to hit the no column definition error.

I think that sounds good, I'll add that in.

jelbourn · 2017-05-31T19:34:47Z

src/lib/core/data-table/data-table.spec.ts

+
+    let initialTableContent = [['Column A', 'Column B', 'Column C']];
+    dataSource.data.forEach(rowData => initialTableContent.push([rowData.a, rowData.b, rowData.c]));
+    expect(tableElement).toMatchTableContent(initialTableContent);


You typically want to avoid having logic in unit tests like this, instead preferring to hard-code the values. I think this all should be

expect(tableElement).toMatchTableContent([ ['Column A', 'Column B', 'Column C'], [data[0].a, data[0].b, data[0].c], [data[1].a, data[1].b, data[2].c], [data[2].a, data[2].b, data[1].c], ]);

Which is also much easier to visually see what's going on with the test

jelbourn · 2017-05-31T19:42:55Z

src/lib/core/data-table/data-table.ts

    // TODO(andrewseguin): If the data source is not
    //   present after view init, connect it when it is defined.
    // TODO(andrewseguin): Unsubscribe from this on destroy.
-    this.dataSource.connect(this).subscribe((rowsData: any[]) => {
+    const renderChange = [this.dataSource.connect(this), this.columnsChange];
+    Observable.combineLatest(renderChange).subscribe(([data]) => {


Why are you destructuring the function arguments here?

The only part of the observable we want is the first element in the array result, so just grabbing it here. If it looks too odd I can change it back to (result: any[]) and then grab result[0] for our data.

jelbourn · 2017-05-31T19:44:28Z

src/lib/core/data-table/data-table.ts

      new BehaviorSubject<{start: number, end: number}>({start: 0, end: Number.MAX_VALUE});

+  /** Stream that emits when a row def has a change to its array of columns to render. */
+  columnsChange = new Observable<void[]>();


void[] doesn't make sense- it's "array of nothings". It should either be just void or capture whatever argument is being emitted

I agree but it makes Typescript happy. The merged streams are each Observable and combined make up Observable<void[]>

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-07T19:02:46Z

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 assigned DerekLouie, jelbourn and ErinCoughlan May 26, 2017

andrewseguin requested review from DerekLouie, jelbourn and ErinCoughlan May 26, 2017 19:46

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 26, 2017

andrewseguin added the pr: needs review label May 26, 2017

ErinCoughlan reviewed May 26, 2017

View reviewed changes

jelbourn reviewed May 26, 2017

View reviewed changes

andrewseguin force-pushed the table-dynamic-columns branch from 3d9a7a3 to 8327ecd Compare May 30, 2017 17:32

ErinCoughlan approved these changes May 30, 2017

View reviewed changes

DerekLouie approved these changes May 31, 2017

View reviewed changes

jelbourn reviewed May 31, 2017

View reviewed changes

andrewseguin force-pushed the table-dynamic-columns branch from 2ceeb69 to 1904c26 Compare June 1, 2017 22:08

jelbourn approved these changes Jun 5, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 5, 2017

andrewseguin added 7 commits June 5, 2017 14:59

feat(data-table): re-render when columns change

835c748

minor changes

f4805fc

comment stuff

389713f

add column type

fcb495c

explicit testing:

e8e92d8

fix observable type

8a724b9

sync

ac23efa

andrewseguin force-pushed the table-dynamic-columns branch from 2ed6b9c to ac23efa Compare June 6, 2017 18:36

andrewseguin added 2 commits June 6, 2017 11:37

remove fdescribe

250da54

fix test

31464a7

andrewseguin merged commit e81619b into angular:master Jun 7, 2017

andrewseguin deleted the table-dynamic-columns branch November 28, 2017 20:35

angular-automatic-lock-bot bot unassigned DerekLouie Sep 7, 2019

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

feat(data-table): re-render when columns change #4830

feat(data-table): re-render when columns change #4830

Uh oh!

Conversation

andrewseguin commented May 26, 2017

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!

ErinCoughlan 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!

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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!