-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[PageTransitionsBuilder] Fix 'ZoomPageTransition' built more than once #58686
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
[PageTransitionsBuilder] Fix 'ZoomPageTransition' built more than once #58686
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The forward transition looks exactly the same as before, but there's still some tricky problems in the reverse one and I cannot figure it out. Please help me to implement this more elegantly if you can. /cc @shihaohong |
|
Oh there're some comments copied from |
|
I'll try to take a look later today or tomorrow. However, if I don't get the chance to, I might disappear for about a week due to personal reasons. In the event that I don't get to it this week, I'll ask someone else on the team to take a look. Thank you for putting this together and doing the investigation! |
|
When importing the
|
For now the problems has been solved and this animation is absolutely the same as before! 🎉 I'll add built times test case ASAP. |
Test: _ZoomPageTransition only cause child widget built once
|
Test has been added, this PR is ready for review. |
|
The analyzer is complaining about trailing whitespaces, so those'll need to be addressed |
This test addressed some import issue that I cannot recognize. 😕 |
|
Oops, I misread the error message. It's basically complaining about this line and that it is a recursive dependency |
|
Working on the review, please wait for a few minutes. |
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 modulo the constructor API doc comment
68acb4d to
26345f1
Compare
|
Seems all resolved now, is it good to go? |
|
You still need to address @Hixie's comment right? Sorry, I think my wording might have been confusing. I was saying that this change you suggested looks okay: Unless there's something else that needs to be described in the constructor that I might be missing |
|
Oh that is because I dropped one of the commit 68acb4d which is included. 😢 |
It's my bad, done it. |
|
Tree is green now. 👀 I'm wondering whether the failed test matter which is a timeout error? |
|
@AlexVincent525 It seems one of the test suites timed out. I restarted it and also applied the Thank you so much for this contribution, it was a lot of work and I'm glad we got through it :) |
…than once (flutter#58686)" (flutter#59992) This reverts commit fe15d1e.
…lt more than once (flutter#58686)" (flutter#59992)" (flutter#60245)
Description
This will solve the issue about the
_ZoomPageTransitioncause the widget built three times, and imports theDualTransitionBuilderfrom the animations package.Related Issues
Related to #58345 .
Tests
tests/material/page_transition_theme_test.dart_ZoomPageTransition only cause child widget built onceChecklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read [Handling breaking changes].