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

refactor(tree):clean up flat tree example #14399

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@vivian-hu
Contributor

vivian-hu commented Dec 5, 2018

clean up flat tree example and use element to update doc site

@vivian-hu vivian-hu requested a review from andrewseguin Dec 5, 2018

@vivian-hu vivian-hu requested a review from jelbourn as a code owner Dec 5, 2018

@googlebot googlebot added the cla: yes label Dec 5, 2018

@vivian-hu vivian-hu force-pushed the vivian-hu:master branch from 2891b97 to d26ed8d Dec 5, 2018

refactor(tree):clean up flat tree example
clean up flat tree example and use element to update doc site

@vivian-hu vivian-hu force-pushed the vivian-hu:master branch from d26ed8d to 8742959 Dec 6, 2018

@@ -24,7 +24,7 @@ import {ChecklistTreeDemo} from './checklist-tree-demo/checklist-tree-demo';
import {ChecklistNestedTreeDemo} from './checklist-tree-demo/checklist-nested-tree-demo';
import {DynamicTreeDemo} from './dynamic-tree-demo/dynamic-tree-demo';
import {LoadmoreTreeDemo} from './loadmore-tree-demo/loadmore-tree-demo';
import {ExamplePageModule} from '../example/example-module';

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

This should not be imported here. This file is auto-generated to import all of the individual example NgModules, so you're creating a circular dependency.

This comment has been minimized.

@vivian-hu

vivian-hu Dec 6, 2018

Contributor

This is a different example-module. The one you talked about is under material-examples.

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

Ah, right, I mixed those up.

@@ -50,7 +50,7 @@ export class MatTreeFlattener<T, F> {
constructor(public transformFunction: (node: T, level: number) => F,
public getLevel: (node: F) => number,
public isExpandable: (node: F) => boolean,
public getChildren: (node: T) => Observable<T[]> | T[]) {}
public getChildren: (node: T) => Observable<T[]> | T[] | undefined) {}

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

The fix for this file should really be its own PR since it's a separate bug fix from cleaning up the examples. It also needs a unit test. I'd allow null here as well.

This comment has been minimized.

@crisbeto

crisbeto Dec 6, 2018

Member

Would it make sense to allow ReadonlyArray<T> here as well? We had a fix a while ago that added it to all the data source usages.

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

That would be good. The fix would be something like "relax allowed types for tree traversal function"

{
name: 'Fruit',
children: [
{ name: 'Apple' },

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

Omit spaces in braces
(here and elsewhere)

<!-- Copyright 2018 Google Inc. All Rights Reserved.
Use of this source code is governed by an MIT-style license that
can be found in the LICENSE file at http://angular.io/license -->

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

The copyright is added automatically, you don't have to add it to the source.

October: 'pdf',
November: 'pdf',
Tutorial: 'html'
const TREE_DATA2: FoodNode[] = [

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

I would just call this foodData
(similar for other examples)

database.dataChange.subscribe(data => {
this.dataSource.data = data;
});
private _transformer = (node: FoodNode, level: number) => {

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

Don't use underscores for private members in the examples; that's just a convention for our libraries.
(here and elsewhere)

database.dataChange.subscribe(data => {
this.dataSource.data = data;
});
private _transformer = (node: FoodNode, level: number) => {

This comment has been minimized.

@jelbourn

jelbourn Dec 6, 2018

Member

Add a comment that explains what transformer does
(here and for similar code in other examples)

{{node.filename}} : {{node.type}}
</mat-tree-node>
</mat-tree>
<mat-expansion-panel-header>Flat tree</mat-expansion-panel-header>

This comment has been minimized.

@crisbeto

crisbeto Dec 6, 2018

Member

Rather than listing out the examples individually, you can use the material-example-list component to render out all of them. This has the advantage that it will include any new example that we add. You can see how it's being used in the table-demo.ts.

@@ -50,7 +50,7 @@ export class MatTreeFlattener<T, F> {
constructor(public transformFunction: (node: T, level: number) => F,
public getLevel: (node: F) => number,
public isExpandable: (node: F) => boolean,
public getChildren: (node: T) => Observable<T[]> | T[]) {}
public getChildren: (node: T) => Observable<T[]> | T[] | undefined) {}

This comment has been minimized.

@crisbeto

crisbeto Dec 6, 2018

Member

Would it make sense to allow ReadonlyArray<T> here as well? We had a fix a while ago that added it to all the data source usages.

});
private _transformer = (node: FoodNode, level: number) => {
return {
expandable: !!node.children,

This comment has been minimized.

@crisbeto

crisbeto Dec 6, 2018

Member

Should this have a check for node.children.length as well? E.g. if there's a children array, but it's empty, we might not want the node to be expandable.

return {
expandable: !!node.children,
name: node.name,
level: level,

This comment has been minimized.

@crisbeto

crisbeto Dec 6, 2018

Member

{level: level,} can be reduced to just {level,}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment