-
Notifications
You must be signed in to change notification settings - Fork 207
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
Expose color chooser widgets #6003
Expose color chooser widgets #6003
Conversation
f0258ed
to
f3c8804
Compare
I made some changes today and squashed them into the original commits since they haven't been reviewed yet. They're cosmetic for the most part - tweaked some icons / indicators, added text entries to the menu for the color field component choices and made some code improvements. 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 didn't manage to spot the code change that removed the spacing between the numeric widgets and the sliders, but I think we should restore the original spacing. Butting up the rounded edge of the numeric field with the flat edge of the slider reads strangely and although it would be fixable by a flat join and a rounded right end of the slider, I think the widgets are distinct enough to deserve their original spacing. It also feels like we could do with a similar spacing on the left of the menu icon? I suspect you're looking for ways of getting a bit more horizontal space now we're squeezing the sliders from both sides, but I think we do need the spacing for clarity still.
My other comments are a mixed bag of API/metadata stuff and UX/bikeshedding stuff. For your sanity, it might be best to attack the former first as I think it's reasonably objective. The rest is undoubtedly subjective, and should be weighed up against other opinions, and - i think - some testing with real users.
Cheers...
John
87bf541
to
4bce89b
Compare
I pushed a stack of fixups to address a good portion of the comments this afternoon. I also added a small fix to deactivate the static component menu items if the color field is not visible : 5ddc788 I think the main things were down to is improving the layout for keeping the color field square, and generally getting user feedback on the changes overall and especially the choice mechanism of the static channel. |
4bce89b
to
1731ab6
Compare
1731ab6
to
dea359b
Compare
I updated the commit links to the fixups above and added a custom layout for the color chooser row to address the double-layout processing we were doing. I also added a few commits to address user feedback :
I think that takes care of the code comments and most of the user-feedback for this PR. Would be great to get some interactive testing on the what-components-are-in-the-color-field interaction in particular. Ready for a new look! |
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! Many apologies in advance for continuing the bikeshedding with a few inline UX-related comments after playing with these latest changes...
3119cd1
to
b06aa05
Compare
I pushed a raft of new commits that address the comments above, which I've noted inline. There are a couple of additional commits too :
|
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! Feels like we're pretty close to something we can ship in a 1.5 alpha. Few comments inline but nothing too major...
15fcc7b
to
f448a56
Compare
All the latest notes are addressed in my most recent push. Hopefully very close and I can do a big squash! |
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! One last comment about the changelog, but feel free to address it while you're squashing everything down.
fdfbe57
to
f086446
Compare
f086446
to
0669040
Compare
The previous approach was causing they layout to often be resized multiple times because we were resizing the color field widget within the resize event. This creates a custom widget and custom layout to achieve the same layout constraints without multiple resize events.
Expanding these icons by 2px makes them the same width as the gear icon which is used on `ColorPlugValueWidget` and makes their left edges aligned.
0669040
to
a29d617
Compare
I squashed everything down today and went through and edited the commits to make sure they make sense in isolation and do what they claim to do. The only change I made was removing a few entries left over in Checking against a backup branch from before all the rebasing shows only changes to the |
Thanks! Merging! |
This reveals the new widgets added in and #5962, #5944 and #5999 and gives control over their visibility.
There's probably some discussion to have over the API introduced in f0258ed for getting and setting widget visibility in the Color Chooser, and how the state is stored.
When creating a color chooser, there are three places it's looking to see if there are settings for widget visibility :
Color3f
/Color4f
plug(s) the widget is associated withcolorChooser:inlineOptions
/colorChooser:dialogueOptions
userDefault
global metadata entrycolorChosoer:inlineOptions
/colorChooser:dialogueOptions
sessionDefault
global metadata entryThis way you get the state associated with the plug if there is one. If there's no opinion for the plug, we prioritize
userDefault
on the assumption that someone went to the trouble to set that in a startup script and we should honor that. If that does not exist, we use a per-session variable on the assumption that if a user turned off some elements for one plug's widget, there's a good chance they will want them off for other plugs as well.And lastly, there are separate options metadata entries for the inline and dialogue options. Mixing them seems like the logic and expectations could become unclear, and my theory is that people tend to use the inline vs dialogue for different reasons and may want different UI elements visible in each.
Checklist