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 menu 3: add "Move to new container" context menu action #5210

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Feb 15, 2024

What

Export-1708013884748



This PR adds a new "Move to new container" context menu action, that applies when one or more space views and/or containers are selected. The new container is created at the location of the clicked item, and all the selected items are moved into it.

Note: one very often runs into this while using this feature:

TODO in future PR:

  • additional actions
  • entity level manipulations
  • use ListItem instead of label/WidgetText
  • better folder structure
  • release checklist update

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@abey79 abey79 added ui concerns graphical user interface include in changelog labels Feb 15, 2024
@teh-cmc teh-cmc self-requested a review February 16, 2024 08:38
@teh-cmc
Copy link
Member

teh-cmc commented Feb 16, 2024

It might be a bit weird that this interaction shows up even when the multi-selection includes the viewport itself:
image

It doesn't actually applies to it if you go through with it so it's not the end of the world, just weird.

@teh-cmc
Copy link
Member

teh-cmc commented Feb 16, 2024

It is a bit weird that the context menu shows options that will end up doing nothing (e.g. Move to horizontal when you're already in an horizontal container); but again not the end of the world.

@abey79
Copy link
Member Author

abey79 commented Feb 16, 2024

It might be a bit weird that this interaction shows up even when the multi-selection includes the viewport itself

Yeah, that's a matter of choice. The alternative I considered is to disable entirely the context menu whenever the selection is "root container + something", because none of the operations that applies to multiple hierarchy thing can be applied to the root container (visibility, remove, move to new container).

None of these seem ideal to me though.

Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Once again discovered many things I didn't know about 🤯

crates/re_viewport/src/viewport.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/viewport.rs Outdated Show resolved Hide resolved
@abey79
Copy link
Member Author

abey79 commented Feb 16, 2024

It is a bit weird that the context menu shows options that will end up doing nothing (e.g. Move to horizontal when you're already in an horizontal container); but again not the end of the world.

Two issues there:

  • I forgot to exclude horizontal-in-horizontal (and V-in-V) options, which doesn't make sense 🤦🏻. Will fix.
  • We still have some simplification of the hierarchy that kicks in in a disruptive was sometimes. I still have to track that, but it's a wider issue (ie. affect all creation of containers, not only through context menu.

Base automatically changed from antoine/cm2-multi-selection to main February 16, 2024 14:09
@abey79 abey79 force-pushed the antoine/cm3-move-to-new-container branch from d3dfd4e to cf1cd55 Compare February 16, 2024 15:39
@abey79 abey79 merged commit c2646fd into main Feb 16, 2024
40 checks passed
@abey79 abey79 deleted the antoine/cm3-move-to-new-container branch February 16, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants