chore: enforce consistency in material-examples #11539

rafaelss95 · 2018-05-27T19:54:09Z

This:

Enables ViewEncapsulation;
Ensures that all css classes are initialized with .example-;
Ensures all spaces between curly braces are ommitted (in component [objects] and html);
Replaces redundant syntax for boolean @Inputs, like: <mat-horizontal-stepper linear="true"> with sugar syntax <mat-horizontal-stepper linear>;
Replaces all any usages with more expressive types;
Replaces classes with interfaces;
Creates interfaces for complex examples (generally arrays);
Removes unnecessary type assertions;
Indents files correctly (some were indented with 4 spaces);

crisbeto · 2018-05-28T14:26:30Z

src/material-examples/cdk-tree-flat/cdk-tree-flat-example.ts

-        node.children = this.buildFileTree(v, level + 1);
-      } else {
-        node.type = v;
+  buildFileTree(obj: object, level: number): FileNode[] {


obj: object is about as useful as value: any. Don't we know the type here?

Unfortunately we don't have the type, because we just parse a "stringified" JSON and it becomes any.

crisbeto · 2018-05-28T14:27:37Z

src/material-examples/cdk-tree-flat/cdk-tree-flat-example.ts

@@ -141,13 +135,10 @@ export class CdkTreeFlatExample {
    });
  }

+  hasChild = (_: number, _nodeData: FileFlatNode) => { return _nodeData.expandable; };


Can be reduced further to:

hasChild = (_: number, _nodeData: FileFlatNode) => _nodeData.expandable;

crisbeto · 2018-05-28T14:28:20Z

src/material-examples/cdk-tree-nested/cdk-tree-nested-example.ts

