Skip to content
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

Merged
merged 6 commits into from
Jun 2, 2021
Merged

Update to Clock styles to follow spec #2338

merged 6 commits into from
Jun 2, 2021

Conversation

StefanoRivolta-Previero
Copy link
Contributor

Fixes #2336
Fixes #2317

Summary

  • Adds four new styles to reflect the updated MaterialDesign specs: https://material.io/components/time-pickers
  • Adds a Property to disable the header in all styles
  • Fixes the animations for selecting from seconds to minutes and from hours to seconds

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

FSV2XKDuAr
J0yevVE8mA
2VtNv1KFmX
yOrDuK2LYK

Copy link
Member

@Keboo Keboo left a 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.

Comment on lines 764 to 765
<Setter Property="Height" Value="auto"/>
<Setter Property="Width" Value="auto"/>
Copy link
Member

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?

Copy link
Contributor Author

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=":"
Copy link
Member

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.

Copy link
Contributor Author

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: : }

Copy link
Contributor Author

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" />

immagine

</TextBlock>
</Grid>

<TextBlock Text=":" Style="{StaticResource TimeTextBlock}"
Copy link
Member

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=":"
Copy link
Member

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.

Comment on lines 2950 to 2951
<Setter Property="Height" Value="auto"/>
<Setter Property="Width" Value="auto"/>
Copy link
Member

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=":"
Copy link
Member

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.

@Keboo Keboo added this to the 4.2.0 milestone May 29, 2021
@Keboo Keboo added enhancement release notes Items are likely to be highlighted in the release notes. labels May 29, 2021
@Keboo Keboo added the visual breaking change Items here have affected the visual look of controls. label Jun 2, 2021
@Keboo Keboo merged commit 2006220 into MaterialDesignInXAML:master Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release notes Items are likely to be highlighted in the release notes. visual breaking change Items here have affected the visual look of controls.
Projects
None yet
2 participants