fix(tree): improve nested tree node & fix nested tree control #10454
Conversation
googlebot
added
the
cla: yes
tinayuangao
force-pushed
the
tree-improve-nested
branch
2 times, most recently
from
955dd08 to
10d85e9
Compare
tinayuangao
added
pr: needs review
target: major
tinayuangao
requested review from
andrewseguin,
jelbourn and
crisbeto
and removed request for
andrewseguin
| @@ -26,15 +26,17 @@ export class NestedTreeControl<T> extends BaseTreeControl<T> { | |||
| expandAll(): void { | |||
| this.expansionModel.clear(); | |||
| let toBeExpanded = <any>[]; | |||
| this.dataNodes.forEach(dataNode => toBeExpanded.push(...this.getDescendants(dataNode))); | |||
| this.dataNodes.forEach(dataNode => | |||
There was a problem hiding this comment.
Rather than using a forEach and a push, you should be able to use map or reduce instead.
| this.expansionModel.select(...toBeExpanded); | ||
| } | ||
|
|
||
| /** Gets a list of descendant dataNodes of a subtree rooted at given data node recursively. */ | ||
| getDescendants(dataNode: T): T[] { | ||
| const descendants = []; | ||
| this._getDescendants(descendants, dataNode); | ||
| return descendants; | ||
| // Remove the node itself |
There was a problem hiding this comment.
This only returns a slice with the second item in it, but it doesn't remove it. Is it intended?
src/cdk/tree/nested-node.ts
Outdated
| }); | ||
| this.nodeOutlet.changes.pipe(takeUntil(this._destroyed)) | ||
| .subscribe((_) => this._addChildrenNodes()); | ||
| .subscribe((_) => this._updateChildrenNodes()); |
There was a problem hiding this comment.
Not something that was added in this PR, but you can remove the _ parameter here.
src/cdk/tree/tree.ts
Outdated
| } else { | ||
| throw getTreeNoValidDataSourceError(); | ||
| } | ||
| } | ||
|
|
||
| /** Check for changes made in the data and render each change (node added/removed/moved). */ | ||
| private _renderNodeChanges(dataNodes: T[]) { | ||
| const changes = this._dataDiffer.diff(dataNodes); | ||
| renderNodeChanges(data: T[], dataDiffer?: IterableDiffer<T>, viewContainer?: ViewContainerRef) { |
There was a problem hiding this comment.
Instead of doing the ternary for the dataDiffer and the viewContainer below, you can provide the default ones as parameters:
renderNodeChanges(data: T[], dataDiffer: IterableDiffer<T> = this._dataDiffer, viewContainer: ViewContainerRef = this._nodeOutlet.viewContainer) {
}
tinayuangao
force-pushed
the
tree-improve-nested
branch
from
10d85e9 to
8dcc6a0
Compare
|
@crisbeto Comments addressed. Please take a look. Thanks for review! |
src/cdk/tree/tree.ts
Outdated
| const changes = this._dataDiffer.diff(dataNodes); | ||
| renderNodeChanges(data: T[], dataDiffer: IterableDiffer<T> = this._dataDiffer, | ||
| viewContainer: ViewContainerRef = this._nodeOutlet.viewContainer) { | ||
| const changes = (dataDiffer ? dataDiffer : this._dataDiffer).diff(data); |
There was a problem hiding this comment.
Now that you're guaranteed to have a dataDiffer, you don't need the ternary here anymore.
| this.dataNodes.forEach(dataNode => toBeExpanded.push(...this.getDescendants(dataNode))); | ||
| this.expansionModel.select(...toBeExpanded); | ||
| const allNodes = this.dataNodes.map(dataNode => [...this.getDescendants(dataNode), dataNode]) | ||
| .reduce((first, second) => [...first, ...second]); |
There was a problem hiding this comment.
I don't think you need both map and reduce here. Something like this should work the same:
this.dataNodes.reduce((accumulator, dataNode) => [...accumulator, ...this.getDescendants(dataNode), dataNode], []);
tinayuangao
force-pushed
the
tree-improve-nested
branch
from
8dcc6a0 to
aee0af9
Compare
|
Updated and thanks for review! |
tinayuangao
added
action: merge
tinayuangao
force-pushed
the
tree-improve-nested
branch
from
aee0af9 to
e20030e
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.