-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added a way to modify the Easing Function used in animations (issue #500) #507
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
Conversation
|
Hi @hig-ag, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
Hello there is no special skill to add a mention about your newly parameter in the doc (you already have written the text in the xmldoc btw) |
|
(beside that, great PR 👍 ) |
|
Build has failed on this, please run '.\build.ps1 UpdateHeaders' and commit the changes. |
|
Up? In order to get this one for 1.2 we need to merge it before end of next week |
| /// </summary> | ||
| public static partial class AnimationExtensions | ||
| { | ||
| private static readonly EasingFunctionBase _defaultStoryboardEasingFunction = new CubicEase(); |
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.
What do you think about having a public default easing function that can be the default unless otherwise specified?
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 think that it would be nice but that would maybe decrease the ease of use of the toolkit. In the current form the user doesn't have to know the internal things like EasingFunctionBase and so on.
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 are right. I was thinking of having an EasingType (or InterpolationType depending on what you name it) that is public with a default value
| return result; | ||
| } | ||
|
|
||
| private static EasingFunctionBase GetEasingFunction(InterpolationType interpolationType) |
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 only works for storyboards. Can you also add support for composition animations, like blur? You will most likely need to have two functions, one for composition and one for storyboards
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.
The composition animations only support Cubic Bezier, and Step easing functions:
https://msdn.microsoft.com/en-us/windows/uwp/graphics/composition-animation#easing-functions
So the implementation is a little bit difficult.
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.
@shenchauhan might be able to help with this
| return result; | ||
| } | ||
|
|
||
| private static EasingFunctionBase GetEasingFunction(InterpolationType interpolationType) |
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.
Also, does it make sense to make this function public? This way animation extensions outside of the toolkit can take advantage of it.
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 question is addressed to me: I really don't know.
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 think it safe to make the functions public
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.
agree with Nikola
docs/animations/Fade.md
Outdated
|
|
||
| [Fade Behavior Sample Page Source](https://github.com/Microsoft/UWPCommunityToolkit/tree/master/Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Fade) | ||
|
|
||
| ## InterpolationType |
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.
Just a question, why did you decide to go with InterplationType vs EasingType for the naming? EasingType seems more approachable, but that might be just me.
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 thought that a linear interpolation can't be an easing function but I'm wrong. I think that both names are good but maybe EasingType is more suitable because the code and the documentation uses the word "easing". Note: I'm not a native English speaker.
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.
Let's see if anyone else has an opinion, but I'm for changing it to easing type to make it more obvious what it is.
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'm for easing as well
|
ping |
|
Up? Last call before I close the PR |
…ngFunction() public
|
@nmetulev is that ok for you? |
|
By the way, I signed the contribution license agreement but the label didn't disappear. |
|
Can you try it again? |
|
@hig-ag, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
This looks great. The work still needs to be done to support Composition easing functions however, but I'm ok with that being a separate PR as long as we have it in the documentation that easing functions only work when using Storyboard mode. |
|
Thank you very much! |
This commit adds a way to modify the easing function used in animations using an optional enum type which is named "InterpolationType" (See issue #500). I tried to document the things right but I don't have much experience in documenting. So please be considerate. I only documented in code with the xml-tags. I hope that someone else can make the proper documentation in (https://github.com/Microsoft/UWPCommunityToolkit/tree/dev/docs) and modify the UWP Toolkit Samples app to show off the different easing functions.
Info: https://msdn.microsoft.com/en-us/windows/uwp/graphics/composition-animation#easing-functions