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

Segment multiselect #7242

Merged
merged 24 commits into from
Aug 7, 2023
Merged

Segment multiselect #7242

merged 24 commits into from
Aug 7, 2023

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Jul 31, 2023

Steps to test:

  • Go to segments tab in any annotation, preferably one with a mesh file
  • Press shift and select more than one segment
  • Right-click to open the context menu like you are used to
  • Select any action you would like to execute on all selected segments, such as loading all meshes
  • Make sure this also works for meshes in different groups
  • Make sure all actions are working as expected
  • Make sure the visibility toggle for "Show /Hide meshes is working"
  • Make sure selecting a group after having selected multiple segments opens a toast with a warning like in the skeleton tab
  • After having selected meshes, open a group context menu and move all selected segments into the group

TODOs:

  • self-review code before making the PR ready-for-review
  • fix case where group is selected after having selected multiple segments, but ctrl is not pressed (currently missing warning toast)

Issues:


(Please delete unneeded items, merge only when none are left open)

@@ -455,7 +457,7 @@ function _SegmentListItem({
padding: "2px 5px",
}}
className={classnames("segment-list-item", {
"is-selected-cell": segment.id === selectedSegmentId,
"is-selected-cell": selectedSegmentIds?.includes(segment.id), //TODO this wont work
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what exactly does this do? If I set it to false the segments are selected nonetheless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(will delete the TODO comment obviously)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will test whether this can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this actually makes the coloring a little better, but the box thats created while hovering has a different size than the box thats created upon selection, so I will see whether I can maybe improve this.

@dieknolle3333 dieknolle3333 marked this pull request as ready for review July 31, 2023 14:16
@@ -466,7 +465,11 @@ function _SegmentListItem({
}}
>
<Dropdown
Copy link
Contributor Author

@dieknolle3333 dieknolle3333 Jul 31, 2023

Choose a reason for hiding this comment

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

@philippotto I know we talked about increasing the zone where a segment item can be right-clicked by letting the dropdown be the parent of more content. I did not manage to do that however, because then the segment name would not react to the click anymore (so only the MeshInfoItem) – I had tried to include the MeshInfoItem into the dropdown.

@dieknolle3333 dieknolle3333 self-assigned this Jul 31, 2023
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.

Didn't test yet, but looks pretty good already :)

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.

Awesome, works really well 👍 Please merge on Monday :)

@dieknolle3333 dieknolle3333 merged commit 2d412fa into master Aug 7, 2023
@dieknolle3333 dieknolle3333 deleted the segment-multiselect branch August 7, 2023 11:12
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.

Segments Groups
2 participants