Closed esDotDev closed 2 years ago
I considered that, but I don't believe it's worth the complexity. You can achieve what you're after today by simply wrapping the widget in the sub-route and not in the parent route (super route?).
So like:
GoRoute(path: '/app',
routes: [
GoRoute(path: '/messages',
routes: [
GoRoute(path: '/inbox', pageBuilder: (_, _) => AppMenu(child: MessagesMenu(child: Inbox()))),
GoRoute(path: '/outbox', pageBuilder: (_, _) => AppMenu(child: MessagesMenu(child: Outbox()))),
]
)
]
)
Ya it works, it's kinda cludgy. As my tree grows, moving things around is a lot harder. If I want to change some properties of the Menu, I'm pasting it N number of times, basically it's not DRY.
Regarding, complexity I think it's just a few lines of recursive build calls. I was planning on putting together a PR for this sometime soon, so maybe you can look at the code and see if it's as bad as you think?
The other thing to consider is, we can have imperative logic in these sub-builders, just like in the root-level navigatorBuilder, and it lets that logic live next to the routes it is concerned with. Which the inline method doesn't really support well.
Ya it works, it's kinda cludgy. As my tree grows, moving things around is a lot harder. If I want to change some properties of the Menu, I'm pasting it N number of times, basically it's not DRY.
Gosh, if only the Dart language itself had a way to package reusable code that you could leverage for this situation... (he said, sarcastically : )
Regarding, complexity I think it's just a few lines of recursive build calls. I was planning on putting together a PR for this sometime soon, so maybe you can look at the code and see if it's as bad as you think?
It's a slippery slope, man. I don't want to add anything to GR that you can already do in Flutter w/o my help.
Sure you could make config widgets purely for the purposes of building specific sets of widgets, like:
class AppMenuWithMessagesMenu {
AppMenuWithMessagesMenu(this.child);
final Widget child;
...
}
class AppMenuWithHomeMenu {}
class AppMenuWithMessageMenuWithAdminMenu {}
but yuck :p I think canonical flutter way here is nested builders for sure.
perhaps a good ol' fashioned parameterized function would do the trick?
Named functions is better than having to declare a class, but still objectively cludgy compared to a builder.
Also, thinking about it some more, I'm not sure you actually can get the same behavior from wrapping an inner widget.
Navigator
and do not navigate with the route. So there is a fundamental difference here. Pretty sure any arguments made against this, can effectively be made against navigatorBuilder
.
The navigatorBuilder
function allows you to create a widget above the Navigator
. The builder
function would just let you factor things differently when you already have three ways to factor:
Regardless, each one of these would be wrapped in a Page<dynamic>
and hosted by the Navigator
.
I think I'm seeing our disconnect. This proposal is for allowing multiple widgets above navigator, in cases where you don't want a "single wrapping scaffold", you instead want multiple wrapping scaffolds or just different scaffolds for different sets of sub-routes.
For the same reason we would want AppMenu
above the navigator, we would want MessagesMenu
above it (ie, to preserve it's state easily, and avoid being animated away when changing page withn the same sub-section)
The desired widget structure with my example code is something like:
AppMenu(
child: MessagesMenu(
child: Navigator(
pages: [
...
]
)
)
)
Not:
AppMenu(
child: Navigator(
pages: [
Page(child: MessagesMenu( ... ))
]
)
)
The only way to do this now, is a glob of imperitive code:
navigatorBuilder: (nav){
if(route.contains('/messages')) return AppMenu(child: MessagesMenu(child: nav)
return AppMenu(child: nav);
}
Which again, works ok in trivial examples, falls apart and becomes hard to read and debug when things begin to scale.
I see. You're proposing adding a navigatorBuilder to each route. Do you think it'll get that much use? Is it giving you something you can't do in the top level navigatorBuilder?
Personally I think it's one of those tiny features that unlocks many use cases. Like I mentioned in the OP, it doesn't give you anything you can't do imperatively, but it lets to express the same thing declaratively. And it's this combination of using declarative and imperative that really provides the nicest workflow and flexibility.
As a concrete example, one very common architectural pattern is one where your initial route does not have a main menu, but then all inner routes do.
final router = GoRouter(
routes: [
GoRoute(path: '/', pageBuilder: (c, __) => buildAuthPage(c)),
GoRoute(
path: '/tabs',
navigatorBuilder: (context, child, state) => TabMenu(child),
routes: [
GoRoute(path: '/1', pageBuilder: (_, __) => buildPageOne()),
GoRoute(path: '/2', pageBuilder: (_, __) => buildPageTwo()),
GoRoute(path: '/3', pageBuilder: (_, __) => buildPageThree()),
],
),
],
);
A little unrelated, but I keep coming up with this problem in the navigatorBuilder
, I can't actually access the current location.
final router = GoRouter(
navigatorBuilder: (context, child) {
// I want to make some decisions based on .location here but can't seem to access the router
}
)
It would be nice if the signature for this builder was (BuildContext, Widget, GoRouterState)
so we could easily make decisions based on the current location. This would also be the same signature for the nested builders who may also want to have small chunks of imperative logic inside their builders. This would also solve the misleading Widget?
in the builder. Happy to make a ticket if you think it makes sense.
You begin to convince me.
side note - one issue that crops up when using this technique is Dialogs, Overlays and BottomSheets. Usually you want them to appear full-screen, and not be nested in any surrounding nav, which can be an issue when your Navigator
is wrapped by scaffolding.
Luckily, I just tested and this appears to work great:
runApp(
MaterialApp(
home: MaterialApp.router(
routerDelegate: router.routerDelegate,
routeInformationParser: router.routeInformationParser,
),
),
);
The top level MaterialApp can handle the full screen stuff, with the inner one handling all routing and wrapping. I guess we can now truly say that it uses "nested" navigators :D
https://user-images.githubusercontent.com/736973/140859579-2bec183f-6d38-4839-837e-96047f25a0fb.mp4
wtf?! that's pure craziness. I like it! : )
That makes me wonder... could we just use Router directly? Based on the docs, it almost seems like that should work:
runApp(
MaterialApp(
home: Router(
routerDelegate: goRouter.routerDelegate,
routeInformationParser: goRouter.routeInformationParser,
),
),
);
[Edit] Just tested and this works great! Very cool! Feels a lot less hacky than using multiple MaterialApps.
That makes me wonder... could we just use Router directly? Based on the docs, it almost seems like that should work:
runApp( MaterialApp( home: Router( routerDelegate: goRouter.routerDelegate, routeInformationParser: goRouter.routeInformationParser, ), ), );
[Edit] Just tested and this works great! Very cool! Feels a lot less hacky than using multiple MaterialApps.
What if the MaterialApp uses the same delegate as the nested router? Does the nav stack get added to the existing one or create a new one?
Is there a real-world example of where this would be useful? If so, I'm inclined to add it.
Using navigatorBuilder
looks good. I don't think this would be true nested navigation (i.e a navigator inside a navigator) but from my experience nested navigation is never used for more than what navigatorBuilder
could do. Great idea for much less headaches !
WARNING about the side note: DON'T use MaterialApp.router
or Router
nested inside a MaterialApp
. Both code samples in https://github.com/csells/go_router/issues/133#issuecomment-963787414 and https://github.com/csells/go_router/issues/133#issuecomment-964655235 will break:
cc @esDotDev thanks for the tip, @lulupointu !
Is there a real-world example of where this would be useful? If so, I'm inclined to add it.
gmail uses this paradigm, all the routes under /settings
, are wrapped in this secondary tab menu.
In this case,
SettingsMenu
with General | Labels | ...
which is only present on the settings sub-routes. SettingsMenu
outside of the navigator so it retains animation state, and does not fadeIn or slideIn with the route itself.Sorry. I meant if you wanted to produce an example to add to the repo that required route-level navigatorBuilder
, then I'm happy to add the feature.
Is this enough to run with?
class GoRouteBuildersDemo extends StatefulWidget {
const GoRouteBuildersDemo({Key? key}) : super(key: key);
@override
State<GoRouteBuildersDemo> createState() => _GoRouteBuildersDemoState();
}
class _GoRouteBuildersDemoState extends State<GoRouteBuildersDemo> {
MaterialPage createPage(Key key, Widget child) => MaterialPage(child: child, key: key);
late GoRouter goRouter = GoRouter(
routes: [
// No scaffold for login page
GoRoute(path: '/', pageBuilder: (_, __) => const MaterialPage(child: LoginPage())),
GoRoute(
path: '/app',
// All /app pages get the main scaffold
navigatorBuilder: (_, child) => MainScaffold(child: child!),
pageBuilder: (_, __) => const MaterialPage(child: SizedBox.shrink()),
routes: [
GoRoute(path: 'inbox', pageBuilder: (_, state) => createPage(state.pageKey, const InboxPage())),
GoRoute(
path: 'settings',
// all /app/settings pages get SettingsMenu
navigatorBuilder: (_, child) => SettingsMenu(child: child!),
pageBuilder: (_, __) => const MaterialPage(child: SizedBox.shrink()),
routes: [
GoRoute(path: 'general', pageBuilder: (_, s) => createPage(s.pageKey, const GeneralPage())),
GoRoute(path: 'accounts', pageBuilder: (_, s) => createPage(s.pageKey, const AccountsPage())),
GoRoute(path: 'filters', pageBuilder: (_, s) => createPage(s.pageKey, const FiltersPage())),
],
),
],
),
],
errorPageBuilder: (BuildContext context, GoRouterState state) => MaterialPage(child: Text('${state.error}'))
);
@override
Widget build(BuildContext context) {
return MaterialApp.router(
routeInformationParser: goRouter.routeInformationParser,
routerDelegate: goRouter.routerDelegate,
);
}
}
This gives you:
https://user-images.githubusercontent.com/736973/142553655-601f61b3-73a2-463c-b464-6d42f4125814.mp4
This is possible now, with the blob of imperative code at the root level, but's not that great to read, and would only get worse over time:
navigatorBuilder: (_, child) {
// on a page under /app/settings? add a settings menu
if (goRouter.location.startsWith('/app/settings')) {
child = SettingsMenu(child: child!);
}
// on a page under /app add the main scaffold
if (goRouter.location.startsWith('/app/')) {
child = MainScaffold(child: child!);
}
return child;
},
Supporting content classes here:
class ContentPage extends StatelessWidget {
const ContentPage({Key? key, required this.title}) : super(key: key);
final String title;
@override
Widget build(BuildContext context) =>
Scaffold(body: Center(child: Text(title, style: const TextStyle(fontSize: 48))));
}
class LoginPage extends ContentPage {
const LoginPage({Key? key}) : super(key: key, title: 'LOGIN');
@override
Widget build(BuildContext context) {
return GestureDetector(onTap: () => context.go('/app/inbox'), child: super.build(context));
}
}
class InboxPage extends ContentPage {
const InboxPage({Key? key}) : super(key: key, title: 'INBOX');
}
class GeneralPage extends ContentPage {
const GeneralPage({Key? key}) : super(key: key, title: 'GeneralPage');
}
class AccountsPage extends ContentPage {
const AccountsPage({Key? key}) : super(key: key, title: 'AccountsPage');
}
class FiltersPage extends ContentPage {
const FiltersPage({Key? key}) : super(key: key, title: 'FiltersPage');
}
class MainScaffold extends StatelessWidget {
const MainScaffold({Key? key, required this.child}) : super(key: key);
final Widget child;
@override
Widget build(BuildContext context) {
return Scaffold(
backgroundColor: Colors.grey.shade200,
body: Column(
children: [
const Text('APP TITLE', style: TextStyle(fontSize: 32)),
Expanded(child: child),
Row(
children: [
TabButton('inbox', onPressed: () => context.go('/app/inbox')),
TabButton('settings', onPressed: () => context.go('/app/settings/general'))
],
)
],
),
);
}
}
class SettingsMenu extends StatelessWidget {
const SettingsMenu({Key? key, required this.child}) : super(key: key);
final Widget child;
@override
Widget build(BuildContext context) {
return Scaffold(
body: Column(
children: [
Text('My Settings', style: TextStyle(fontSize: 32)),
ColoredBox(
color: Colors.grey.shade300,
child: Row(
children: [
TabButton('general', onPressed: () => context.go('/app/settings/general')),
TabButton('accounts', onPressed: () => context.go('/app/settings/accounts')),
TabButton('filters', onPressed: () => context.go('/app/settings/filters'))
],
),
),
Expanded(child: child),
],
),
);
}
}
class TabButton extends StatelessWidget {
const TabButton(this.label, {Key? key, required this.onPressed}) : super(key: key);
final VoidCallback onPressed;
final String label;
@override
Widget build(BuildContext context) {
return Expanded(
child: TextButton(
child: Padding(
padding: const EdgeInsets.all(16.0),
child: Text(
label.toUpperCase(),
style: TextStyle(fontSize: 24),
),
),
onPressed: onPressed));
}
}
I'm not sure I like this. In my opinion:
DO use navigatorBuilder
for dependency injection
DON'T use navigatorBuilder
for UI building
The reason is that the animations are not right if you do so. When you navigate from InboxPage
to a page in SettingsMenu
, the top settings menu should be animated with the rest of the page imo. This is just my opinion though, and anyway once/if this feature is implemented nothing will prevent developers to use it for UI building, but I don't think this should be encouraged either.
Well the counter point to that, stateful animations within the UI will not work right if they are inside the navigator.
Imagine a basic TabBar in flutter, that tweens the underline to each new tab. This breaks if the menu is outside the navigator.
There's no hard rules here, the design of each app will dictate what is more desirable. imo, just keep it flexible and unopinionated.
If I had your issue IRL, we would likely switch to Fade transitions, this would give us a nice transition from Inbox to Settings, but also maintain a stateful SettingsMenu so I can do cool stuff in there if I need.
Additionally, if I really wanted to, it is trivial to have SettingsMenu have some sort of "SlideIn" effect when it first shows, since it's stateful and can do that sort of thing :) So if you made your routes fade in, and have your inner menus fade in, you'd get basically get the best of both worlds, transitions would look as expected but the menu maintains state when changing child pages.
I guess what you are probably recommending is more so like this micro-routing approach, where at some point, all the routes merge into one page, that has a stack of pages internally.
So like in this case, all the /settings/:subroute
routes would end up on MaterialPage(key: ValueKey('/settings'), child: SettingsPage(state.params['subroute']))
, and internally, Settings would decide which child Widget to show. This would also get you best of both worlds, but the tradeoff is that you need to bury this nested sub-routing code all over your ui, rather than just declaring routes and builders at the top of the tree.
So to me they both have tradeoffs, which way you go would depend on the specific design of your app, and the priorities of the stakeholders. There are ways to make the outer builder approach look good when transitioning, that might be fairly easy to implement, and give an even cooler effect than slideUp or fadeIn.
Like imagine if my sub-menus quickly folded-open or slid-out as the page content slides in from another direction, now we're talking!
For example, I added a simple FadeIn
widget to the top of the SettingsMenu
, and now it looks quite nice? Another 30m of work on it and it could look even better.
https://user-images.githubusercontent.com/736973/142662057-234b204b-ef01-4110-9ed1-6d03539ee5eb.mp4
Worth noting this API is nice for providing pieces of state to the tree below in addition to visual overlays or wrapping scaffolds:
GoRoute(
navigatorBuilder: (context, navigator){
return Provider(create: (_)=> Quiz1(), child: navigator);
},
path: '/quiz',
pageBuilder: (context, state) => MaterialPage<void>(...),
routes: [ ... ],
);
As mentioned here: https://github.com/csells/go_router/issues/185
I personally think this would be confusing to add it to all the GoRoute as it changes the functionality of the routes
Right the navigations is one-level that allow multiple combinations that are stacked one above the other
I think it should be possible to have nested navigators like:
We could create a specific a new GoNavigatorRoute
class, instead of breaking the behaviour of the current GoRoute
( that is the most used case scenario)
agreed, @jamesblasco. that's something we're exploring in https://github.com/csells/go_router/discussions/222
Some time ago I was discussing similar thing here #128.
Although I've got some workaround for it, it's still not as great as per-route navigationBuilder
would be, especially with more routes.
Then I thought the new v3 top level navigatorBuilder
with the state
parameter would solve it. It doesn't, because I cannot get URL params there.
navigatorBuilder: (context, state, child) {
if (/* I'm on /users/:userId/xxx */) {
return Provider(
create: (_) => UserBloc(state.params['userId']), // params are not available here
child: child,
);
}
}
Also this would not easily work with nested routes and their nested providers and the top level navigatorBuilder
would get really big.
So I tried to make PoC how this issue would solve my problem, it's available here and the example is here.
The GoRouter
code is not pretty, but for PoC it does the job :)
The only problem I have noticed is going back to /users
from /users/:id/xxx
.
When you press the back button, it goes from /users/:id/xxx
to /users/:id
- because /users/:id
is somehow also considered as a route although it's only purpose is to redirect.
So in this example you need to manually remove the user ID when going back to /users
, otherwise it would still keep the old UserBloc
.
But I don't believe this would be impossible to solve, right?
So... What do you think?
I personally think this would be confusing to add it to all the GoRoute as it changes the functionality of the routes
I'd argue it compliments the functionality of the routes, which are used not only to declare single routes, but also to declare parent/child relationships and nesting.
Often scaffolding, or provided controllers, need to be injected along these same parent/child boundaries, so it's a natural place to do it.
For example, if all pages within /search
, need a persistent search bar at the top, the most obvious place to declare it is within the parent GoRoute
:
return GoRoute(
path: '/search',
navigatorBuilder(_, __, child) => SearchScaffold(body: child),
routes: [
GoRoute('inbox', ...),
GoRoute('outbox', ...),
]
)
This is most intuitive, and nothing else is comparable in terms of readability and scalability. It does not take very many decisions before the "do it all in the top builder" turns into a big mess.
Using a dedicated widget is a good idea too, VRouter
did this with a VNester
, but not sure it's worth the extra API surface and complexity when just adding a navigatorBuilder
seems effective and simple.
But then how it would work inside? If there is a navigatorBuilder then the Route would inject a Navigator but if not builder is provided it still uses its parent one? We should not be creating a navigator per route.
For my use case it would use all navigatorBuilder
s from the top level one to the final matched GoRoute
.
So if you had navigatorBuilder
defined for every single GoRoute
(just an example) and opened route /users/:id/roles
, it would use 4 matched builders:
GoRouter # this one
|_GoRoute `users` # this one
| |_GoRoute `:id` # this one
| |_GoRoute `roles` # this one
| | |_GoRoute `:roleId`
| |_GoRoute `info`
|_GoRoute `settings`
Like this:
root navigation builder
|_users navigation builder
|_id navigation builder
|_roles navigation builder
Since all of them are navigator builders
, I think it makes sense that all matched routes navigator builders are used.
It would just act like a "top level builder extension".
This also meets lulupointu's opinion about using it for dependency injection.
@jamesblasco if I understand @esDotDev correctly, he doesn't inject new navigators, am I right? I believe his idea is similar to mine, that is just wrapping the main navigator with custom widgets (correct me if I'm wrong).
Yes, there is not a new navigator created for each route, it would just be wrapping additional scaffolding or providers around the one Navigator.
In the example above, after all 4 builders have ran, you'd end up with something like this returned from the router delegate:
RootNav(
child: UsersNav(
child: IdNav(
child: RolesNav(
child: Navigator(pages: [])
)
)
)
)
While
navigatorBuilder
is useful for providing a single top-level scaffold or menu around your app, it forces you to use imperative logic in cases where you do not simply want to have a single wrapping scaffold.Consider a use-case, where I have a single login page with no scaffold, once logging in, I want to wrap my main menu around the inner routes.
I could write it imperitively, with something like:
Or, I could write it declaratively like this:
Both approaches are valid, but currently only the imperative is possible. Imperative works nice for simple use cases like the above, but has some problems expressing complex trees.
Declarative allows much more flexibility for various navigation schemes. For example, it easily allows you to wrap secondary or tertiary menus around your routes by just declaring the builders around the routes that need them: