Skip to content
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

Context action to move tree to group #7005

Merged
merged 19 commits into from
Apr 25, 2023

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Apr 24, 2023

Steps to test:

  • Go to any annotions view of a dataset and switch to the Skeleton tab
  • With any tree active, right-click a group and make sure there is a context action called Move active tree here
  • Repeat this step having selected multiple trees. Make sure the label of the context action is adjusted to "trees".
  • Actually move a single and multiple trees to a group and see that it works.
  • It is not yet possible to move a group to another group via context action. Thus the context action shouldnt appear if a group is active and you want to move it to another group.
  • After having selected multiple trees, clear the selection. Right-click a group and make sure there is no context action called Move active [group|trees?] here without an active component.

Issue:


@dieknolle3333 dieknolle3333 changed the title Context action move treegroup to group Context action to move tree to group Apr 24, 2023
@dieknolle3333 dieknolle3333 marked this pull request as ready for review April 24, 2023 15:36
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great 🎉 Code looks also good, but I left a couple of smaller suggestions.

Also, I noticed that the context menu doesn't close automatically. This is also the case for other entries, but I think, we should change that. Can you add

this.handleTreeDropdownMenuVisibility(tree.treeId, false);

in the context menu item handlers where it makes sense so that the menu closes after the action? for shuffle-tree-colors, the menu shouldn't close automatically, so that users can shuffle repeatedly until they are satisfied with the new color.

@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Apr 25, 2023

Works great tada Code looks also good, but I left a couple of smaller suggestions.

Also, I noticed that the context menu doesn't close automatically. This is also the case for other entries, but I think, we should change that. Can you add

this.handleTreeDropdownMenuVisibility(tree.treeId, false);

in the context menu item handlers where it makes sense so that the menu closes after the action? for shuffle-tree-colors, the menu shouldn't close automatically, so that users can shuffle repeatedly until they are satisfied with the new color.

Thank you for the quick review!

Regarding the automatic closing of the context menu, are you referring only to group context actions or to tree context actions aswell? For group context actions, I understand that I need to close the dropdown in cases where it's useful.
For tree actions, the menu closes automatically if I delete the tree or hide/show other trees and remains open for all other actions afaict. Should this also be adjusted in any way?
[Edit:] I will add it for tree actions aswell and see what you say :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the automatic closing of the context menu, are you referring only to group context actions or to tree context actions aswell? For group context actions, I understand that I need to close the dropdown in cases where it's useful.
For tree actions, the menu closes automatically if I delete the tree or hide/show other trees and remains open for all other actions afaict. Should this also be adjusted in any way?
[Edit:] I will add it for tree actions aswell and see what you say :)

Yes, I think, your changes make sense 👍 The only thing is that this doesn't work well for "Change tree color", because that menu item opens a color picker and with your newest changes the color picker closes immediately after one has changed the color for the very first time. I added two code suggestion which revert that and then, the PR should be good to go :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent 🎉 Feel free to merge 👍

@dieknolle3333
Copy link
Contributor Author

I think I deleted my changelog entry when merging main into my branch, so I updated it again 😅

@dieknolle3333 dieknolle3333 merged commit b0368cc into master Apr 25, 2023
1 check passed
@dieknolle3333 dieknolle3333 deleted the context-action-move-treegroup-to-group branch April 25, 2023 13:40
hotzenklotz added a commit that referenced this pull request Apr 28, 2023
…ove_wkconnect

* 'master' of github.com:scalableminds/webknossos:
  Update docker compose commands + dev install readme (#7002)
  Add segment groups (#6966)
  Add screenshot nightly test for wkorg (#7030)
  Workaround for WebGL crash for datasets with many segmentation layers (#6995)
  Fix download of public annotation, include access ctx in user cache key (#7025)
  Fix that changing a segment color could lead to a crash (#7000)
  Add more error chaining to annotation download (#7023)
  Guard against NaNs in shader (#7018)
  Store editable mappings in multiple fossildb columns+keys (#6903)
  Context action to move tree to group (#7005)
  Release 23.05.0 (#7014)
  Remove vault cache when reloading dataset (#7007)
  Fix viewing of public datasets (#7010)
  Update screenshots scalebar positioning (#7003)
  Update team members (#6999)
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ty-list-drawings

* 'master' of github.com:scalableminds/webknossos: (25 commits)
  Fix issues with styling in dark mode on login page (#7052)
  Fix nightly by setting missing token (#7048)
  Release 23.05.1 (#7042)
  DRY types in update_actions.ts (#7036)
  Remove some spammy logging from backend (#7039)
  Use zarr string fill values (#7017)
  Fix voxel offset for Neuroglancer Precomputed datasets (#7019)
  Log when user is activated (#7027)
  Fix exception in applying UpdateTreeGroupVisibility skeleton action (#7037)
  Fix organization storage layouting (#7034)
  Update docker compose commands + dev install readme (#7002)
  Add segment groups (#6966)
  Add screenshot nightly test for wkorg (#7030)
  Workaround for WebGL crash for datasets with many segmentation layers (#6995)
  Fix download of public annotation, include access ctx in user cache key (#7025)
  Fix that changing a segment color could lead to a crash (#7000)
  Add more error chaining to annotation download (#7023)
  Guard against NaNs in shader (#7018)
  Store editable mappings in multiple fossildb columns+keys (#6903)
  Context action to move tree to group (#7005)
  ...
@dieknolle3333 dieknolle3333 mentioned this pull request Jun 5, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Move selected trees/groups into this group" as context action
2 participants