Skip to content

Conversation

@AlexV525
Copy link
Member

@AlexV525 AlexV525 commented Jun 4, 2020

Description

This will solve the issue about the _ZoomPageTransition cause the widget built three times, and imports the DualTransitionBuilder from the animations package.

Related Issues

Related to #58345 .

Tests

  • tests/material/page_transition_theme_test.dart _ZoomPageTransition only cause child widget built once

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 4, 2020
@fluttergithubbot
Copy link
Contributor

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.

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 4, 2020

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

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 4, 2020

Oh there're some comments copied from FadeThroughTransition, I'll reword them in next commit. 😃

@shihaohong
Copy link
Contributor

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!

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 5, 2020

When importing the DualTransitionBuilder, the TransitionBuilder has already defined in src/widgets/framwork.dart, which is different the one in dual_transition_builder.dart: Widget Function(BuildContext context, Animation<double> animation, Widget child). So I changed its name to AnimatedTransitionBuilder.

typedef TransitionBuilder = Widget Function(BuildContext context, Widget child);

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 5, 2020

but there's still some tricky problems in the reverse one and I cannot figure it out.

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
@AlexV525
Copy link
Member Author

AlexV525 commented Jun 5, 2020

Test has been added, this PR is ready for review.

@shihaohong
Copy link
Contributor

The analyzer is complaining about trailing whitespaces, so those'll need to be addressed

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 5, 2020

The analyzer is complaining about trailing whitespaces, so those'll need to be addressed

This test addressed some import issue that I cannot recognize. 😕

@shihaohong
Copy link
Contributor

Oops, I misread the error message. It's basically complaining about this line and that it is a recursive dependency

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 5, 2020

Working on the review, please wait for a few minutes.

Copy link
Contributor

@shihaohong shihaohong left a 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

@AlexV525 AlexV525 force-pushed the fix-zoom-page-transition-build branch from 68acb4d to 26345f1 Compare June 16, 2020 10:19
@AlexV525
Copy link
Member Author

Seems all resolved now, is it good to go?

@shihaohong
Copy link
Contributor

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:

/// Creates a [_ZoomPageTransition].
///
/// The [animation] and [secondaryAnimation] argument are required and must
/// not be null.

Unless there's something else that needs to be described in the constructor that I might be missing

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 16, 2020

Oh that is because I dropped one of the commit 68acb4d which is included. 😢

@AlexV525
Copy link
Member Author

Sorry, I think my wording might have been confusing.

It's my bad, done it.

@AlexV525
Copy link
Member Author

AlexV525 commented Jun 17, 2020

Tree is green now. 👀 I'm wondering whether the failed test matter which is a timeout error?

@shihaohong
Copy link
Contributor

@AlexVincent525 It seems one of the test suites timed out. I restarted it and also applied the waiting for tree to green label, which will merge the PR once all checks are green.

Thank you so much for this contribution, it was a lot of work and I'm glad we got through it :)

@fluttergithubbot fluttergithubbot merged commit fe15d1e into flutter:master Jun 17, 2020
@AlexV525 AlexV525 deleted the fix-zoom-page-transition-build branch June 18, 2020 09:20
renyou added a commit that referenced this pull request Jun 22, 2020
@AlexV525 AlexV525 restored the fix-zoom-page-transition-build branch June 23, 2020 06:32
renyou added a commit that referenced this pull request Jun 23, 2020
shihaohong pushed a commit that referenced this pull request Jun 24, 2020
fluttergithubbot pushed a commit that referenced this pull request Jun 25, 2020
@AlexV525 AlexV525 deleted the fix-zoom-page-transition-build branch June 27, 2020 08:46
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants