-
Notifications
You must be signed in to change notification settings - Fork 23
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 Tree Group Color Shuffling #6586
Conversation
…to group_colors * 'group_colors' of github.com:scalableminds/webknossos: added typescript definitions for react-sortable-tree
…up_colors * 'master' of github.com:scalableminds/webknossos: suppress error toasts in isDatasetAccessibleBySwitching API call (#6580)
> | ||
<i className="fas fa-adjust" /> Shuffle Tree Group Colors | ||
</Menu.Item> | ||
{id !== MISSING_GROUP_ID ? ( |
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.
The group to tree accessors/helpers do not work with the root group. Hence, we can not offer this option on the root group.
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.
For the sake of completeness/consistency, I'd add an extra method for this case (similar to the shuffle-all-trees case). I think, it should quite short. Something like:
function ... {
const setTreeColorActions = Object.keys(this.props.trees).map((treeId) => setTreeColorAction(treeId, color));
this.props.onBatchActions(setTreeColorActions, "SET_TREE_COLOR");
}
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.
Good idea. I added such a method.
getNodeStyleClassForBackground = (id: number) => { | ||
const isTreeSelected = this.props.selectedTrees.includes(id); | ||
|
||
if (isTreeSelected) { | ||
return "selected-tree-node"; | ||
} | ||
|
||
return null; | ||
return undefined; |
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.
Change for type compatibility
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.
cool stuff! props for improving the typing, too 👍 only left two minor suggestions.
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
> | ||
<i className="fas fa-adjust" /> Shuffle Tree Group Colors | ||
</Menu.Item> | ||
{id !== MISSING_GROUP_ID ? ( |
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.
For the sake of completeness/consistency, I'd add an extra method for this case (similar to the shuffle-all-trees case). I think, it should quite short. Something like:
function ... {
const setTreeColorActions = Object.keys(this.props.trees).map((treeId) => setTreeColorAction(treeId, color));
this.props.onBatchActions(setTreeColorActions, "SET_TREE_COLOR");
}
frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx
Outdated
Show resolved
Hide resolved
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.
👍 Only see my one suggestion before merging.
…chy_view.tsx Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
PR adds the functionality to set and shuffle the colors of a whole tree group. I also fixed/added some more TS type hints for that file.
URL of deployed dev instance (used for testing):
Steps to test:
lgn_c_22_demo_synapses.nml.zip
Issues:
(Please delete unneeded items, merge only when none are left open)