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

Make layer settings visually consistent #7295

Merged
merged 6 commits into from
Aug 31, 2023

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Aug 24, 2023

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a tracing with more than one color layer.
  • The color layers should be able to be rearranged.
  • The volume and skeleton layers should also have a drag handle with less opacity. The drag handle should be disabled and a tooltip should tell that this layer cannot be moved.

TODOs:

  • Get feedback

Issues:

  • no issue exists for this

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

const DragHandle = SortableHandle(({ hasLessThanTwoColorLayers }: DragHandleProps) => {
return hasLessThanTwoColorLayers ? (
<Tooltip title="Order is only changeable with more than one color layer.">
dragHandleIcon(true)
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this print the string dragHandleIcon because it's not in {} braces? Why not use something like <DragHandleIcon isDisabled=... />?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, I forgot the {...} here. The reason why I did not went for the "jsx-version" () is that in this scenario the tooltip does not work. The tooltip somehow needs an immediate child. This problem can be solved by adding a fragment, but this clutters the code imo more than the {...} version. Here is how it would look like with the jsx style:

<Tooltip title={`Layer not movable: ${layerType} layers are always rendered on top.`}>
  <>
    <DragHandleIcon isDisabled />
  </>
</Tooltip>

As I dislike the additional fragments, I used the { ... } style

@MichaelBuessemeyer
Copy link
Contributor Author

One open question imo is whether to hide all drag handles in case every drag handle is disabled anyway: e.g. when there is only one color layer, the color layer order cannot be changed and all the drag handles will be disabled and take up space on the screen.
Pro: less wasted screen space
Con: inconsistent behaviour compared to datasets with multiple color layers. -> in some annotations the drag handles would be rendered and in some other annotations, the drag handles are not available :/

I do not have a strong option on this but have a tendency to go for "leaving the handles always visible" as this is more consistent and might not create the confusion that for some annotations some UI elements are not available. The reason the drag handle is disabled is explained by the tooltips and that should be explanation enough why the handle does not work and is greyed out.

@philippotto
Copy link
Member

I do not have a strong option on this but have a tendency to go for "leaving the handles always visible" as this is more consistent and might not create the confusion that for some annotations some UI elements are not available. The reason the drag handle is disabled is explained by the tooltips and that should be explanation enough why the handle does not work and is greyed out.

Yes, I agree with your stance here 👍

@MichaelBuessemeyer MichaelBuessemeyer merged commit 587fde0 into master Aug 31, 2023
1 check passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the make-layer-settings-visual-consistent branch August 31, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants