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

Add TweenSequenceEffect + test #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

esDotDev
Copy link
Contributor

@esDotDev esDotDev commented Nov 9, 2022

What this does

Allows users to more seamlessly use Flutters built in TweenSequence API to express complex multi-step tweens.

// fades in, out, and back in
foo.animate().tweenSequence(
   duration: 1.seconds,
   sequence: TweenSequence<double>([
     TweenSequenceItem(tween: Tween(begin: 0, end: 1), weight: .5),
     TweenSequenceItem(tween: Tween(begin: 1, end: 0), weight: .5),
     TweenSequenceItem(tween: Tween(begin: 0, end: 1), weight: .5),
   ]),
   builder: (_, double value, Widget child) {
     return Opacity(opacity: value, child: child);
   },
 )

Currently using TweenSequence is possile using a CustomEffect but requires slightly more boilerplate.

Why use a TweenSequence

  • Provides an alternative syntax to declaring complex tweens (as opposed to setting delays and using then(), or manually making calls on the animation controller), which may be easier to read and maintain
  • Avoids issues with stacking effects of the same type, which is inherent to the then or delay approach.

@esDotDev
Copy link
Contributor Author

esDotDev commented Nov 10, 2022

Just spit balling... We could further reduce this by providing some syntax sugar for

TweenSequence<double>([  
  TweenSequenceItem(tween: ..., weight: 1)
]

Could default to begin/end to 0/1, and weight to 1.

Going from this:

sequence: TweenSequence<double>([
     TweenSequenceItem(tween: Tween(begin: 0, end: 1), weight: .5),
     TweenSequenceItem(tween: Tween(begin: 1, end: 0), weight: .5),
     TweenSequenceItem(tween: Tween(begin: 0, end: 1), weight: .5),
   ]),

to:

items: <double>[
   EffectSequenceItem(), 
   EffectSequenceItem(begin: 1, end: 0), 
   EffectSequenceItem(), 
 ]),

Not sure it's worth the lift / abstraction layer, just a thought. The more verbose style is more flexible, as tween can be any Animatable, whereas the more terse version is really just allowing you to define sets of begin + end + weight.

[Edit] Thinking about it more, I think taking the entire TweenSequence probably feels about the right level of abstraction. Still very powerful/flexible, but fairly concise.

@gskinner
Copy link
Owner

Thoughts on just adding this to CustomEffect? I believe it's essentially identical right now, except the addition of the sequence param, which could be optional.

sequence?.evaluate(animation.value) ?? animation.value

@esDotDev
Copy link
Contributor Author

esDotDev commented Nov 14, 2022

I think I would slightly prefer the better semantics of seeing TweenSequenceEffect(...) in the tree, vs CustomEffect(...), but if that's the way you want to go, it should work fine.

One concrete benefit of this is that with TweenSequenceEffect the sequence can be required, then it can't mistakenly get removed later without no-one noticing, as would be possible on CustomEffect. Future dev would have to manually change the effect name from TweenSequenceEffect to CustomEffect to remove the sequence.

So it kinda reads a little better but is also just a little more robust / strict

@gskinner
Copy link
Owner

I don't have a strong opinion from a semantics perspective, but it's one less Effect class to maintain and update. :)

@esDotDev
Copy link
Contributor Author

esDotDev commented Nov 14, 2022

Yep. My preference would be to take the hit on the maintenance side, and have the better semantics, but all good either way. They certainly are very similar classes.

Just lmk what direction you want to go in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants