-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Update to Clock styles to follow spec #2338
Update to Clock styles to follow spec #2338
Conversation
Add IsHeaderVisibile Property
increased the width of the clock hand to make it more visible when themed
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.
This is really great work. I appreciate all of the work you have put in on this. Just a couple comments than we can get it merged.
<Setter Property="Height" Value="auto"/> | ||
<Setter Property="Width" Value="auto"/> |
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.
Are these setters needed? Aren't these the default values for the properties?
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.
You're right, not needed. In the current implementation there are hardcoded dimensions, and I was experimenting with leaving it on auto by default.
</TextBlock> | ||
</Grid> | ||
|
||
<TextBlock x:Name="PART_SecondColonPrefix" Text=":" |
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.
It would be good to use a string format specifier to get a localized version of the time separator.
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.
I left it like it was in the current version tbh, didn't even think of it. I tried right now to make it culture responsive, but I'm having some trouble.
I've had the best luck by using StringFormat on the DateTime property, but it doesn't really work like I'd like to. This doesn't work:
Text="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=Time, Mode=OneWay, StringFormat={}{0::}}"
It seems like at least two characters are required for StringFormat to work. Adding a leading or trailing space seems to work though.
On the internet there doesn't seem to be much about this issue. Do you know of something I'm not aware of? Otherwise the best solution I've come to would be to add both a leading and trailing whitespace, which would result in only slightly larger space taken up by the separators. Essentially like this:
StringFormat={}{0: : }
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.
This implementation seems to work pretty ok. Please let me know if you think a better one could be done
<TextBlock Text="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=Time, Mode=OneWay, StringFormat={}{0: : }}"
Style="{StaticResource TimeTextBlock}"
Foreground="{DynamicResource MaterialDesignBody}"
Margin="-6 0 -6 0"
VerticalAlignment="Center" />
</TextBlock> | ||
</Grid> | ||
|
||
<TextBlock Text=":" Style="{StaticResource TimeTextBlock}" |
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.
Same comment here. Would be good to use a string format specifier to get a localized string.
</TextBlock> | ||
</Grid> | ||
|
||
<TextBlock x:Name="PART_SecondColonPrefix" Text=":" |
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.
Same comment here. Would be good to use a string format specifier to get a localized string.
<Setter Property="Height" Value="auto"/> | ||
<Setter Property="Width" Value="auto"/> |
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.
Are these Setters needed? I believe auto is the default.
</TextBlock> | ||
</Grid> | ||
|
||
<TextBlock x:Name="PART_SecondColonPrefix" Text=":" |
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.
Same comment here. Would be good to use a string format specifier to get a localized string.
Clean up code
Fixes #2336
Fixes #2317
Summary
Notes
The new styles work both with standlalone Clock and with TimePicker (using ClockStyle). Same for the header visibility.
Could use some help with the naming of the styles tbh. Also I'm not sure if this is the right way of doing it, ie making multiple styles vs making properties. I felt properties would have been quite convoluted in this case.
I'm also quite new to the whole git thing, so if I'm doing anything wrong, please let me know.
Screenshots