more “on” colors #4929
more “on” colors #4929
Conversation
…rning and success While the label uses this color for foreground, the button uses it for background.
This keeps the color values compactly grouped at a single place. It might also prevent useless allocations because the compiler probably cannot optimize here.
These might diverge to improve the contrast.
…ning Besides “primary” these are the colors used to express different importance levels. To achieve a reasonable contrast, the color to paint on these (or to paint these on) might be different to the default foreground (or background) color. The new “on” colors are introduced with the foreground value but probably change, soon.
The production theme is not guaranteed to use unique color values and therefore is not reliable to create correct markup renderings.
Formerly, the theme’s background color was used and therefore the “onError” color was changed to the background color value to make this a seamless change.
Formerly, the theme’s background color was used and therefore the “onSuccess” color was changed to the background color value to make this a seamless change. This includes a “success button” in the Fyne demo app.
Formerly, the theme’s background color was used and therefore the “onWarning” color was changed to the background color value to make this a seamless change.
theme/color.go
Outdated
| // ColorNameOnSuccess is the name of theme lookup for a contrast color to the success color. | ||
| // | ||
| // Since: 2.5 | ||
| ColorNameOnSuccess fyne.ThemeColorName = "onSuccess" |
There was a problem hiding this comment.
I'm a bit sceptical to the "On" naming (applies to the colour we already have?). It clashes a bit too much with the namning we have for callbacks in my opinion. Can we not call it something like ColorNameContrastSuccess or similar?
There was a problem hiding this comment.
I like the naming approach (the color to be used on some other color) and don’t think that it clashes with names in a completely different field (event handling).
There was a problem hiding this comment.
I really do think it sounds as if "ColorNameOnSuccess" is a colour that in some way "happens" when something succeeds. If it is a contrasting colour to it, I'd prefer to mention contrast in the name. Do you have any thoughts on it @dweymouth?
There was a problem hiding this comment.
I agree with Jacob - I find the "on" names a little confusing. But maybe prefixing the color names with "Foreground" would work? eg:
ColorNameForegroundOnSuccess fyne.ThemeColorName = "foregroundOnSuccess"
That would:
- Make it clear that these "on" colors are variants of the foreground color, not the success color (in this example)
- Make the naming clash with events less confusing as those names all begin with
On
I also think though with the feature freeze this should wait until 2.6 to go in
There was a problem hiding this comment.
We already have one "On" colour in and we need to decide on what to do here (but yeah, I do agree on this waiting for 2.6). ColorNameForegroundOnSuccess is a big improvement. There is still room for confusion though. Maybe we can use "Over" or "Above" instead of "On" in that case? Or tie in contrast in some way?
There was a problem hiding this comment.
We should get agreement on this before the new "OnPrimary" is landed in the coming release!
There was a problem hiding this comment.
I just implemented my suggestion.
There was a problem hiding this comment.
@dweymouth @Jacalz would you go with this recent commit?
There was a problem hiding this comment.
Added some minor fixes, so the commit I was referring to is 9de5027.
This color used the wrong constant because it had already the same value as the background color when the constant for that one was introduced in 47bf464.
Description:
This PR adds additional “on” colors for the “error”, “warning” and “success” colors.
Later, these will be used to improve the contrast on buttons or other widgets using these colors.
Checklist: