-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add ability to show only collection applicable nodes in tree #4023
Conversation
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.
Overal good, but in the future I would discourage you from making changes to the same component at the same time in different PRs (this and split tree) - that causes merge conflicts, which are not fan to resolve
Triggered by 334939a on branch refs/heads/issue-2051
@grantfitzsimmons , when you have time could you take a look at this PR on the test panel please? |
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.
With the checkbox selected, the tree still shows nodes that don't have associated objects.
Screen.Recording.2023-10-02.at.10.22.58.AM.mov
@bronwyncombs they are different queries, that's why the result are different. the tree count in tree viewer check for preferred taxon. The query builder (the one you opened in new tab), checks for taxon. |
Can this also be tested on some bigger trees like herb_rbge? |
Based on #2051, looks like collections want to hide phylums (which is a very high rank). A bit concerned about performance since the query will be more expensive for it. Would need to have performance testing for it. @grantfitzsimmons do you know any other institutions which requested this, on which we can test? If it becomes a problem,
|
Conclusion: Make this run queries on the entire tree once Vinny's count query rework is implemented. Moving to |
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.
issue-2051.mov
Nodes with count 0 are hidden when box is checked. Looks good!
@CarolineDenis Can this be in 7.9.3? |
Requested By: Eyal at The Hebrew University of Jerusalem |
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.
Works as expected, looks good.
20231220133522.mp4
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.
Conclusion: Make this run queries on the entire tree once Vinny's count query rework is implemented.
We still haven't implemented this yet, right? In @emenslin's video, counts aren't showing for every node in the tree. I still see this as a requirement (as discussed in our group weeks ago).
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.
After meeting with @CarolineDenis today, I feel the current implementation is satisfactory. While there may be a request in the future to run the count for all nodes, the behavior is understandable and resolving it is easy. I need to make a note in the documentation before release to clarify behavior to the users.
⚡ Nice work!! Thanks for talking with me about it @CarolineDenis 😄
Fixes #2051