Update RadialGauge to use theme resources #1771
Conversation
| Text="{TemplateBinding Value}" | ||
| TextAlignment="Center" /> | ||
| <TextBlock Margin="0" | ||
| FontSize="16" | ||
| Foreground="{TemplateBinding UnitBrush}" | ||
| Foreground="{ThemeResource RadialGaugeAccentBrush}" |
There was a problem hiding this comment.
Can we have this default to the Foreground brush if the RadialGaugeAccentBrush isn't specified?
There was a problem hiding this comment.
How would it not be specified?
There was a problem hiding this comment.
Foreground could set both UnitBrush and the ValueBrush, while the Accent is transparent, or something like that. Then setting the RadialGaugeAccentBrush can override the UnitBrush color.
There was a problem hiding this comment.
Sorry... I'm not following. How would RadialGaugeAccentBrush not be specified?
There was a problem hiding this comment.
As in not manually defined. Probably not important.
There was a problem hiding this comment.
It's defined in the ThemeDictionary
|
This looks great other than I'm not sure if not having a property for the Units brush is the right thing to do. Or, is it possible to use the foreground property for both but enable the resource brush to be used to modify them individually? The documentation should also be updated to reflect these changes. |
|
This approach is how theming is done within core controls. There isn't a property for every brush used, but instead resources that are used. The doc was updated to remove the old properties. Should we add a style and template section to the doc template with resource keys and descriptions? |
|
I would still expect Foreground to change the foreground of all the text elements, no? Yeah, there should be a way for developers to discover resource they can modify to customize the app. Style and template seems like the right section for that |
|
If foreground changes both elements then we lose functionality. |
|
If they can be individually modified with resources, the functionality would still there I would think? With this change, we will already be introducing breaking changes in 3.0 and asking the developer to use resources for one property anyway, why not for both. |
|
Ignore my last comment, this is not possible right now. Other than updating the docs and the bind file, it's good to go. |
|
Nice improvement! |
|
@skendrot, could you update the docs and we can merge this? |
|
This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1766 |
Issue: #1766
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The RadialGauge text does not update properly when the theme changes
PR Checklist
Please check if your PR fulfills the following requirements:
What is the new behavior?
The text will update when the theme changes from light-dark or dark-light
Does this PR introduce a breaking change?
Other information
Two properties were made obsolete with this change but still maintains functionality.