-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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 tests for tween_animation_builder.0.dart API example. #148902
Add tests for tween_animation_builder.0.dart API example. #148902
Conversation
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. I think it might be prudent to add an additional test that the reverse animation is triggered immediately the icon is tapped during the animation (not just at the end), and vice versa. I'm not sure if that's already covered here.
Thanks for writing these tests!
Hi @victorsanni, thanks for the review! |
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.
), | ||
), | ||
); | ||
} | ||
} | ||
|
||
class TweenAnimationBuilderExample extends StatefulWidget { | ||
const TweenAnimationBuilderExample({super.key}); | ||
const TweenAnimationBuilderExample({ | ||
required this.duration, |
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.
Maybe make this optional, and provide a default duration of 1 second?
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.
@gspencergoog thanks for the review!
In the recent commit I updated the duration to be optional and provided a default value.
Could you please take a look again?
303113f
to
64fc8a2
Compare
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #148902 at sha 8fd218b3e208da75cfc8b2adf8d3c0c85edc8c99 |
8fd218b
to
607ea31
Compare
@@ -25,28 +29,35 @@ class TweenAnimationBuilderExampleApp extends StatelessWidget { | |||
} | |||
|
|||
class TweenAnimationBuilderExample extends StatefulWidget { | |||
const TweenAnimationBuilderExample({super.key}); | |||
const TweenAnimationBuilderExample({ | |||
this.duration = TweenAnimationBuilderExampleApp.duration, |
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 you can just hard code const Duration(seconds: 1)
here as the default. Having it be a separate static reference doesn't add anything to the example, and people will replicate it, which isn't necessarily desirable. Instead of referencing the static in the test, just hardcode it there too.
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.
@gspencergoog got it, thanks for the suggestion!
auto label is removed for flutter/flutter/148902, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
a9f778e
to
e888493
Compare
@victorsanni @gspencergoog thanks a lot for the review! |
…8902) This PR contributes to flutter#130459 ### Description - Adds tests for `examples/api/lib/widgets/tween_animation_builder/tween_animation_builder.0.dart`
This PR contributes to #130459
Description
examples/api/lib/widgets/tween_animation_builder/tween_animation_builder.0.dart
Pre-launch Checklist
///
).