Skip to content

doc(tree):simplify tree example #14718

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2019
Merged

doc(tree):simplify tree example #14718

merged 1 commit into from
Jan 25, 2019

Conversation

vivian-hu-zz
Copy link
Contributor

Remove unnecessary code to better demonstrate how to use tree.

@vivian-hu-zz vivian-hu-zz requested a review from jelbourn as a code owner January 3, 2019 22:38
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 3, 2019
@vivian-hu-zz vivian-hu-zz requested a review from manughub January 3, 2019 22:39
@vivian-hu-zz vivian-hu-zz added target: patch This PR is targeted for the next patch release pr: merge safe labels Jan 3, 2019
crisbeto
crisbeto reviewed Jan 6, 2019
View reviewed changes
andrewseguin
andrewseguin reviewed Jan 9, 2019
View reviewed changes
src/material-examples/tree-flat-overview/tree-flat-overview-example.html
@@ -1,16 +1,16 @@
<mat-tree [dataSource]="dataSource" [treeControl]="treeControl">
<mat-tree-node *matTreeNodeDef="let node" matTreeNodeToggle matTreeNodePadding>
<button mat-icon-button disabled></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

This disabled hidden button is not intuitive or explained. We use it to offset extra padding but the user may not understand why this is here. Perhaps leave a comment saying that this is necessary because it helps offset leaf nodes, or provide an additional example-leaf-offset-padding class that explicitly provides this padding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment for now. We should provide a better template later.

src/material-examples/tree-flat-overview/tree-flat-overview-example.html Outdated
@@ -1,16 +1,16 @@
<mat-tree [dataSource]="dataSource" [treeControl]="treeControl">
<mat-tree-node *matTreeNodeDef="let node" matTreeNodeToggle matTreeNodePadding>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think each tree-node should have some comment above that describes how they are distinguished. For example,

<!-- This is the tree node template for leaf nodes -->

<!-- This is the tree node template for expandable nodes -->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/material-examples/tree-nested-overview/tree-nested-overview-example.html
@@ -2,20 +2,20 @@
<mat-tree-node *matTreeNodeDef="let node" matTreeNodeToggle>
<li class="mat-tree-node">
<button mat-icon-button disabled></button>
{{node.filename}}: {{node.type}}
{{node.name}}
</li>
</mat-tree-node>

<mat-nested-tree-node *matTreeNodeDef="let node; when: hasNestedChild">
<li>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me (or the user) why this example uses <ul> and </li> tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. We should provide a better template in the following PR.

vivian-hu-zz
vivian-hu-zz commented Jan 9, 2019
View reviewed changes
Copy link
Contributor Author

@vivian-hu-zz vivian-hu-zz left a comment

Choose a reason for hiding this comment

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

Updated based Andrew's comment. Please review again.

@vivian-hu-zz vivian-hu-zz force-pushed the treeFix branch 2 times, most recently from 8764de4 to 0ad5e60 Compare January 11, 2019 22:49
vivian-hu-zz
vivian-hu-zz commented Jan 11, 2019
View reviewed changes
Copy link
Contributor Author

@vivian-hu-zz vivian-hu-zz left a comment

Choose a reason for hiding this comment

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

Remove MatTreeDatasource from CDK tree demo.

andrewseguin
andrewseguin approved these changes Jan 23, 2019
View reviewed changes
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

LGTM - good first step in simplifying the tree

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

hasNestedChild = (_: number, nodeData: FileNode) => !nodeData.type;
treeControl: NestedTreeControl<FoodNode> = new NestedTreeControl<FoodNode>(node => node.children);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove the first type declaration

andrewseguin
andrewseguin reviewed Jan 25, 2019
View reviewed changes
src/material-examples/tree-nested-overview/tree-nested-overview-example.ts Outdated
})
export class TreeNestedOverviewExample {
nestedTreeControl: NestedTreeControl<FileNode>;
nestedDataSource: MatTreeNestedDataSource<FileNode>;
treeControl: NestedTreeControl<FoodNode> = new NestedTreeControl<FoodNode>(node => node.children);
Copy link
Contributor

Choose a reason for hiding this comment

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

The immediate typing can be removed since Typescript will infer from what you are creating

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Jan 25, 2019
@ngbot
Copy link

ngbot bot commented Jan 25, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: tests_local_browsers" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@andrewseguin andrewseguin merged commit dc435f7 into angular:master Jan 25, 2019
andrewseguin pushed a commit that referenced this pull request Jan 25, 2019
@vivian-hu-zz @andrewseguin
doc(tree):simplify tree example (#14718)
939ad3f
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@devversion devversion devversion left review comments

@andrewseguin andrewseguin andrewseguin approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

@manughub manughub Awaiting requested review from manughub

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vivian-hu-zz @crisbeto @devversion @andrewseguin @googlebot