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
Change color of labels that correspond to disabled inputs #19938
Conversation
7a15334
to
32e1296
Compare
Update: added |
@@ -23,12 +23,14 @@ public class CheckboxWidget : ButtonWidget | |||
public Func<bool> IsChecked = () => false; | |||
public int CheckOffset = 2; | |||
public bool HasPressedState = ChromeMetrics.Get<bool>("CheckboxPressedState"); | |||
public new Color TextColorDisabled = ChromeMetrics.Get<Color>("TextDisabledColor"); |
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.
Why don't we just rely on TextColorDisabled
and GetColorDisabled
already defined on the base ButtonWidget
class?
In particular ButtonWidget
defines:
public Color TextColor = ChromeMetrics.Get<Color>("ButtonTextColor");
public Color TextColorDisabled = ChromeMetrics.Get<Color>("ButtonTextColorDisabled");
So that means instead of using the ButtonTextColor
/ButtonTextColorDisabled
pair of styles which presumably work well together we are using ButtonTextColor
/TextDisabledColor
which might not necessarily work well?
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.
Is this because the text of a checkbox is not usually on the same background as a regular button? If so I would recommend getting rid of the shadowed TextColorDisabled
and just setting it in the constructor instead.
i.e. don't define public new Color TextColorDisabled ...
and in the constructors do:
TextColorDisabled = ChromeMetrics.Get<Color>("TextDisabledColor");
This means you avoid the following surprise:
CheckboxWidget c = some_value;
c.TextColorDisabled; // Is TextDisabledColor from ChromeMetrics.
ButtonWidget b = c;
b.TextColorDisabled; // Is ButtonTextColorDisabled from ChromeMetrics.
// c.TextColorDisabled is not equal to b.TextColorDisabled(!)
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.
@RoosterDragon you guessed correctly. Checkboxes inherit from buttons but they don't look like buttons - the text on a checkbox is ultimately a label and not a button and should get its colors from the same place that labels do. I was looking for a better way to do this - thanks for your suggestion, I will look into it. Or if you have a better idea, please share :-)
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.
Updated the checkbox widget to use the approach suggested here.
- Add a new widget type for input and extend it from other input widgets - Add a new label type that can be linked to an input widget - Change the label color when the input's disabled state changes
32e1296
to
23dbc94
Compare
Update: rebased and addressed #19938 (comment) |
Closes #19860