Skip to content

Fix color picker mode buttons#67967

Open
Iniquitatis wants to merge 1 commit intogodotengine:masterfrom
Iniquitatis:patch-2
Open

Fix color picker mode buttons#67967
Iniquitatis wants to merge 1 commit intogodotengine:masterfrom
Iniquitatis:patch-2

Conversation

@Iniquitatis
Copy link
Copy Markdown
Contributor

@Iniquitatis Iniquitatis commented Oct 28, 2022

These three buttons have been an eyesore for me for the last couple of weeks.
The changes are trivial, and theme override for these buttons seem unnecessary in any case (they aren't really attached to a tab container), not to mention that it makes them look barely readable in the light mode (yes, I'm using it wherever I can because of eyesight reasons).

EDIT: That icon on the shape popup button? Yes, dark/light mode icons are already broken in various places, so it needs to be fixed globally, not just in the color picker.

Before

Light Dark
Godot Color Picker Before Godot Color Picker Dark Before

After

Light Dark
Godot Color Picker After Godot Color Picker Dark After

@Iniquitatis Iniquitatis requested a review from a team as a code owner October 28, 2022 04:06
@Chaosus Chaosus added this to the 4.0 milestone Oct 28, 2022
@fire
Copy link
Copy Markdown
Member

fire commented Oct 28, 2022

I think this is ok. @KoBeWi ?

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Oct 28, 2022

I think the old buttons are better. It's more clear which one is active.
I'd fix the light theme instead.

@Iniquitatis
Copy link
Copy Markdown
Contributor Author

@KoBeWi My bad, I used the lower contrast (which I'm using in light mode) for demonstrating the dark mode.
Here how it looks in default color scheme:

Before After
Color Picker Default Before Color Picker Default After

If this is still not clear enough, then this is a problem with the editor's pressed button style.

Don't really want to argue over three obviously broken buttons, but they do look out of place even among the tab bars, so they're inconsistent both with tab buttons and normal buttons. Though I don't see a point in giving them a tab button appearance, as mentioned is the OP.

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Oct 28, 2022

Well I tested it and compared myself. I don't think it's button style problem, because we don't really have tab-like buttons like this anywhere else and this style was never meant to be used this way. Hence why tabs style is used - because technically these buttons are "tabs".

Not sure why the tab style looks bad on light theme. You should have the same problem with scene tabs, no?

We could gather more feedback on this matter, but personally I prefer the current style. Or we could come up with some new style.

@Iniquitatis
Copy link
Copy Markdown
Contributor Author

Hence why tabs style is used

And it is already used incorrectly.

Regular tabs:
Normal Tabs
Color picker "tabs":
Color Picker Tabs
(Both are in default color scheme.)

I cannot find tabs styled like the latter anywhere in the editor.

because technically these buttons are "tabs".

More like radio buttons. ;)
So why bother if we have to pretend that these buttons are representing a different control type? Even if they currently do look like tabs, they don't really convey the same meaning as tabs are usually somehow connected to the pages (especially in Godot itself, which isn't styled accordingly to some guidelines where tab pages are visually distinct from the tabs themselves).

Not sure why the tab style looks bad on light theme. You should have the same problem with scene tabs, no?

No, everything else looks perfectly fine.

Light Tabs

We could gather more feedback on this matter, but personally I prefer the current style. Or we could come up with some new style.

I'm obviously all for removing these style overrides for the time being as they are already do not function properly, and coming up with a new style can take quite some time. It doesn't hurt the usability more than it hurts now, and actually fixes the problem for light theme users.

@antimundo
Copy link
Copy Markdown

I think this change is actually quite nice. A good improvement in the style of the buttons.

@YuriSizov
Copy link
Copy Markdown
Contributor

YuriSizov commented Nov 5, 2022

The buttons don't update their look when the theme changes. They probably look fine if you restart the editor after changing the theme (or create a new color picker). But there is currently no code that reacts on NOTIFICATION_THEME_CHANGED to update the overrides on these buttons.

Edit: By the way, calling get_theme_* methods in the constructor, like we currently do, is generally wrong. In that moment the control is not in the tree, because it's only being initialized. This means that the styles that you get are not even from the editor theme, they are from the default theme. This is why the looks are not consistent with the other editor tabs.

This is what needs to be fixed if we want to preserve the look. But also, these classes were omitted when I was moving all control nodes to the new theme cache approach, because they were being reworked at that moment. So the whole theming code here needs to be improved. From the looks of it, fetching of theme properties is all over the place.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 14, 2023
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.

7 participants