-
Notifications
You must be signed in to change notification settings - Fork 206
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
ColorChooser : Color Field widget #5944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Eric! I know this is a work in progress so I've just made a couple of preliminary comments at this point. Couple of other thoughts in the "bikeshedding musings" category :
-
I think the "static component" makes total sense from an API perspective, but I'm not sure indicating it that way in the UI is especially intuitive. I wonder if indicating the pair of non-static components would come across better, something like this?
-
As we continue to add more features to the colour picker, I wonder if packing everything into one layout simultaneously will be the way to go or not. It seems quite nice to not have tabs at this point, and instead have everything at once, but will that hold up if we add swatches and TMI?
- To avoid tabs, could we make it possible to toggle each thing on and off individually? Could clicking again on "H" in the screenshot toggle the field off completely?
- If the field took the height of only the sliders, could we fit a bank of small swatches underneath it, to the left of the two massive previous/current swatches?
- Would TMI tip us over the edge with that approach? It feels like RGBAHSVTMI sliders might make the field wide enough (to match its new height) that it would dominate too much width-wise.
-
The square field seems to draw fast enough that we might be able to avoid the while C++/GLSL thing if the circular one performs similarly.
Cheers...
John
aef4b24
to
a9caaf5
Compare
Thanks for the initial review the other day, @johnhaddon. I've incorporated those notes and a number of other improvements into the latest push. That is also rebased onto the latest I opted for adding the widgets but making them invisible in this push. There is enough coupling between the Ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Eric! I've resolved the previous conversations and added some new notes inline. If it's easier to squash your current commits so you have fewer fixup targets for any future changes then that would be fine.
There is enough coupling between the ColorChooser and _ColorField that not having a _ColorField at all was starting to get complicated. I can revisit that if it's important to keep the widgets out entirely until we're ready to reveal them.
No, I think just hiding them for now is a good approach. Thanks!
a9caaf5
to
bd072dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Eric! I've resolved all my previous comments and added a couple more that should hopefully be quick to address.
I think the circle is an improvement over the arrow, but we could probably still do with something a little more dedicated to the purpose. But let's deal with that in the next PR where we actually expose this functionality.
Cheers...
John
bd072dd
to
2bddfe6
Compare
Great! I made addressed the comments and it's ready for a fresh look. |
2bddfe6
to
493b4e1
Compare
Thanks Eric! Squashed in the fixups, dropped the temp commit that shows the field, and and merging... |
This adds a new widget to the color chooser, the color field. Users can pin a single color component, R, G, B, H, S or V, from color slider labels and the other two components in the
RGB
orHSV
triplets will be used as the axes of the color field.I think this is in a good spot to review now, though we have talked about changing the field to a radial field when hue is the static component. But this is already a fairly significant PR, and I think adding a radial widget will slot it pretty cleanly with the current structure.
Checklist