feat(schematics): table schematic #10012

amcdnl · 2018-02-18T17:33:04Z

This PR creates a table schematic.

Source: https://github.com/amcdnl/material-example-schematics/tree/schematic-table

jelbourn · 2018-02-18T19:48:36Z

schematics/table/README.md

@@ -0,0 +1,5 @@
+ # Table
+ Creates a table component with sorting and pager.


"pager" -> "paginator"

jelbourn · 2018-02-18T19:53:48Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

+<div class="mat-elevation-z8">
+  <mat-table #table [dataSource]="dataSource" matSort>
+
+    <!-- Position Column -->


I think we should just do two columns (ID and Name), since they're pretty universal. This should be less of an example of how to use the table and more of a seed to add onto, so the less someone would have to change/delete the better

jelbourn · 2018-02-18T19:54:38Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

@@ -0,0 +1,38 @@
+<div class="mat-elevation-z8">
+  <mat-table #table [dataSource]="dataSource" matSort>


We should ensure that the a11y for this template is really good, including placeholder aria labels where appropriate (I think the mat-table element likely needs one)

jelbourn · 2018-02-18T19:55:56Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

+
+  <mat-paginator #paginator
+    [length]="20"
+    [pageIndex]="0"


The length should come from the data, no?

jelbourn · 2018-02-18T19:57:47Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

+    [length]="20"
+    [pageIndex]="0"
+    [pageSize]="10"
+    [pageSizeOptions]="[5, 10, 25, 100]">


I'd make the sizes [25, 50, 100, 250], which feels more realistic.

Do we really want to encourage sometime to load 250 into view?

jelbourn · 2018-02-18T20:19:02Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+  `,<% } else { %>
+  templateUrl: './<%= dasherize(name) %>.component.html',<% } if(inlineStyle) { %>
+  styles: []<% } else { %>
+  styleUrls: ['./<%= dasherize(name) %>.component.<%= styleext %>']<% } %><% if(!!viewEncapsulation) { %>,


Is the !! necessary?

I didn't write this, I copied the code from the default component template for this section.

Doesn't mean it shouldn't be corrected.

jelbourn · 2018-02-18T20:20:52Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+export class <%= classify(name) %>Component implements OnInit {
+  @ViewChild(MatPaginator) paginator: MatPaginator;
+  @ViewChild(MatSort) sort: MatSort;
+  displayedColumns = ['position', 'name', 'weight', 'symbol'];


Add a description to this like

/** Columns displayed in the table. Columns IDs can be added, removed, or reordered. */

jelbourn · 2018-02-18T20:22:44Z

schematics/table/index.ts

+/**
+ * Adds the required modules to the relative module.
+ */
+function addNavModulesToModule(options: Schema) {


Nav Modules?

jelbourn · 2018-02-18T20:23:13Z

schematics/table/index.ts

+ */
+export default function(options: Schema): Rule {
+  return chain([
+    buildComponent({ ...options }),


Omit spaces in braces.

jelbourn · 2018-02-18T20:24:39Z

schematics/table/index_spec.ts

+    const tree = runner.runSchematic('materialTable', { ...options }, createTestApp());
+    const files = tree.files;
+
+    expect(files.indexOf('/src/app/foo/foo.component.css')).toBeGreaterThanOrEqual(0);


You should be able to use the toContain matcher:

expect(files).toContain('/src/app/foo/foo.component.css'); ...

crisbeto · 2018-02-19T18:59:58Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

+      <mat-cell *matCellDef="let element">{{element.weight}}</mat-cell>
+    </ng-container>
+
+    <!-- Color Column -->


The comment says "color", but the column is "symbol".

crisbeto · 2018-02-19T19:00:46Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.spec.ts

+  it('should create', () => {
+    expect(component).toBeTruthy();
+  });
+});


Missing a newline at the end of the file.

crisbeto · 2018-02-19T19:01:25Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.spec.ts

+  let component: <%= classify(name) %>Component;
+  let fixture: ComponentFixture<<%= classify(name) %>Component>;
+
+  beforeEach(async(() => {


Maybe this should be fakeAsync, instead of async? We've moved over most of our components to be fakeAsync.

This was copied from the base component schematic.

crisbeto · 2018-02-19T19:02:18Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.spec.ts

+    fixture.detectChanges();
+  });
+
+  it('should create', () => {


The test name sounds off. Maybe something along the lines of should compile?

This was copied from the base component schematic.

crisbeto · 2018-02-19T19:02:38Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.spec.ts

+    .compileComponents();
+  }));
+
+  beforeEach(() => {


You can combine these two beforeEach calls into one.

This was copied from the base component schematic.

crisbeto · 2018-02-19T19:04:52Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+    }));
+  }
+
+  disconnect() {}


Maybe there should be an example of what cleaning up here would look like, or a comment that says something like "Don't forget to clean up!".

crisbeto · 2018-02-19T19:06:42Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+
+  getSortedData(data) {
+    if (!this._sort.active || this._sort.direction === '') { return data; }
+    return data.sort((a, b) => {


nit: missing an extra space above the return. Also switching the if to multiple lines.

crisbeto · 2018-02-19T19:07:58Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+    return data.sort((a, b) => {
+      const propertyA = a[this._sort.active];
+      const propertyB = b[this._sort.active];
+      const valueA = isNaN(+propertyA) ? propertyA : +propertyA;


Since these are also supposed to serve as an example, it might be more appropriate to use something more readable like parseFloat(property) || 0.

Refactored to different implementation.

crisbeto · 2018-02-19T19:08:31Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+          <mat-cell *matCellDef="let element">{{element.weight}}</mat-cell>
+        </ng-container>
+
+        <!-- Color Column -->


The column is actually "symbol".

Changed to different property.

crisbeto · 2018-02-19T19:10:45Z

schematics/table/schema.json

+    },
+    "flat": {
+      "type": "boolean",
+      "description": "Flag to indicate if a dir is created.",


I don't think it's clear what a dir means in this case.

This was copied from the base component schematic.

Doesn't mean we can't make it better for ourselves

I agree but I wanted to keep this as close to the original as possible so when they support JSON schema inheritance, we can just do away with this file completely.

# Conflicts: # schematics/collection.json # schematics/tsconfig.json

amcdnl · 2018-02-28T02:18:40Z

@jelbourn - Addressed all feedback minus the client/server datasource examples. Will address that in another round.

jelbourn · 2018-02-28T14:39:23Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+  `,<% } else { %>
+  templateUrl: './<%= dasherize(name) %>.component.html',<% } if(inlineStyle) { %>
+  styles: []<% } else { %>
+  styleUrls: ['./<%= dasherize(name) %>.component.<%= styleext %>']<% } %><% if(!!viewEncapsulation) { %>,


Doesn't mean it shouldn't be corrected.

jelbourn · 2018-02-28T14:40:02Z

schematics/table/schema.json

+    },
+    "flat": {
+      "type": "boolean",
+      "description": "Flag to indicate if a dir is created.",


Doesn't mean we can't make it better for ourselves

jelbourn · 2018-02-28T14:42:18Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

+  <mat-paginator #paginator
+    [length]="dataSource.data.length"
+    [pageIndex]="0"
+    [pageSize]="10"


Compromise on 50? 10 is just too small to ever be realistic.

jelbourn · 2018-02-28T14:43:18Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+import { Observable } from 'rxjs/Observable';
+import { map } from 'rxjs/operators/map';
+import { MatPaginator, MatSort } from '@angular/material';
+import { DataSource } from '@angular/cdk/collections';


Yeah, we should be consistent with the rest of the CLI even if spaces in braces is nonsense.

jelbourn · 2018-02-28T14:44:06Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+import { map } from 'rxjs/operators/map';
+import { MatPaginator, MatSort } from '@angular/material';
+import { DataSource } from '@angular/cdk/collections';
+import { of } from 'rxjs/observable/of';


import {of as observableOf} from 'rxjs/observable/of';

Since of as a function is confusing with for of

jelbourn · 2018-02-28T14:52:18Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+    }));
+  }
+
+  disconnect() {}


