Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Hide/show child nodes of a branch #325
Hide/show child nodes of a branch #325
Changes from 13 commits
8f8e65a
bdfcfc5
c867398
12ff66d
7f5f35b
204583d
068e26a
32d1e1e
b8dfccf
fc9fe67
68141bc
84a6c25
ea2dd7d
5d4e097
ea706ff
4f197de
9ccde3b
36b5319
4f2288c
0d010d5
04e5176
bef9113
250a84c
587016f
5beaf21
d0be27c
85454a7
ee483a7
63f58ec
de316ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldnt it be easier to set the hidden field directly when the button is pressed for all nodes, and not inside this render method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by this? There's practically no difference whether you actually set a field on the node itself or do it like this, as you'd still have to call both methods (
setHiddenChildrenIcon
andremoveHiddenChildrenIcon
) separately whenever you add/remove the icon to make it work reliably, at least in my testing.You also can't avoid calling
dom.nodes.each
asouter.each
(which is based on theenter
method of D3.js) only seems to include nodes that were changed, ie those that were hidden, whereas we explicitly want the node that wasn't hidden.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check to return a consistent return type. here, you either return false or you assign a variable and return undefined (which is also false).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other methods also either assign a variable or return false, so I was just orienting myself on those. Maybe a general refactoring idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho only because the other methods follow a bad practice, this doesn't mean we have to continue this way. The return value is useless if it's always false? But yes, the other methods could be refactored in a separate refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've refactored this one to return undefined, I'll open an issue for the other ones.