RatingControl: Fix visibility issues of unselected stars in new style #3930
Conversation
ghost
added
the
|
@YuliKl Do we also need to update the caption foreground color? In it's current state it is definitely not adhering to a11y standards. |
Yes, thank you for catching that! Will find the correct new color momentarily. |
|
@chingucoding the caption text should also use |
|
Thank you @YuliKl. I updated the template to set the caption foreground appropriately (screenshots updated at the top of the PR). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
StephenLPeters
added
area-RatingControl
team-Controls
dev/RatingControl/RatingControl.xaml
Outdated
| @@ -52,7 +52,10 @@ | |||
|
|
|||
| <StackPanel Orientation="Horizontal" Grid.Row="0" Margin="-20,-20,-20,-20"> | |||
| <StackPanel x:Name="RatingBackgroundStackPanel" Orientation="Horizontal" Background="Transparent" Margin="20,20,0,20"/> | |||
| <TextBlock x:Name="Caption" Height="32" Margin="4,9,20,0" TextLineBounds="TrimToBaseline" Style="{ThemeResource CaptionTextBlockStyle}" VerticalAlignment="Center" AutomationProperties.AccessibilityView="Raw" AutomationProperties.Name="RatingCaption" IsHitTestVisible="False" Text="{TemplateBinding Caption}"/> | |||
| <TextBlock x:Name="Caption" Height="32" Margin="4,9,20,0" TextLineBounds="TrimToBaseline" Style="{ThemeResource CaptionTextBlockStyle}" | |||
| Foreground="{ThemeResource TextFillColorSecondaryBrush}" | |||
There was a problem hiding this comment.
A new static resource can be created for this.
There was a problem hiding this comment.
Given that this TextBlock uses the CaptionTextBlockStyle would it be even better to alter that style? The only issue is that we would need to put that into WinUI as it currently isn't in the WinUI 2 codebase I think.
There was a problem hiding this comment.
@YuliKl What do you think? Would it be better to adjust the CaptionTextBlockStyle or just fix this on the RatingControl here?
There was a problem hiding this comment.
Just noticed we already have a brush for this, updated that now. Ignore my last two comments.
There was a problem hiding this comment.
The color value shouldn't be part of CaptionTextBlockStyle, that style primarily captures the correct font size (and a few other incidental properties). So I don't think it makes sense to modify CaptionTextBlockStyle for this particular scenario.
You could make a RatingControlCaptionStyle based on CaptionTextBlockStyle. Not sure if the overhead is worth it but maybe that's what @karenbtlai is suggesting?
There was a problem hiding this comment.
The issue was that at the top of RatingControl, we set the foreground color to RatingControlCaptionForeground which had a faulty value. Changing that brushes value to the specified value fixed the issue. CaptionTextBlockStyle should be fine. Sorry about the confusion.
There was a problem hiding this comment.
I apologize for the ambiguity in my comments. I meant the referenced ThemeResource in Foreground can reference a new static resource rather than the brush directly.
There was a problem hiding this comment.
Oh I see. Is the current setup using the RatingControlCaptionForeground fine?
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Switches the unset glyph and updates the unset foreground color.
Motivation and Context
Closes #3925
How Has This Been Tested?
Screenshots (if appropriate):