Add a comment inside the empty disconnect like

// TODO: clean up any open connections, free any held resources, etc.

jelbourn · 2018-02-28T14:52:42Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+
+  disconnect() {}
+
+  private getPagedData(data) {


Add method description

jelbourn · 2018-02-28T14:52:47Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+    return data.splice(startIndex, this._paginator.pageSize);
+  }
+
+  private getSortedData(data) {


Add method description

jelbourn · 2018-02-28T14:52:58Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+    super();
+  }
+
+  connect(): Observable<Element[]> {


Return value should be Observable<DataItem>

I did one better and did: <%= classify(name) %>Item[]

jelbourn · 2018-02-28T15:01:04Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.ts

+    {id: 20, name: 'Calcium'},
+  ];
+
+  constructor(private _paginator: MatPaginator, private _sort: MatSort) {


I've been thinking about this, and I'm thinking it would be good to decouple the data source and the sorter/paginator. This would look something like the data source having its own sort and pagination state, and then using the outputs on the directives to update the data source state.

My main reason for wanting to do this is to not make users think they must use those components.

Ya, I can see that but it wouldn't be near as clean then. What about a comment instead? Best way I could think of would be to:

this.dataSource = new DataSource({ pageIndex: this.paginator.pageIndex, pageSize: this.paginator.pageSize }, { active: this.sort.active, direction: this.sort.direction });

amcdnl · 2018-03-04T16:44:48Z

@jelbourn - Addressed feedback.

jelbourn · 2018-03-06T00:19:47Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+import { map } from 'rxjs/operators/map';
+import { Observable } from 'rxjs/Observable';
+
+/** TODO: Replace this with your own data model type */


Switch to // style comments to be consistent with other TODOs

jelbourn · 2018-03-06T00:21:58Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+
+  /**
+   * Client-side page the data by slicing out the next from the data array.
+   * If you are using external datasource for pagination, you would connect it here.


I'd rephrase this as:

Paginate the data (client-side). If you're using server-side pagination, this would be replaced by requesting the appropriate data from the server.

jelbourn · 2018-03-06T00:22:33Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+
+  /**
+   * Client-side sort the data array.
+   * If you are using a external datasource for sorting, you would connect it here


I'd rephrase this as:

Sort the data (client-side). If you're using server-side sorting, this would be replaced by requesting the appropriate data from the server.

jelbourn · 2018-03-06T00:23:07Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+  }
+}
+
+/** Simple sort comparator for example ID/Name columns. */


/** Simple sort comparator for example ID/Name columns (for client-side sorting). */

jelbourn · 2018-03-06T00:29:42Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+  {id: 20, name: 'Calcium'},
+];
+
+export class <%= classify(name) %>DataSource extends DataSource<<%= classify(name) %>Item> {


Add a class description like

Data source for the xxx view. This class should encapsulate all logic for fetching and manipulating the displayed data (including sorting, pagination, and filtering).

andrewseguin

Pretty cool stuff, I like that we can use schematics for this case

andrewseguin · 2018-03-06T21:28:31Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+export class <%= classify(name) %>DataSource extends DataSource<<%= classify(name) %>Item> {
+  data: <%= classify(name) %>Item[] = EXAMPLE_DATA;
+
+  constructor(private _paginator: MatPaginator, private _sort: MatSort) {


Though we use underscores for private variables in Material, I believe in this case we should follow the standard Angular-way:

Avoid prefixing private properties and methods with an underscore.
https://angular.io/guide/styleguide#style-03-04

andrewseguin · 2018-03-06T21:30:12Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+  /**
+   * Connect this data source to the table. The table will only update when
+   * the returned stream emits new items.
+   * @returns A stream of the items to be rendered.


Do other schematics use jsdoc format (e.g. @returns)?

andrewseguin · 2018-03-06T21:33:54Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__-datasource.ts

+  }
+
+  disconnect() {
+    // TODO: clean up any open connections, free any held resources, etc.


This TODO would make me believe I must do something here. Rather than TODO, maybe leave some jsdoc for the function.

/** * Called when the table is being destroyed. Use this function to clean up any open connections or free * any held resources that were set up during connect. */ disconnect() { }

andrewseguin · 2018-03-06T21:36:44Z

schematics/table/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html

+    <!-- Id Column -->
+    <ng-container matColumnDef="id">
+      <mat-header-cell *matHeaderCellDef mat-sort-header>Id</mat-header-cell>
+      <mat-cell *matCellDef="let row">{{row.id}}</mat-cell>


Can we make it easier for users to know what can be renamed (e.g. row)? Perhaps calling the data element instead of row?

I like row better because its a bit more generic. First thing I'm gonna do when I download this is rename that, if its row I'm less likely to want to do this.

amcdnl · 2018-03-08T00:28:38Z

@jelbourn - Ready for another review.

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-08T11:02:11Z

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

feat(schematics): table schematic

de5177d

amcdnl self-assigned this Feb 18, 2018

amcdnl requested a review from jelbourn February 18, 2018 17:33

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

jelbourn requested review from crisbeto and andrewseguin February 18, 2018 19:37

jelbourn reviewed Feb 18, 2018

View reviewed changes

crisbeto reviewed Feb 19, 2018

View reviewed changes

amcdnl added 2 commits February 27, 2018 20:15

chore: nit

4fe9892

Merge branch 'master' into table-schematic

13fbd00

# Conflicts: # schematics/collection.json # schematics/tsconfig.json

jelbourn reviewed Feb 28, 2018

View reviewed changes

chore: nit

4801ed9

jelbourn reviewed Mar 6, 2018

View reviewed changes

andrewseguin reviewed Mar 6, 2018

View reviewed changes

chore: pr feedback

42b2923

Merge branch 'master' into table-schematic

dbaf5a9

jelbourn approved these changes Mar 8, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Mar 8, 2018

jelbourn merged commit e7533a5 into angular:master Mar 8, 2018

jelbourn added the target: major This PR is targeted for the next major release label Mar 8, 2018

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

		@@ -0,0 +1,5 @@
		# Table
		Creates a table component with sorting and pager.

		@@ -0,0 +1,38 @@
		<div class="mat-elevation-z8">
		<mat-table #table [dataSource]="dataSource" matSort>

feat(schematics): table schematic #10012

feat(schematics): table schematic #10012

Uh oh!

Conversation

amcdnl commented Feb 18, 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!

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!

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!

amcdnl commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

amcdnl commented Feb 28, 2018 •

edited

Loading