feat(tree): Add Cdk Tree component #7984

tinayuangao · 2017-10-23T21:46:19Z

An initial version of cdk-tree

jelbourn · 2017-10-23T23:59:43Z

src/cdk/collections/tree-adapter.ts

@@ -0,0 +1,18 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.


The license changed recently; it's Google LLC now

jelbourn · 2017-10-24T00:00:19Z

src/cdk/collections/tree-adapter.ts

+
+  expandFlattenedNodes(nodes: T[], expansionModel: SelectionModel<T>): T[];
+
+  nodeDecedents(node: T, nodes: T[], onlyExpandable: boolean);


Add JsDoc description for these methods?

jelbourn · 2017-10-24T00:01:03Z

src/cdk/tree/nested-node.ts

+
+
+/**
+ * Nested node, add children to `cdkNodePlaceholder` in template.


Clarify that this is a child of <cdk-tree> and what is means to be a "nested node"? I'm also not sure exactly what "add children to `cdkNodePlaceholder` in template." means

jelbourn · 2017-10-24T00:03:57Z

src/cdk/tree/nested-node.ts

+  protected _childrenSubscription: Subscription;
+
+  /** Subscribes to node placeholder changes. */
+  protected _placeholderSubscription: Subscription;


One pattern we've been using more lately is to create a subject like _destroyed and then use takeUntil(this._destroyed) to avoid needing to manually keep track of the subscription and unsubscribe

jelbourn · 2017-10-24T00:05:31Z

src/cdk/tree/nested-node.ts

+  @ContentChildren(CdkNodePlaceholder) nodePlaceholder: QueryList<CdkNodePlaceholder>;
+
+  /** The Children nodes data. */
+  protected _children: T[];


I'm not very clear on what this represents exactly

jelbourn · 2017-10-24T00:31:58Z

src/cdk/tree/tree-control.ts

