Refactor: Migrate page transition builder class to widgets #174321
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PageTransitionsBuilder class by moving it from packages/flutter/lib/src/material/page_transitions_theme.dart to a new file packages/flutter/lib/src/widgets/page_transitions_builder.dart. This is a good architectural improvement, placing the general-purpose builder in a lower-level library. The changes are clean and the existing tests should cover this refactoring. I've added a couple of suggestions to improve the documentation in the new file to better reflect its new, more generic location.
| /// Used by [PageTransitionsTheme] to define a [MaterialPageRoute] page | ||
| /// transition animation. | ||
| /// | ||
| /// Apps can configure the map of builders for [ThemeData.pageTransitionsTheme] | ||
| /// to customize the default [MaterialPageRoute] page transition animation | ||
| /// for different platforms. | ||
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [FadeUpwardsPageTransitionsBuilder], which defines a page transition | ||
| /// that's similar to the one provided by Android O. | ||
| /// * [OpenUpwardsPageTransitionsBuilder], which defines a page transition | ||
| /// that's similar to the one provided by Android P. | ||
| /// * [ZoomPageTransitionsBuilder], which defines the default page transition | ||
| /// that's similar to the one provided in Android Q. | ||
| /// * [CupertinoPageTransitionsBuilder], which defines a horizontal page | ||
| /// transition that matches native iOS page transitions. | ||
| /// * [FadeForwardsPageTransitionsBuilder], which defines a page transition | ||
| /// that's similar to the one provided by Android U. |
There was a problem hiding this comment.
Since PageTransitionsBuilder is now in the widgets library, its documentation should be more generic and not solely focused on its usage within the material library.1
I suggest updating the class documentation to first provide a general description and then specify its use in material. Also, adding PageTransitionsTheme and MaterialPageRoute to the "See also" section would provide better context for developers.
/// Defines a page transition animation for a [PageRoute].
///
/// This class is used by [PageTransitionsTheme] to define a [MaterialPageRoute]
/// page transition animation. Apps can configure the map of builders for
/// [ThemeData.pageTransitionsTheme] to customize the default
/// [MaterialPageRoute] page transition animation for different platforms.
///
/// See also:
///
/// * [PageTransitionsTheme], which uses this class to configure page transitions.
/// * [MaterialPageRoute], which uses this class to build its transition.
/// * [FadeUpwardsPageTransitionsBuilder], which defines a page transition
/// that's similar to the one provided by Android O.
/// * [OpenUpwardsPageTransitionsBuilder], which defines a page transition
/// that's similar to the one provided by Android P.
/// * [ZoomPageTransitionsBuilder], which defines the default page transition
/// that's similar to the one provided in Android Q.
/// * [CupertinoPageTransitionsBuilder], which defines a horizontal page
/// transition that matches native iOS page transitions.
/// * [FadeForwardsPageTransitionsBuilder], which defines a page transition
/// that's similar to the one provided by Android U.Style Guide References
Footnotes
| /// Wraps the child with one or more transition widgets which define how [route] | ||
| /// arrives on and leaves the screen. | ||
| /// | ||
| /// The [MaterialPageRoute.buildTransitions] method looks up the | ||
| /// current [PageTransitionsTheme] with `Theme.of(context).pageTransitionsTheme` | ||
| /// and delegates to this method with a [PageTransitionsBuilder] based | ||
| /// on the theme's [ThemeData.platform]. |
There was a problem hiding this comment.
The documentation for buildTransitions is very specific to its usage in MaterialPageRoute. To improve clarity and adhere to better layering, I suggest making the documentation more general first, explaining the method's purpose, and then providing the MaterialPageRoute usage as an example.1
/// Wraps the child with one or more transition widgets which define how [route]
/// arrives on and leaves the screen.
///
/// Subclasses override this method to create a transition animation.
///
/// The [MaterialPageRoute.buildTransitions] method is an example of a method
/// that uses this to build a transition. It looks up the current
/// [PageTransitionsTheme] with `Theme.of(context).pageTransitionsTheme`
/// and delegates to this method with a [PageTransitionsBuilder] based
/// on the theme's [ThemeData.platform].Style Guide References
Footnotes
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
I would avoid design-language-specific documentation and references (Material, Theme and ThemeData etc). Try to make this as standalone and as design-language-agnostic as possible. Instead, move the Material-specific documentation to the relevant Material widgets.
I asked offline and found this is not entirely true. Referencing |
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
The documentation suggestions from gemini look like a good place to start.
Also are there tests for this class that can be moved here? I don't think we want to add a new file + class in widgets/ without some sort of test.
rkishan516
force-pushed
the
page-transitions-builder
branch
from
0fc4d18 to
b74aecf
Compare
|
I don't think there are tests for |
A test could make a mock transitions builder that extends |
rkishan516
force-pushed
the
page-transitions-builder
branch
from
b74aecf to
21cee4d
Compare
|
Approach with test so far is looking good. Can you adopt the gemini doc recommendations and fix the failing tests in CI? |
rkishan516
force-pushed
the
page-transitions-builder
branch
2 times, most recently
from
8a2a6e8 to
37a9080
Compare
rkishan516
force-pushed
the
page-transitions-builder
branch
from
37a9080 to
907ca6f
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
Looking good so far, one final comment asking for more documentation.
rkishan516
force-pushed
the
page-transitions-builder
branch
from
907ca6f to
313d4b2
Compare
|
@victorsanni I have updated the paragraph, please have a look. |
| scale: Tween<double>( | ||
| begin: 0.5, | ||
| end: 1.0, | ||
| ).animate(CurvedAnimation(parent: animation, curve: Curves.easeInOut)), |
There was a problem hiding this comment.
this might be causing a memory leak, consider using CurveTween or something similar instead.
| /// in any design system. Custom [PageRoute] subclasses can accept a | ||
| /// PageTransitionsBuilder and delegate to its [buildTransitions] method when | ||
| /// overriding [ModalRoute.buildTransitions]. This enables reusable transition | ||
| /// animations that work with [Navigator] and other navigation primitives without |
There was a problem hiding this comment.
Not sure we need to explicitly say "without requiring Material Design components". The idea is the class should be understandable as a standalone concept for developers who don't even know Material exists.
There was a problem hiding this comment.
So this should precede the paragraph on PageTransitionsTheme, while that paragraph should be prefixed with an annotation that it is an example of how this class can be used.
There was a problem hiding this comment.
Yup, Make sense.
There was a problem hiding this comment.
I have pushed snipped directly in doc, right now. Do you feel, we should create a example file for this?
rkishan516
force-pushed
the
page-transitions-builder
branch
2 times, most recently
from
383ab98 to
d3c5c97
Compare
github-actions
bot
added
d: api docs
rkishan516
force-pushed
the
page-transitions-builder
branch
from
d3c5c97 to
69bd510
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
To fix linux analyze, run dart format packages/flutter/test/widgets/page_transitions_builder_test.dart
rkishan516
force-pushed
the
page-transitions-builder
branch
from
3872487 to
04b7698
Compare
MitchellGoodwin
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
…74321) Changes * Move PageTransitionsBuilder from `material/page_transitions_theme.dart` to `widget/page_transitions_builder.dart` part of: flutter#172929 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…74321) Changes * Move PageTransitionsBuilder from `material/page_transitions_theme.dart` to `widget/page_transitions_builder.dart` part of: flutter#172929 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…74321) Changes * Move PageTransitionsBuilder from `material/page_transitions_theme.dart` to `widget/page_transitions_builder.dart` part of: flutter#172929 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
3 participants
Changes
material/page_transitions_theme.darttowidget/page_transitions_builder.dartpart of: #172929
Pre-launch Checklist
///).