-
Notifications
You must be signed in to change notification settings - Fork 703
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
Add on press scale animation to carets #4949
Conversation
* Update spacing on edges * Clean code to remove changes * Small change reverted * Udpate expected result * Force failure for expected results * Fix carat to edge spacing with no test error * Revert tree view tests failures * Update verification files
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
</VisualState.Setters> | ||
<Storyboard RepeatBehavior="Forever"> | ||
<DoubleAnimationUsingKeyFrames Storyboard.TargetName="ScaleTransform" Storyboard.TargetProperty="ScaleX"> | ||
<DiscreteDoubleKeyFrame KeyTime="0:0:0.016" Value="{ThemeResource CalendarViewNavigationButtonScalePressed}"/> |
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.
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 don't think developers will want to modify this resource very often. You think it's worth moving it to theme dicts?
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.
Probably not. This seems fine for now.
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.
If that is true, I think it would be better to reference this by StaticResource as well. Then this wont be overridable at all, which might be okay, but the ThemeResource reference is likely more expensive than triplicating.
[Windows.UI.Xaml.Controls.ContentPresenter] | ||
Foreground=#E4000000 | ||
Padding=0,0,0,0 | ||
[Windows.UI.Xaml.Controls.Border] |
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.
How is CalendarView the only control with updated VisualVerification Files?
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.
only Calendar View button Template was changed - from ContentPresenter to TextBlock
Would there be any value in creating an AnimatedIcon with the Carets, which could be used with multiple controls? |
This might be a good idea, @StephenLPeters what do you think? |
@@ -165,16 +168,13 @@ | |||
<Setter Property="Template"> | |||
<Setter.Value> | |||
<ControlTemplate TargetType="Button"> | |||
<ContentPresenter | |||
x:Name="Text" | |||
<Border |
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.
@RBrid as FYI since he is also updating CalendarView
That sounds like a good idea. @beervoley do you want to file an issue and we can follow up with the design team on that ? For now lets not block on that. |
Description
This PR adds on press scale animation to icons using carets.
Motivation and Context
To indicate that the caret was actually pressed design decided that we need to add Scale to the icon.
How Has This Been Tested?
Manually.
Screenshots (if appropriate):
FlipView (as an example):