-        node.children = this.buildFileTree(v, level + 1);
-      } else {
-        node.type = v;
+  buildFileTree(obj: object, level: number): FileNode[] {


Same comment as above. We should know the type of the obj here.

crisbeto · 2018-05-28T14:29:12Z

src/material-examples/cdk-tree-nested/cdk-tree-nested-example.ts


-  hasNestedChild = (_: number, nodeData: FileNode) => {return !(nodeData.type); };
+  private _getChildren = (node: FileNode) => { return observableOf(node.children); };


These two can be reduced to:

hasNestedChild = (_: number, nodeData: FileNode) => !nodeData.type; private _getChildren = (node: FileNode) => observableOf(node.children);

This one still hasn't been addressed @rafaelss95.

crisbeto · 2018-05-28T14:32:13Z

src/material-examples/datepicker-disabled/datepicker-disabled-example.html

@@ -18,6 +18,6 @@
  <mat-form-field>
    <input matInput [matDatepicker]="dp3" placeholder="Input disabled" disabled>
    <mat-datepicker-toggle matSuffix [for]="dp3"></mat-datepicker-toggle>
-    <mat-datepicker #dp3 disabled="false"></mat-datepicker>
+    <mat-datepicker #dp3 [disabled]="false"></mat-datepicker>


The old usage was also valid.

Yes, however shouldn't we prefer to pass the literal false instead of string with false value?

Since we handle both strings and booleans, and the value doesn't change, it would be better to use the string binding. See the template syntax docs.

crisbeto · 2018-05-28T14:32:54Z

src/material-examples/focus-monitor-directives/focus-monitor-directives-example.ts

@@ -8,8 +8,8 @@ import {ChangeDetectorRef, Component, NgZone} from '@angular/core';
  styleUrls: ['focus-monitor-directives-example.css']
 })
 export class FocusMonitorDirectivesExample {
-  elementOrigin: string = this.formatOrigin(null);
-  subtreeOrigin: string = this.formatOrigin(null);
+  elementOrigin = this.formatOrigin(null);


Why was the type removed here?

Well, formatOrigin returns a string, so elementOrigin is automatically inferred as string, there's no necessity to be redundant.

crisbeto · 2018-05-28T14:34:56Z

src/material-examples/tree-checklist/tree-checklist-example.ts

-    let flatNode = this.nestedNodeMap.has(node) && this.nestedNodeMap.get(node)!.item === node.item
-      ? this.nestedNodeMap.get(node)!
-      : new TodoItemFlatNode();
+    const existentNode = this.nestedNodeMap.get(node);


existentNode -> existingNode

crisbeto · 2018-05-28T14:36:14Z

src/material-examples/tree-dynamic/tree-dynamic-example.ts

    }
    if (change.removed) {
-      change.removed.reverse().forEach((node) => this.toggleNode(node, false));
+      change.removed.reverse().forEach(node => this.toggleNode(node, false));


Might be a good idea to add a slice() before the reverse here since reverse will reverse the array in place.

crisbeto · 2018-05-28T14:37:24Z

src/material-examples/tree-loadmore/tree-loadmore-example.ts

@@ -128,10 +123,14 @@ export class TreeLoadmoreExample {
  getChildren = (node: LoadmoreNode): Observable<LoadmoreNode[]> => { return node.childrenChange; };

  transformer = (node: LoadmoreNode, level: number) => {
-    if (this.nodeMap.has(node.item)) {
-      return this.nodeMap.get(node.item)!;
+    const existentNode = this.nodeMap.get(node.item);


existentNode -> existingNode

crisbeto · 2018-05-28T14:38:10Z

src/material-examples/tree-nested-overview/tree-nested-overview-example.ts


-  hasNestedChild = (_: number, nodeData: FileNode) => {return !(nodeData.type); };
+  private _getChildren = (node: FileNode) => { return observableOf(node.children); };


Can be reduced to:

hasNestedChild = (_: number, nodeData: FileNode) => !nodeData.type; private _getChildren = (node: FileNode) => observableOf(node.children);

rafaelss95 · 2018-05-29T01:03:39Z

@crisbeto Ready for another review round.

ngbot · 2018-06-07T19:29:39Z

Hi @rafaelss95! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

rafaelss95 · 2018-06-12T01:45:55Z

@crisbeto is there something that I can do to speed up this PR?

crisbeto

LGTM

crisbeto · 2018-06-12T18:01:08Z

src/material-examples/cdk-tree-nested/cdk-tree-nested-example.ts


-  hasNestedChild = (_: number, nodeData: FileNode) => {return !(nodeData.type); };
+  private _getChildren = (node: FileNode) => { return observableOf(node.children); };


This one still hasn't been addressed @rafaelss95.

rafaelss95 · 2018-06-13T00:21:37Z

Done @crisbeto.

angular-automatic-lock-bot · 2019-09-09T03:02:15Z

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

rafaelss95 requested review from amcdnl, jelbourn and tinayuangao as code owners May 27, 2018 19:54

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

crisbeto reviewed May 28, 2018

View reviewed changes

crisbeto approved these changes Jun 12, 2018

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 12, 2018

chore: enforce consistency in material-examples

15b2346

tinayuangao merged commit f59cf59 into angular:master Jun 20, 2018

rafaelss95 deleted the consistent-material-examples branch June 21, 2018 00:50

victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018

chore: enforce consistency in material-examples (#11539)

5ba35f5

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


		hasNestedChild = (_: number, nodeData: FileNode) => {return !(nodeData.type); };
		private _getChildren = (node: FileNode) => { return observableOf(node.children); };

chore: enforce consistency in material-examples #11539

chore: enforce consistency in material-examples #11539

Uh oh!

Conversation

rafaelss95 commented May 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelss95 May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rafaelss95 commented May 29, 2018

Uh oh!

ngbot bot commented Jun 7, 2018

Uh oh!

rafaelss95 commented Jun 12, 2018

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelss95 commented Jun 13, 2018

Uh oh!

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

Uh oh!

Uh oh!

rafaelss95 May 29, 2018 •

edited

Loading