docs(tree-checklist-example): root tree node correct selected state #13523
Conversation
Fixed the checked state of the root node in the tree when the initial root state was checked, but then children were deselected, causing the root node to not update correctly. Fixes angular#13314
googlebot
added
the
cla: yes
…ate when children updated When all the children of a root node was selected the root node wasn't updated to selected correctly
| descendantsAllSelected(node: TodoItemFlatNode): boolean { | ||
| const descendants = this.treeControl.getDescendants(node); | ||
| return descendants.every(child => this.checklistSelection.isSelected(child)); | ||
| const descAllSelected = descendants.every(child => | ||
| this.checklistSelection.isSelected(child) |
There was a problem hiding this comment.
It looks like the this.checklistSelection is being repeated in this function a lot and it's somewhat long. Consider moving it into a constant like const selection = this.checklistSelection.
| /** Whether all the descendants of the node are selected */ | ||
| /** | ||
| * Whether all the descendants of the node are selected. | ||
| * And remove the node from this.checklistSelection if it isn't selected. |
There was a problem hiding this comment.
I don't think that this function is the proper place to be doing the update. The function is used as a getter and it shouldn't be used to make changes to other properties. It might be more appropriate to do the change in the todoItemSelectionToggle method that is called when a checkbox is changed.
There was a problem hiding this comment.
I also thought about this; When toggling a child, it will need to find its parent and calculate its checked state. I'll refactor this after work today.
There was a problem hiding this comment.
Also when a child is toggled it calls the this.checklistSelection.toggle(node) directly, so I will have to make another function.
There was a problem hiding this comment.
There seems to not be a function to get the parent of a node. So there are two options:
- Create a getParents function in
FlatTreeControl - Create a getParents function in
tree-checklist-example.ts
For this PR I think changing only the tree-checklist-example.ts is the better way, then create a new Feature Request to implement getParents in FlatTreeControl
There was a problem hiding this comment.
I've been looking at this the whole day and thinking about a solution.
Firstly the problem lies in the fact that the isSelected state of parent nodes aren't the same value if you select it or select all the children.
So my solution was to try and rectify this, and keep the parent nodes selected state consistent.
I agree with your statement above stating that descendantsAllSelected is a getter and should be a pure function and not mutate the state of the object, but because of the nature of nested (even flattened) structures it can potentially be a very expensive operation to update every parent up the tree structure (recursive), which what needs to happen if we are going to change the state of the parent nodes on the (change) function.
So I propose my solution which tries to be a lightweight as possible (no recursion), but it does unfortunately mutate state in the getter. I have moved the mutating function to a new function.
Your thoughts would be greatly appreciated
There was a problem hiding this comment.
I don't think that recursing through the entire tree would be that expensive since it only happens once when the checkbox is toggled. Even if it's thousands of nodes, we're probably looking at something that's going to take a couple of milliseconds max. If we put aside the fact that the getter shouldn't be changing any state, there's still the issue that the getter is invoked for every change detection cycle, even if the user didn't interact with the tree.
| ); | ||
| if (this.checklistSelection.isSelected(node) && !descAllSelected) { | ||
| this.checklistSelection.deselect(node); | ||
| } else if(!this.checklistSelection.isSelected(node) && descAllSelected) { |
There was a problem hiding this comment.
Needs a space after the else if.
There was a problem hiding this comment.
Ill refactor this when I do the above changes
Root node correctly changes selected state based on children changing selected state
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
googlebot
added
cla: no
|
@fabioloreggian looks like you pushed a commit signed with a different email addressed which causes the CLA check to fail. |
I saw, and I am not sure why it did that. I'm trying to rectify it now. |
Removed the parent selection logic from the getter to its own function.
|
CLAs look good, thanks! |
googlebot
added
cla: yes
| descendantsAllSelected(node: TodoItemFlatNode): boolean { | ||
| const descendants = this.treeControl.getDescendants(node); | ||
| return descendants.every(child => this.checklistSelection.isSelected(child)); | ||
| const descAllSelected = descendants.every(child => |
There was a problem hiding this comment.
nit: you can just use return descendants.every(...) here instead of assigning it to a constant.
crisbeto
added
pr: lgtm
action: merge
|
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. |
Fixed the checked state of the root node in the tree when the initial root state was checked, but then children were deselected, causing the root node to not update correctly.
Fixes #13314