[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. |
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].