Skip to content

feat(schematics): table schematic #10012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 8, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions schematics/collection.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
{
"$schema": "./node_modules/@angular-devkit/schematics/collection-schema.json",
"schematics": {
// Creates a table component
"materialTable": {
"description": "Create a table component",
"factory": "./table/index",
"schema": "./table/schema.json",
"aliases": [ "material-table" ]
},
// Adds Angular Material to an application without changing any templates
"materialShell": {
"description": "Create a Material shell",
Expand Down
5 changes: 5 additions & 0 deletions schematics/table/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Table
Creates a table component with sorting and paginator.

Command: `ng generate material-table --collection=material-schematics`

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<div class="mat-elevation-z8">
<mat-table #table [dataSource]="dataSource" matSort aria-label="Elements">

<!-- 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</ng-container>

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

<mat-header-row *matHeaderRowDef="displayedColumns"></mat-header-row>
<mat-row *matRowDef="let row; columns: displayedColumns;"></mat-row>
</mat-table>

<mat-paginator #paginator
[length]="dataSource.data.length"
[pageIndex]="0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length should come from the data, no?

[pageSize]="10"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[pageSizeOptions]="[25, 50, 100, 250]">
</mat-paginator>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@

import { fakeAsync, ComponentFixture, TestBed } from '@angular/core/testing';

import { <%= classify(name) %>Component } from './<%= dasherize(name) %>.component';

describe('<%= classify(name) %>Component', () => {
let component: <%= classify(name) %>Component;
let fixture: ComponentFixture<<%= classify(name) %>Component>;

beforeEach(fakeAsync(() => {
TestBed.configureTestingModule({
declarations: [ <%= classify(name) %>Component ]
})
.compileComponents();

fixture = TestBed.createComponent(<%= classify(name) %>Component);
component = fixture.componentInstance;
fixture.detectChanges();
});

it('should compile', () => {
expect(component).toBeTruthy();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import { Component, OnInit, ViewChild, <% if(!!viewEncapsulation) { %>, ViewEncapsulation<% }%><% if(changeDetection !== 'Default') { %>, ChangeDetectionStrategy<% }%> } from '@angular/core';
import { Observable } from 'rxjs/Observable';
import { map } from 'rxjs/operators/map';
import { MatPaginator, MatSort } from '@angular/material';
import { DataSource } from '@angular/cdk/collections';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have the spaces around the curly braces here? We don't have them in our source.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default schematics have them so I'd tend to stay the same for consistency. @jelbourn - what do u think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { of } from 'rxjs/observable/of';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

observableOf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Since of as a function is confusing with for of

import { merge } from 'rxjs/observable/merge';

/** Todo: Replace this with your own data model type */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO (all caps)

export interface DataItem {
name: string;
id: number;
}

function compare(a, b, isAsc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this name be more specific? sortComparator? Should also have a comment like

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

I would also move this to the bottom of the file.

return (a < b ? -1 : 1) * (isAsc ? 1 : -1);
}

export class ExampleDataSource extends DataSource<any> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data source should go in its own file. I'd like this data source to play a big role in guiding users in structuring their data access and manipulation.

It would be great if we could add an option for a client data-source vs. a server data source. The client one would perform everything locally and synchronously, while the server one would call out to (fake) server methods and do thing asynchronously.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "Example", could we use the name of the page that's being generated?

data: DataItem[] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this array to be a constant at the top like EXAMPLE_DATA, and then when it's assigned in the class it should have a comment like

// TODO: replace this with real data from your application

{id: 1, name: 'Hydrogen'},
{id: 2, name: 'Helium'},
{id: 3, name: 'Lithium'},
{id: 4, name: 'Beryllium'},
{id: 5, name: 'Boron'},
{id: 6, name: 'Carbon'},
{id: 7, name: 'Nitrogen'},
{id: 8, name: 'Oxygen'},
{id: 9, name: 'Fluorine'},
{id: 10, name: 'Neon'},
{id: 11, name: 'Sodium'},
{id: 12, name: 'Magnesium'},
{id: 13, name: 'Aluminum'},
{id: 14, name: 'Silicon'},
{id: 15, name: 'Phosphorus'},
{id: 16, name: 'Sulfur'},
{id: 17, name: 'Chlorine'},
{id: 18, name: 'Argon'},
{id: 19, name: 'Potassium'},
{id: 20, name: 'Calcium'},
];

constructor(private _paginator: MatPaginator, private _sort: MatSort) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
});

super();
}

connect(): Observable<Element[]> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a method description like

/** 
 * 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.
 */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return value should be Observable<DataItem>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Combine everything that affects the rendered data into one update
// stream for the data-table to consume.
const dataMutations = [
of(this.data),
this._paginator.page,
this._sort.sortChange
];

return merge(...dataMutations).pipe(map(() => {
return this.getPagedData(this.getSortedData(this.data));
}));
}

disconnect() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment inside the empty disconnect like

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


private getPagedData(data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add method description

const startIndex = this._paginator.pageIndex * this._paginator.pageSize;
return data.splice(startIndex, this._paginator.pageSize);
}

private getSortedData(data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add method description

if (!this._sort.active || this._sort.direction === '') {
return data;
}

return data.sort((a, b) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const isAsc = this._sort.direction == 'asc';
switch (this._sort.active) {
case 'name': return compare(a.name, b.name, isAsc);
case 'id': return compare(+a.id, +b.id, isAsc);
default: return 0;
}
});
}
}

@Component({
selector: '<%= selector %>',<% if(inlineTemplate) { %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hansl are there any plans to make it easier to do templateUrl vs inline template without duplicating?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be done manually, ie. put the template content in a variable that’s used both here and in the separate file. Read that value from a file on disk. There’s no silver bullet for this one.

template: `
<div class="mat-elevation-z8">
<mat-table #table [dataSource]="dataSource" matSort aria-label="Elements">

<!-- 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>
</ng-container>

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

<mat-header-row *matHeaderRowDef="displayedColumns"></mat-header-row>
<mat-row *matRowDef="let row; columns: displayedColumns;"></mat-row>
</mat-table>

<mat-paginator #paginator
[length]="dataSource.data.length"
[pageIndex]="0"
[pageSize]="10"
[pageSizeOptions]="[25, 50, 100, 250]">
</mat-paginator>
</div>
`,<% } else { %>
templateUrl: './<%= dasherize(name) %>.component.html',<% } if(inlineStyle) { %>
styles: []<% } else { %>
styleUrls: ['./<%= dasherize(name) %>.component.<%= styleext %>']<% } %><% if(!!viewEncapsulation) { %>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the !! necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't mean it shouldn't be corrected.

encapsulation: ViewEncapsulation.<%= viewEncapsulation %><% } if (changeDetection !== 'Default') { %>,
changeDetection: ChangeDetectionStrategy.<%= changeDetection %><% } %>
})
export class <%= classify(name) %>Component implements OnInit {
@ViewChild(MatPaginator) paginator: MatPaginator;
@ViewChild(MatSort) sort: MatSort;
dataSource: ExampleDataSource;

/** Columns displayed in the table. Columns IDs can be added, removed, or reordered. */
displayedColumns = ['id', 'name'];

ngOnInit() {
this.dataSource = new ExampleDataSource(this.paginator, this.sort);
}
}
29 changes: 29 additions & 0 deletions schematics/table/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import {chain, Rule, noop, Tree, SchematicContext} from '@angular-devkit/schematics';
import {Schema} from './schema';
import {addModuleImportToModule} from '../utils/ast';
import {findModuleFromOptions} from '../utils/devkit-utils/find-module';
import {buildComponent} from '../utils/devkit-utils/component';

/**
* Scaffolds a new table component.
* Internally it bootstraps the base component schematic
*/
export default function(options: Schema): Rule {
return chain([
buildComponent({...options}),
options.skipImport ? noop() : addTableModulesToModule(options)
]);
}

/**
* Adds the required modules to the relative module.
*/
function addTableModulesToModule(options: Schema) {
return (host: Tree) => {
const modulePath = findModuleFromOptions(host, options);
addModuleImportToModule(host, modulePath, 'MatTableModule', '@angular/material');
addModuleImportToModule(host, modulePath, 'MatPaginatorModule', '@angular/material');
addModuleImportToModule(host, modulePath, 'MatSortModule', '@angular/material');
return host;
};
}
56 changes: 56 additions & 0 deletions schematics/table/index_spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
import {join} from 'path';
import {Tree} from '@angular-devkit/schematics';
import {createTestApp} from '../utils/testing';
import {getFileContent} from '@schematics/angular/utility/test';

const collectionPath = join(__dirname, '../collection.json');

describe('material-table-schematic', () => {
let runner: SchematicTestRunner;
const options = {
name: 'foo',
path: 'app',
sourceDir: 'src',
inlineStyle: false,
inlineTemplate: false,
changeDetection: 'Default',
styleext: 'css',
spec: true,
module: undefined,
export: false,
prefix: undefined,
viewEncapsulation: undefined,
};

beforeEach(() => {
runner = new SchematicTestRunner('schematics', collectionPath);
});

it('should create table files and add them to module', () => {
const tree = runner.runSchematic('materialTable', { ...options }, createTestApp());
const files = tree.files;

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

const moduleContent = getFileContent(tree, '/src/app/app.module.ts');
expect(moduleContent).toMatch(/import.*Foo.*from '.\/foo\/foo.component'/);
expect(moduleContent).toMatch(/declarations:\s*\[[^\]]+?,\r?\n\s+FooComponent\r?\n/m);
});

it('should add table imports to module', () => {
const tree = runner.runSchematic('materialTable', { ...options }, createTestApp());
const moduleContent = getFileContent(tree, '/src/app/app.module.ts');

expect(moduleContent).toContain('MatTableModule');
expect(moduleContent).toContain('MatPaginatorModule');
expect(moduleContent).toContain('MatSortModule');

expect(moduleContent).toContain(
`import { MatTableModule, MatPaginatorModule, MatSortModule } from '@angular/material';`);
});

});
3 changes: 3 additions & 0 deletions schematics/table/schema.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import {Schema as ComponentSchema} from '@schematics/angular/component/schema';

export interface Schema extends ComponentSchema {}
98 changes: 98 additions & 0 deletions schematics/table/schema.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
{
"$schema": "http://json-schema.org/schema",
"id": "SchematicsMaterialTable",
"title": "Material Table Options Schema",
"type": "object",
"properties": {
"path": {
"type": "string",
"description": "The path to create the component.",
"default": "app",
"visible": false
},
"sourceDir": {
"type": "string",
"description": "The path of the source directory.",
"default": "src",
"alias": "sd",
"visible": false
},
"appRoot": {
"type": "string",
"description": "The root of the application.",
"visible": false
},
"name": {
"type": "string",
"description": "The name of the component."
},
"inlineStyle": {
"description": "Specifies if the style will be in the ts file.",
"type": "boolean",
"default": false,
"alias": "is"
},
"inlineTemplate": {
"description": "Specifies if the template will be in the ts file.",
"type": "boolean",
"default": false,
"alias": "it"
},
"viewEncapsulation": {
"description": "Specifies the view encapsulation strategy.",
"enum": ["Emulated", "Native", "None"],
"type": "string",
"alias": "ve"
},
"changeDetection": {
"description": "Specifies the change detection strategy.",
"enum": ["Default", "OnPush"],
"type": "string",
"default": "Default",
"alias": "cd"
},
"prefix": {
"type": "string",
"description": "The prefix to apply to generated selectors.",
"default": "app",
"alias": "p"
},
"styleext": {
"description": "The file extension to be used for style files.",
"type": "string",
"default": "css"
},
"spec": {
"type": "boolean",
"description": "Specifies if a spec file is generated.",
"default": true
},
"flat": {
"type": "boolean",
"description": "Flag to indicate if a dir is created.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from the base component schematic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"default": false
},
"skipImport": {
"type": "boolean",
"description": "Flag to skip the module import.",
"default": false
},
"selector": {
"type": "string",
"description": "The selector to use for the component."
},
"module": {
"type": "string",
"description": "Allows specification of the declaring module.",
"alias": "m"
},
"export": {
"type": "boolean",
"default": false,
"description": "Specifies if declaring module exports the component."
}
},
"required": [
"name"
]
}
Loading