-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor StandardPage and Add Support for Flutter 3.38.0 #49
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
Conversation
|
Although there are many file changes, most of them are due to running dart format in response to new Dart linter rules. |
|
|
||
| if (tRouteInformation != null) { | ||
| delegate?.routeWithConfiguration(tRouteInformation); | ||
| delegate?.routeWithConfiguration(tRouteInformation, null, pushParentPage); |
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.
documentation for pushParentPage
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.
I have updated the documentation.
| void route( | ||
| String location, [ | ||
| StandardPageNavigationMode? navigationMode, | ||
| bool pushParentPage = true, |
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.
documentation for pushParentPage
| } | ||
|
|
||
| /// Pops the Navigator of [StandardAppApp.navigator]. | ||
| /// maybePop the Navigator of [StandardAppApp.navigator]. |
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.
'Attempts to pop the Navigator.....'
| _sendPageDataEvent(); | ||
| (Router.of(context).routerDelegate as StandardRouterDelegate?) | ||
| ?._updatePages(); | ||
| getApp().standardAppPlugin.delegate?._updatePages(); |
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.
Does this work inside a test environment? Before getApp() couldn't be used in unit tests.
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.
You are right that retrieving it via the context is safer, so I have made that change.
|
|
||
| if (_active != tIsCurrentlyActive) { | ||
| fUpdateNestedPageActiveStatus() { |
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.
Don't need a void return type?
| key: nestedNavigatorKey, | ||
| pages: _pageInstances, | ||
| observers: [ | ||
| ...getApp().navigatorObservers, |
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.
Here too, does this work in a unit test environment?
|
|
||
| final tCallbackList = <(RegExp, Object? Function(RegExpMatch, Uri))>[]; | ||
| for (var tLink in tLinkMap.entries) { | ||
| final tRegExp = RegExp( |
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.
This won't practically work...
One major reason we use RegExp is so the user can make links like (\w+). In other words, they are capturing pieces of the paths. Since RegExp keeps track of these in sequential order... if patapata is auto generating the RegExp, the order of the captures will be wrong from the view of the user.
If there's no other way to support this at all, then at least we need to make it very clear in documentation that a user is expected to manually keep track of the parent/child relationships in the links, and when you change a parent link, it will affect children links's code as well.
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.
I added restrictions to the regular expressions used for deep links and embedded assertions to allow only named capture groups.
| List<StandardPageInterface> get pageInstances { | ||
| final tInstances = <StandardPageInterface>[]; | ||
|
|
||
| fPushNestedPages(StandardPageInterface page) { |
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.
Need a void here too?
| @@ -1428,53 +2305,122 @@ class StandardRouterDelegate extends RouterDelegate<StandardRouteData> | |||
|
|
|||
| /// {@template patapata_widgets.StandardRouteDelegate.pageInstances} | |||
| /// The current [Page] history. | |||
| /// | |||
| /// Returns a flattened list of all [Page] instances, including both root pages | |||
| /// and nested pages within nested navigators, collected recursively in depth-first order. | |||
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.
Isn't this not depth-first order? Doesn't depth-first mean the deepest is first? Perhaps it's a bit unambiguous so let's maybe change it to say simply, 'collected recursively in order of addition'.
| Widget tChild = Navigator( | ||
| key: _navigatorKey, | ||
| restorationScopeId: 'StandardAppNavigator', | ||
| observers: [ | ||
| ...context.read<App>().navigatorObservers, | ||
| _rootNavigatorObserver, | ||
| ...getApp().navigatorObservers, |
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.
once again, can we use getApp here?
…he StandardPageFactory documentation for details.
…set Route's implicitAppBarDismissal to true.
Summary
Adds a parent–child relationship to StandardPage, allowing the parent page to be automatically pushed when navigating to a child page.
Introduces a StandardPage that contains a nested navigator. This is useful for applications with footer/tab menus where each tab maintains its own navigation stack.
This callback is forwarded to the corresponding Navigator property.
As a result, the Patapata monorepo is now managed using the new Pub workspaces