feat(schematics): navigation schematic #10009

amcdnl · 2018-02-18T17:09:07Z

This PR creates a navigation/toolbar schematic.

Large Screen

Small Screen

jelbourn · 2018-02-18T21:01:38Z

...tics/nav/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.__styleext__

+
+mat-drawer {
+  width: 200px;
+  box-shadow: 3px 0 6px rgba(0,0,0,.24);


Should this be done with one of the elevation classes instead?

I thought about using the elevation helper but that would only work if you are using SCSS. We have to assume CSS/inline css/etc.

jelbourn · 2018-02-18T21:02:34Z

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

@@ -0,0 +1,18 @@
+<mat-drawer-container>
+  <mat-drawer mode="side" #drawer [opened]="!isHandset">


I believe this should be a mat-sidenav since it's at the top level. @mmalerba will know best

yep, if you make it a mat-sidenav you can also use the fixedInViewport option to make the sidenav fixed position on mobile and allow the address bar to scroll away. This is something I think we should include by default for users, or at least give an option to easily enable it, because it can be a little tricky to set up on your own.

jelbourn · 2018-02-18T21:03:27Z

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

@@ -0,0 +1,18 @@
+<mat-drawer-container>
+  <mat-drawer mode="side" #drawer [opened]="!isHandset">


Should the mode change based on the screen size? I.e., when it gets large enough it goes to over.

jelbourn · 2018-02-18T21:05:02Z

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

+  <mat-drawer-content>
+    <mat-toolbar color="primary">
+      <button type="button" mat-icon-button (click)="drawer.toggle()" *ngIf="isHandset">
+        <mat-icon aria-label="Side nav toggle icon">menu</mat-icon>


The aria-label should go on the button instead of the icon and should describe the button's action rather than describe the icon.

jelbourn · 2018-02-18T21:13:49Z

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

@@ -0,0 +1,18 @@
+<mat-drawer-container>
+  <mat-drawer mode="side" #drawer [opened]="!isHandset">
+    <mat-toolbar color="primary">Menu</mat-toolbar>


Does the sidenav need a toolbar / header? Not sure if I typically see this.

I have seen this on Google products before, they will often put the product logo in a toolbar in the sidenav so it lines ups with the main toolbar when the sidenav opens. Google photos and play music are currently doing this, but it seems like most products are switching to the inbox style where the sidenav just opens below the toolbar

@jelbourn - Let me know what you'd like me to do here.

jelbourn · 2018-02-18T21:18:55Z

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

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


It would be good to flesh out a couple of basic tests here (e.g. sidenav toggling)

jelbourn · 2018-02-18T21:19:22Z

schematics/nav/index_spec.ts

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


Should be able to use toContain here

jelbourn · 2018-02-18T21:22:07Z

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

+export class <%= classify(name) %>Component {
+  isHandset: boolean;
+  constructor(breakpointObserver: BreakpointObserver) {
+    this.isHandset = breakpointObserver.isMatched(Breakpoints.Handset);


Instead of just setting this once we should subscribe to the observer to keep it up-to-date.

This is a good case for the async pipe. See how it was done for the tooltip: https://github.com/angular/material2/blob/master/src/lib/tooltip/tooltip.html#L3

crisbeto · 2018-02-19T19:20:03Z

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

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


The two beforeEach calls can be combined.

This code was copied from the base create component schematic.

crisbeto · 2018-02-19T19:20:16Z

schematics/nav/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:20:27Z

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

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


should create -> should compile

This code was copied from the base create component schematic.

crisbeto · 2018-02-19T19:20:39Z

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

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


Use fakeAsync instead of async.

This code was copied from the base create component schematic.

crisbeto · 2018-02-19T19:20:59Z

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

+      <mat-drawer-content>
+        <mat-toolbar color="primary">
+          <button type="button" mat-icon-button (click)="drawer.toggle()" *ngIf="isHandset">
+            <mat-icon aria-label="Side nav toggle icon">menu</mat-icon>


aria-label should be on the button.

crisbeto · 2018-02-19T19:22:03Z

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

+export class <%= classify(name) %>Component {
+  isHandset: boolean;
+  constructor(breakpointObserver: BreakpointObserver) {
+    this.isHandset = breakpointObserver.isMatched(Breakpoints.Handset);


This is a good case for the async pipe. See how it was done for the tooltip: https://github.com/angular/material2/blob/master/src/lib/tooltip/tooltip.html#L3

crisbeto · 2018-02-19T19:22:35Z

schematics/nav/schema.json

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


Unclear what "dir" it's referring to.

This code was copied from the base create component schematic.

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

amcdnl · 2018-02-27T00:45:43Z

@jelbourn - Addressed feedback.

mmalerba · 2018-02-27T17:00:23Z

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

+import { Observable } from 'rxjs/Observable';
+
+@Component({
+  selector: '<%= selector %>',<% if(inlineTemplate) { %>


So I guess schematics doesn't have a way to insert contents of another file here? Do they have a way to add comments that will be stripped out of the generated file? If so it would be good to add some comments for people maintaining this file that they need to update in 2 places

Ya, its very painful. I'm thinking about making a util to handle this automatically.

mmalerba · 2018-02-27T17:08:10Z

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

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


should this be if(viewEncapsulation !== 'Emulated')?

This code was copied from the base create component schematic.

Can we update the base schematic?

mmalerba · 2018-02-27T17:09:04Z

schematics/nav/schema.json

+    "viewEncapsulation": {
+      "description": "Specifies the view encapsulation strategy.",
+      "enum": ["Emulated", "Native", "None"],
+      "type": "string",


"default": "Emulated"

This code was copied from the base create component schematic.

Not really good enough reason on its own; Hans has mentioned that those base schematics are not meant to be normative.

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

...tics/nav/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.__styleext__

+mat-sidenav {
+  width: 200px;
+  box-shadow: 3px 0 6px rgba(0,0,0,.24);
+}


Same comment about the styles as in #10011

jelbourn · 2018-03-06T00:52:53Z

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

+  </mat-sidenav>
+  <mat-sidenav-content>
+    <mat-toolbar color="primary">
+      <button type="button" mat-icon-button (click)="drawer.toggle()" *ngIf="(_isHandset | async)!.matches">


Button needs an aria-label

jelbourn · 2018-03-06T00:54:18Z

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

+  </mat-sidenav>
+  <mat-sidenav-content>
+    <mat-toolbar color="primary">
+      <button type="button" mat-icon-button (click)="drawer.toggle()" *ngIf="(_isHandset | async)!.matches">


We need to make sure the a11y here is sensible. We may need to add a focus-trap to the sidenav (which in turn might need role="dialog" added to the sidenav element along with everything that goes along with that role)

Would you expect this role to only be present in mobile mode where its sliding out?

jelbourn · 2018-03-06T00:56:06Z

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

+})
+export class <%= classify(name) %>Component {
+  _isHandset: Observable<BreakpointState> = this._breakpointObserver.observe(Breakpoints.Handset);
+  constructor(private _breakpointObserver: BreakpointObserver) {}


Avoid prefixing with an underscore (which is a special thing we do in Angular Material that we don't recommend for most folks).

jelbourn · 2018-03-06T00:56:27Z

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

+  changeDetection: ChangeDetectionStrategy.<%= changeDetection %><% } %>
+})
+export class <%= classify(name) %>Component {
+  _isHandset: Observable<BreakpointState> = this._breakpointObserver.observe(Breakpoints.Handset);


Update this as the viewport changes?

Its handled here: [opened]="!(isHandset | async)!.matches"

amcdnl · 2018-03-08T00:11:17Z

@jelbourn - Ready for another review.

jelbourn

LGTM

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

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): add navigation schematic

517a081

amcdnl self-assigned this Feb 18, 2018

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

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

amcdnl changed the title ~~feat(schematics): add navigation schematic~~ feat(schematics): navigation schematic Feb 18, 2018

jelbourn reviewed Feb 18, 2018

View reviewed changes

jelbourn requested review from crisbeto and mmalerba February 18, 2018 21:22

crisbeto reviewed Feb 19, 2018

View reviewed changes

amcdnl added 3 commits February 26, 2018 18:41

chore: nit

c94bf64

Merge branch 'master' into nav-schematic

97188b8

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

chore: fix tests

eec2b0d

mmalerba reviewed Feb 27, 2018

View reviewed changes

jelbourn reviewed Mar 6, 2018

View reviewed changes

chore: pr feedback

1429eb4

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 279c112 into angular:master Mar 8, 2018

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

amcdnl deleted the nav-schematic branch March 8, 2018 00:47

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

		@@ -0,0 +1,18 @@
		<mat-drawer-container>
		<mat-drawer mode="side" #drawer [opened]="!isHandset">

feat(schematics): navigation schematic #10009

feat(schematics): navigation schematic #10009

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!

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