-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add reverse functionality to repeat() of AnimationController #25125
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 reverse functionality to repeat() of AnimationController #25125
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
|
@HansMuller Can you review this? |
|
@goderbauer Looks like a nice enhancement. Will review this week. |
HansMuller
left a comment
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 PR is pretty close to the finish line! Just a few more tweaks, mostly for the corner cases.
|
@HansMuller Reworked the PR again with a nicer implementation of the repeating mechanism. Would be cool if you could have a look 😃 |
HansMuller
left a comment
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.
LGTM
If you could add just one last comment to the test, I can land this for you.
This is a nice new animation feature. Thanks for the contribution and thanks for hanging in there!
| expect(controller.value, 0.0); | ||
|
|
||
| controller.repeat(reverse: true); | ||
| tick(const Duration(milliseconds: 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.
It's not clear why this first duration=0 tick is always needed. I assume it's to enable the simulation to figure out which way it's going. Might be helpful to include one comment at the beginning here, to explain what's going 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.
This can also be found in other tests (e.g. test('animateTo sets correct status') or test('after a reverse call animateTo sets correct status'). Every time when starting a new simulation, the first tick is used to initialise the simulation inside the AnimationController. When the line is removed, the elapsed time will always be 0 which means that the simulation will be evaluated at the time 0 and not after x milliseconds.
In the end I didn't comment on that because it is used quite often in the other tests already 😉
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.
Good point, OK.
…r into add_reverse_to_repeat
This PR adds a reverse flag to the
repeat()method of the AnimationController. With that feature it will be easier to create 'yoyo' animations like a glowing Flutter logo:AnimationController(vsync: this, duration: Duration(seconds: 1))..repeat(reverse: true);Also have a look at the following Stackoverflow question: https://stackoverflow.com/questions/49604221/how-to-do-yoyo-animation-in-flutter
So right now it's only possible to achieve this by listening to the current status of the AnimationController and alternating between calling
forward()andreverse()or through combining a fade-in animation with a fade-out animation.