Remove Material import from navigator_on_did_remove_page_test#184776
Remove Material import from navigator_on_did_remove_page_test#184776xfce0 wants to merge 1 commit intoflutter:masterfrom
Conversation
Part of flutter#177414. Replaces MaterialPage with a local _TestPage helper and MaterialPageRoute with PageRouteBuilder, removing the unnecessary dependency on package:flutter/material.dart. Also removes the file from the knownWidgetsCrossImports allowlist.
There was a problem hiding this comment.
Code Review
This pull request removes a test file from the cross-import checker and updates the navigator test to use a custom _TestPage implementation instead of MaterialPage. The review feedback suggests adding documentation to the new helper class and ensuring consistent animation durations in the PageRouteBuilder for test performance.
| class _TestPage extends Page<void> { | ||
| const _TestPage({required super.key, required this.child}); | ||
|
|
||
| final Widget child; |
There was a problem hiding this comment.
According to the Flutter style guide, all members (including private ones) should have public-quality documentation using ///. Adding documentation helps explain the purpose of this test helper class and its fields.
/// A [Page] implementation used for testing the [Navigator].
class _TestPage extends Page<void> {
/// Creates a [_TestPage].
const _TestPage({required super.key, required this.child});
/// The widget to display as the content of the page.
final Widget child;References
- Use
///for public-quality documentation, even on private members. (link)
| PageRouteBuilder<void>( | ||
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | ||
| return const Text('new page'); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
For consistency with _TestPage and to ensure the test remains fast and deterministic, consider setting transitionDuration and reverseTransitionDuration to Duration.zero here as well. This avoids unnecessary animation delays during the test execution.
| PageRouteBuilder<void>( | |
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | |
| return const Text('new page'); | |
| }, | |
| ), | |
| PageRouteBuilder<void>( | |
| transitionDuration: Duration.zero, | |
| reverseTransitionDuration: Duration.zero, | |
| pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) { | |
| return const Text('new page'); | |
| }, | |
| ), |
Part of #177414.
Replaces
MaterialPagewith a local_TestPagehelper andMaterialPageRoutewithPageRouteBuilder, removing the unnecessary dependency onpackage:flutter/material.dartfrom a widgets-layer test.Also removes the file from the
knownWidgetsCrossImportsallowlist indev/bots/check_tests_cross_imports.dart.Pre-launch Checklist
///).