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

Add color picker to select tree color #4907

Merged
merged 12 commits into from
Nov 16, 2020
Merged

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Nov 2, 2020

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a skeleton tracing and create a tree with some nodes.
  • Then use the color selection by clicking the eye-dropper icon.
    The color of the tree should now be the newly selected color.

Issues:


Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice! I left only a few suggestions.

I think we can improve the UI for this a bit, because the functionality is kind of scattered right now and I feel like the color picking is too prominent. Some options, like "Measure skeleton length" and "Shuffle tree color" are in the dropdown menu up top and this color picker is next to the tree name and always visible. We could put all of these three things into a settings dropdown next to the tree name in the same way it's done for the groups. These setting would also only appear when hovering to keep the UI more clear. Maybe @philippotto has some opinion or suggestions for this as well? :)

@philippotto
Copy link
Member

I think we can improve the UI for this a bit, because the functionality is kind of scattered right now and I feel like the color picking is too prominent. Some options, like "Measure skeleton length" and "Shuffle tree color" are in the dropdown menu up top and this color picker is next to the tree name and always visible. We could put all of these three things into a settings dropdown next to the tree name in the same way it's done for the groups. These setting would also only appear when hovering to keep the UI more clear. Maybe @philippotto has some opinion or suggestions for this as well? :)

Yes, I completely agree with you 👍 It would also be cool if one could right click on a tree and then the settings submenu would open, too (same as clicking on the cog icon). I think antd supports contextmenu as a trigger for opening the menu.

@MichaelBuessemeyer
Copy link
Contributor Author

I think we can improve the UI for this a bit, because the functionality is kind of scattered right now and I feel like the color picking is too prominent. Some options, like "Measure skeleton length" and "Shuffle tree color" are in the dropdown menu up top and this color picker is next to the tree name and always visible. We could put all of these three things into a settings dropdown next to the tree name in the same way it's done for the groups. These setting would also only appear when hovering to keep the UI more clear.

I also think this a very good idea 👍

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Thanks for improving the UX by adding the tree setting dropdowns 👍

Please see my comment regarding the default color that is selected when the color pickers are opened (that I added before this review).
Also, the color picker no longer works for me as it instantly closes after clicking the menu entry (is only a problem in Chrome - Firefox works).

@MichaelBuessemeyer
Copy link
Contributor Author

Also, the color picker no longer works for me as it instantly closes after clicking the menu entry (is only a problem in Chrome - Firefox works).

This took me quite some time to figure it out.
I tried using a library for this but the library made it only worse.

The solution: Somehow controlling the dropdown menu's visibility made it work.

I think the actual problem was that the color selection is a child of the dropdown menu and as the color picker opens, the dropdown menu loses focus and collapses which also closes the color picker.

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer I think I fixed all your mentions and the code looks fine. Could you please check the newest changes again?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Very nice, works like a charm for me! 👍

@daniel-wer daniel-wer merged commit 794f7ab into master Nov 16, 2020
@daniel-wer daniel-wer deleted the color-selection-for-trees branch November 16, 2020 09:22
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.

User-definable colors for trees
3 participants