-
Notifications
You must be signed in to change notification settings - Fork 96
Add navigatorBuilder
params to GoRoute
#133
Comments
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:
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. |
Gosh, if only the Dart language itself had a way to package reusable code that you could leverage for this situation... (he said, sarcastically : )
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:
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.
So there is a fundamental difference here. Pretty sure any arguments made against this, can effectively be made against |
The
Regardless, each one of these would be wrapped in a |
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 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.
A little unrelated, but I keep coming up with this problem in the
It would be nice if the signature for this builder was |
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 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. 7Vfgc5vsPb.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:
[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? |
builder
params to GoRoute navigatorBuilder
params to GoRoute
Is there a real-world example of where this would be useful? If so, I'm inclined to add it. |
Using WARNING about the side note: DON'T use
|
cc @esDotDev thanks for the tip, @lulupointu ! |
Sorry. I meant if you wanted to produce an example to add to the repo that required route-level |
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: VGz5MVJDSO.mp4This 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 DON'T use The reason is that the animations are not right if you do so. When you navigate from |
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 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 i4SrWb2sN8.mp4 |
Worth noting this API is nice for providing pieces of state to the tree below in addition to visual overlays or wrapping scaffolds:
As mentioned here: #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 |
agreed, @jamesblasco. that's something we're exploring in #222 |
Some time ago I was discussing similar thing here #128. Then I thought the new v3 top level 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 So I tried to make PoC how this issue would solve my problem, it's available here and the example is here. Explanation:Here is my GoRoute(
path: '/users',
builder: (context, state) => const UsersScreen(),
routes: [
GoRoute(
path: ':userId',
redirect: (state) => '/users/${state.params['userId']}/roles',
// UserBloc with the current user will be available
// for all subsequent routes..
navigatorBuilder: (context, state, child) => Provider(
create: (_) => UserBloc(int.parse(state.params['userId']!)),
child: child,
),
routes: [
GoRoute(
path: 'roles',
pageBuilder: (context, state) => const MaterialPage<void>(
key: ValueKey('/users/:userId'),
child: UserScreen(tab: 'roles'),
),
routes: [
GoRoute(
path: ':roleId',
builder: (context, state) {
final userId = int.parse(state.params['userId']!);
final roleId = state.params['roleId']!;
return Provider(
create: (_) => RoleBloc(userId, roleId),
child: const RoleScreen(),
);
},
),
],
),
GoRoute(
path: 'info',
pageBuilder: (context, state) => const MaterialPage<void>(
key: ValueKey('/users/:userId'),
child: UserScreen(tab: 'info'),
),
),
],
),
],
), When I am on the Current way of doing it is like this: GoRoute(
path: '/users',
builder: (context, state) => const UsersScreen(),
routes: [
GoRoute(
path: ':userId/roles',
pageBuilder: (context, state) => const MaterialPage<void>(
key: ValueKey('/users/:userId'),
child: Provider(
create: (_) => UserBloc(int.parse(state.params['userId']!)),
child: const UserScreen(tab: 'roles'),
),
),
builder: (context, state) => ,
routes: [
GoRoute(
path: ':roleId',
builder: (context, state) {
final userId = int.parse(state.params['userId']!);
final roleId = state.params['roleId']!;
return Provider(
create: (_) => RoleBloc(userId, roleId),
child: const RoleScreen(),
);
},
),
],
),
GoRoute(
path: ':userId/info',
pageBuilder: (context, state) => const MaterialPage<void>(
key: ValueKey('/users/:userId'),
child: Provider(
create: (_) => UserBloc(int.parse(state.params['userId']!)),
child: const UserScreen(tab: 'info'),
),
),
),
],
), see I have to have Provider(
create: (_) => UserBloc(int.parse(state.params['userId']!)),
child: const UserScreen(tab: $tab),
) written for each route. This gets really ugly when I have multiple blocs and multiple tabs. The only problem I have noticed is going back to So... What do you think? |
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
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, |
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? |
For my use case it would use all So if you had
Like this:
Since all of them are 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:
|
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:
The text was updated successfully, but these errors were encountered: