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

Added "Randomize Marker Color" option from #2210 #2333

Merged

Conversation

Azagwen
Copy link
Contributor

@Azagwen Azagwen commented May 12, 2024

This pull adds a "random marker color" option in the outliner context menu, which will pick a random color (different from the current one) and apply it to any selected element

image

demo video:

electron_dMGeebxFbF.mp4

Copy link

netlify bot commented May 12, 2024

Deploy Preview for blockbench-dev ready!

Name Link
🔨 Latest commit d201be8
🔍 Latest deploy log https://app.netlify.com/sites/blockbench-dev/deploys/666df36c7568cc00088f30bd
😎 Deploy Preview https://deploy-preview-2333--blockbench-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Azagwen Azagwen changed the title Added suggestion from #2210 Added "Randomize Marker Color" option from #2210 May 14, 2024
@JannisX11
Copy link
Owner

A few things to improve:

  • Let's implement this as a standalone Action instead of a menu item. That way users can add it to their toolbars and assign keybindings. Actions can be added back to menus by adding their ID as a string
  • Let's change the name to "Randomize Marker Colors". That way the action is more clear, and the plural clarifies that each element gets its own color
  • Let's change the icon to "fa-shuffle"
  • I haven't fully made up my mind about it, but it might be neat to move this action directly into the Marker Color submenu, below the other points. That way the main menu is less cluttered. But lmk your thoughts.

@Azagwen
Copy link
Contributor Author

Azagwen commented Jun 2, 2024

Will do these changes when I find the time 👍

@Azagwen
Copy link
Contributor Author

Azagwen commented Jun 15, 2024

  • Made it into an action
  • Changed name
  • Replaced individual element code with action reference like other global actions
  • Changed icon
  • Code for the Group element is now the only function for this, and was moved to the action definition, as it works for all elements
electron_MGHOxTaSE1.mp4

@Azagwen
Copy link
Contributor Author

Azagwen commented Jun 15, 2024

A few things to improve:

* Let's implement this as a standalone Action instead of a menu item. That way users can add it to their toolbars and assign keybindings. Actions can be added back to menus by adding their ID as a string

* Let's change the name to "Randomize Marker Colors". That way the action is more clear, and the plural clarifies that each element gets its own color

* Let's change the icon to "fa-shuffle"

* I haven't fully made up my mind about it, but it might be neat to move this action directly into the Marker Color submenu, below the other points. That way the main menu is less cluttered. But lmk your thoughts.

About moving this to the marker color sub-menu, I'm a little mixed myself.

Moving it there would de-clutter the context menu a little bit, but it would also make this option less accessible (one hover away), individual marker colors being in a sub-menu makes sense UX wise, because there are 10 of them (even more with plugins) so having all of them in the context menu would just be a massive clutter (even more with plugins, again), while the randomizer is only one option.

I'm also worried that users might miss it, or mistake it as a sort of "special" color if it's put here, given that it would be one option out of 10+. Especially if they use plugins to have more colors, causing that option to be pushed even further down and flooded by options that all behave the same from one another, and look roughly the same on the UI (color excluded) 🤔

@JannisX11
Copy link
Owner

Thanks, I'm happy with this!

@JannisX11 JannisX11 merged commit 22e6b9c into JannisX11:next Jun 24, 2024
4 checks passed
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.

2 participants