+
+  /** Expands a subtree rooted at given `node` recursively. */
+  expandDecedents(node: T) {
+    let decedents = this.getDecedents(node);


Prefer const (here and elsewhere)

jelbourn · 2017-10-24T00:32:41Z

src/cdk/tree/tree-control.ts

+   */
+  expandAll() {
+    this.expansionModel.clear();
+    this.nodes.forEach((node) => this._expandDecedents(node));


Nit: don't need parens for a single argument with inferred type (here and elsewhere)

jelbourn · 2017-10-24T00:33:32Z

src/cdk/tree/tree-control.ts

+      if (children) {
+        children.forEach((child) => {
+          this.collapseDecedents(child)
+        });


Don't need to use braces here

children.forEach(child => this.collapseDecedents(child));

(similar elsewhere)

jelbourn · 2017-10-24T00:35:24Z

src/cdk/tree/tree-data.ts

+
+/** Interface for data in flat tree. Tree users should implement this interface to use CdkTree. */
+export interface FlatNode {
+  level: number;


Should we do a getLevel method instead of a property? It might be annoying if someone has a level property in their data that means something else. (Same for expandable -> isExpandable).

I also wonder if there's a way to could express this in the template (similar to when) instead of requiring an interface, but I forget if we already explored that option

We didn't explore that option.

I can try to express it in the template, but TreeControl is also using that to expand/collapse subtrees/the whole tree.

If we want to express this in the template, we can also pass that info to TreeControl, or put getLevel getChildren and isExpandable functions in TreeControl and let other components use functions in TreeControl.

Now moved getLevel isExpandable and getChildren to TreeControl. Removed NestedNode and FlatNode types and users can use any type of data.

jelbourn · 2017-10-24T00:37:40Z

src/cdk/tree/tree.spec.ts

+        SimpleCdkTreeApp,
+        // DynamicDataSourceCdkTreeApp,
+        // NodeContextCdkTreeApp,
+        // WhenNodeCdkTreeApp


Add TODO: in front of each of these? That will change them from commented-out-code to just comments

tinayuangao · 2017-10-25T17:38:48Z

Comments addressed. Please take a look. Thanks!

jelbourn

Some quick comments before I I have to run

jelbourn · 2017-10-26T02:36:48Z

src/cdk/collections/tree-adapter.ts

+ */
+export interface TreeAdapter<T> {
+  /** Flatten structured data to an array of data. */
+  flattenNodes( structuredData: any[]): T[];


Extra space

jelbourn · 2017-10-26T02:37:31Z

src/cdk/collections/tree-adapter.ts

+   * Put node descendents of node in array.
+   * If `onlyExpandable` is true, then only process expandable descendents.
+   */
+  nodeDecedents(node: T, nodes: T[], onlyExpandable: boolean);


"Decedents" -> "Descendants"
(here and elsewhere)

jelbourn · 2017-10-26T02:38:06Z

src/cdk/collections/tree-adapter.ts

@@ -0,0 +1,28 @@
+/**
+ * @license
+ * Copyright Google LLC. All Rights Reserved.


Extra . after LLC

jelbourn · 2017-10-26T02:39:25Z

src/cdk/tree/control/base-tree-control.ts

+  nodes: T[];
+
+  /** Expansion info: the changes */
+  expandChange = new BehaviorSubject<T[]>([]);


Maybe expansionChange? @andrewseguin thoughts?

jelbourn · 2017-10-26T02:40:42Z

src/cdk/tree/control/base-tree-control.ts

+  }
+
+  /** Whether a given node is expanded or not. Returns true if the node is expanded. */
+  expanded(node: T) {


isExpanded?

jelbourn · 2017-10-26T02:47:04Z

src/cdk/tree/node.ts

+  /** The tree node data */
+  @Input('cdkTreeNode')
+  set data(v: T) {
+    this._data = v;


Break the content of this setter into a separate function?

jelbourn · 2017-10-26T02:47:25Z

src/cdk/tree/node.ts

+    }
+  }
+  get data(): T { return this._data; }
+  _data: T;


Make this private?

jelbourn · 2017-10-26T02:47:38Z

src/cdk/tree/node.ts

+  _data: T;
+
+  /** The offset top of the element. Used by CdkTree to decide the order of the nodes. [Focus] */
+  private get offsetTop() {


This this is private, make it a method?

jelbourn · 2017-10-26T02:48:32Z

src/cdk/tree/padding.ts

+    return this._dir && this._dir.value === 'rtl' ? this._paddingIndent() : null;
+  }
+
+  constructor(private _treeNode: CdkTreeNode<T>,


Move constructor above methods and below properties

jelbourn · 2017-10-26T02:49:01Z

src/cdk/tree/padding.ts

+
+  constructor(private _treeNode: CdkTreeNode<T>,
+              private _tree: CdkTree<T>,
+              @Optional() private _dir: Directionality) {}


Probably want to have dir.change as well

jelbourn · 2017-10-26T04:54:35Z

src/cdk/tree/trigger.ts

+})
+export class CdkNodeTrigger<T> {
+  /** Whether expand/collapse the node recursively. */
+  @Input('cdkNodeTriggerRecursive') recursive: boolean = true;


coerceBooleanProperty?

jelbourn · 2017-10-26T04:55:16Z

src/cdk/tree/trigger.ts

+ * Node trigger to expand/collapse the node.
+ */
+@Directive({
+  selector: '[cdkNodeTrigger]',


cdkNodeTrigger doesn't seem quite specific enough. Maybe cdkTreeNodeToggle?

jelbourn · 2017-10-26T04:58:27Z

src/cdk/tree/tree.spec.ts

+import {CdkTree} from './tree';
+
+
+describe('CdkTree', () => {


In a follow-up PR, it would be good to explore creating a syntax-sugar way of testing tree content. The table, for example, has this:

expectTableToMatchContent(tableElement, [ ['Column A', 'Column B', 'Column C'], ['a_1', 'b_1', 'c_1'], ['a_2', 'b_2', 'c_2'], ['a_3', 'b_3', 'c_3'], ]);

For the tree, we'd need to capture level and expansion state

jelbourn · 2017-10-26T04:59:05Z

src/cdk/tree/tree-module.ts

+import {CdkNodeDef, CdkTreeNode} from './node';
+import {CdkNestedTreeNode} from './nested-node';
+
+const EXPORTED_DECLARATIONS = [


Maybe just call this TREE_DIRECTIVES?

crisbeto · 2017-10-26T19:29:12Z

src/cdk/tree/control/base-tree-control.ts

+  expansionModel: SelectionModel<T> = new SelectionModel<T>(true);
+
+  /** Toggles one single node. Expands a collapsed node or collapse an expanded node. */
+  toggle(node: T) {


All of these public methods need return types. Same goes for the other files.

crisbeto · 2017-10-26T19:30:59Z

src/cdk/tree/control/base-tree-control.ts

+  /** Expands one single node. */
+  expand(node: T) {
+    this.expansionModel.select(node);
+    this.expansionChange.next(this.expansionModel.selected);


You should be able to replace the expansionChange event with the onChange from the SelectionModel.

crisbeto · 2017-10-26T19:33:04Z

src/cdk/tree/control/nested-tree-control.ts

+   */
+  expandAll() {
+    this.expansionModel.clear();
+    this.nodes.forEach((node) => this._expandDescendants(node));


nit: you don't need the parentheses around node.

crisbeto · 2017-10-26T19:35:58Z

src/cdk/tree/control/nested-tree-control.ts

+        children.forEach((child: T) => this._getDescendants(descendants, child));
+      }
+    });
+    subscription.unsubscribe();


Rather than unsubscribing manually here, you could use the first operators instead.

crisbeto · 2017-10-26T19:39:11Z

src/cdk/tree/node.ts

+      if (!this._tree.treeControl.getChildren) {
+        throw getTreeControlFunctionsMissingError();
+      }
+      takeUntil.call(this._tree.treeControl.getChildren(this._data), this._destroyed)


Since this subscription is set up by the setter, shouldn't it unsubscribe from any previous subscriptions? Otherwise you could end up with multiple subscriptions if the data is set multiple times.

crisbeto · 2017-10-26T19:40:10Z

src/cdk/tree/node.ts

+  }
+
+  /** The offset top of the element. Used by CdkTree to decide the order of the nodes. [Focus] */
+  private getOffsetTop() {


I don't see this method being used anywhere, is it still relevant?

crisbeto · 2017-10-26T19:43:23Z

src/cdk/tree/tree.ts

+
+  ngAfterViewInit() {
+    // For key traversal in correct order
+    this.items.changes.subscribe(items => {


It seems like you're never unsubscribing from this one and I'm not sure whether Angular will complete the stream on destroy.

crisbeto · 2017-10-26T19:44:59Z

src/cdk/tree/tree.ts

+      this.orderedNodes.reset(nodes);
+
+      const activeItem = this._keyManager ? this._keyManager.activeItem : null;
+      this._keyManager = new FocusKeyManager(this.orderedNodes);


Since you're mutating the same QueryList, you shouldn't need to re-create the key manager every time the items change. You may have to reset the active index though.

Reset it because the order of the items in the QueryList is wrong. For example:

node 1

node 2
After expanded node 1, it becomes

node 1
---- node 3
---- node 4
-node 2
in item list, and UP_ARROW and DOWN_ARROW will jump from one node to another.
Resetting the nodes make it become

node 1
---- node 2
---- node 3

node 4
in key manager.

Removed key manager related part.

crisbeto · 2017-10-26T19:45:58Z

src/cdk/tree/tree.ts

+    if (event.keyCode == UP_ARROW) {
+      this._keyManager.setPreviousItemActive();
+    } else if (event.keyCode == DOWN_ARROW) {
+      this._keyManager.setNextItemActive();


The UP_ARROW and DOWN_ARROW behavior should be handled by the key manager already.

tinayuangao · 2017-10-27T22:11:58Z

Added tests for flat-tree-control and nested-tree-control.
Removed cdkTreeNode input from CdkTreeNode and put the data through static variable.
Removed key manager related part.

Please take a look. Thanks for review!

crisbeto

LGTM, a few nits left.

crisbeto · 2017-10-28T10:01:01Z

src/cdk/tree/nested-node.ts

+        this._addChildrenNodes();
+      });
+    takeUntil.call(this.nodeOutlet.changes, this._destroyed)
+      .subscribe((_) => this._addChildrenNodes());


The _ param here is unnecessary. We may have a lint rule for unused parameters that would complain about this.

Looks like this is still here

crisbeto · 2017-10-28T10:02:05Z

src/cdk/tree/node.ts

+   */
+  when: (nodeData: T, index: number) => boolean;
+
+  /** @docs-private */


You should be able to set this on the directive doc, rather than the constructor.

crisbeto · 2017-10-28T10:06:17Z

src/cdk/tree/padding.ts

+  }
+
+  /** The left padding indent value for the tree node. */
+  paddingIndentLeft(): string|null {


Rather than having two properties and bindings for left and right, you could set the padding-left/padding-right instead. E.g.:

setPadding() { const padding = this._paddingIndent(); const paddingProp = this._dir && this._dir.value === 'rtl' ? 'padding-right' : 'padding-left'; this._renderer.setStyle(this._element.nativeElement, paddingProp, padding); }

Updated padding.ts. Now set the properties and listen to dir.change.

googlebot · 2017-10-30T17:22:05Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

tinayuangao · 2017-11-14T01:54:02Z

Comments addressed. Please take another look. Thanks!

* Add Cdk Tree component * Put tree control in control folder. Removed NestedNode FlatNode types and add getLevel getChildren isExpandable in TreeControl. * Use static varaible to pass node data * fix lint * another round * update documents according to comments

timeurg · 2018-01-24T09:02:17Z

src/cdk/tree/control/flat-tree-control.spec.ts

+      expect(treeControl.expansionModel.selected.length)
+          .toBe(2, 'Expect two dataNodes in expansionModel');
+
+      treeControl.collapse(seconNode);


typo seconNode -> secondNode

* Add Cdk Tree component * Put tree control in control folder. Removed NestedNode FlatNode types and add getLevel getChildren isExpandable in TreeControl. * Use static varaible to pass node data * fix lint * another round * update documents according to comments

angular-automatic-lock-bot · 2019-09-08T04:03:02Z

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

tinayuangao requested review from andrewseguin, crisbeto and jelbourn as code owners October 23, 2017 21:46

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 23, 2017

jelbourn reviewed Oct 24, 2017

View reviewed changes

CaerusKaru mentioned this pull request Oct 24, 2017

mat-tree #3175

Closed

tinayuangao force-pushed the cdk-tree branch 4 times, most recently from cd453e3 to 0267610 Compare October 25, 2017 23:44

jelbourn reviewed Oct 26, 2017

View reviewed changes

tinayuangao force-pushed the cdk-tree branch 2 times, most recently from bd7b0c0 to 2df887c Compare October 26, 2017 17:00

crisbeto reviewed Oct 26, 2017

View reviewed changes

tinayuangao force-pushed the cdk-tree branch 2 times, most recently from 8918a41 to 437566f Compare October 27, 2017 22:08

crisbeto approved these changes Oct 28, 2017

View reviewed changes

tinayuangao force-pushed the cdk-tree branch from 437566f to 6eecfd0 Compare October 30, 2017 17:21

tinayuangao requested review from amcdnl, devversion, josephperrott, kara and mmalerba as code owners October 30, 2017 17:21

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Oct 30, 2017

tinayuangao added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 30, 2017

update documents according to comments

5fa650a

andrewseguin approved these changes Nov 14, 2017

View reviewed changes

tinayuangao merged commit 0d04229 into angular:tree Nov 14, 2017

timeurg reviewed Jan 24, 2018

View reviewed changes

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


		expandFlattenedNodes(nodes: T[], expansionModel: SelectionModel<T>): T[];

		nodeDecedents(node: T, nodes: T[], onlyExpandable: boolean);



		/**
		* Nested node, add children to `cdkNodePlaceholder` in template.

feat(tree): Add Cdk Tree component #7984

feat(tree): Add Cdk Tree component #7984

Uh oh!

Conversation

tinayuangao commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tinayuangao commented Oct 25, 2017

Uh oh!

jelbourn left a comment

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!

tinayuangao commented Oct 23, 2017 •

edited

Loading