-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[go_router_builder] Add ShellRoute support to go_router_builder #3269
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
[go_router_builder] Add ShellRoute support to go_router_builder #3269
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). 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. |
@@ -225,7 +260,8 @@ class PersonScreen extends StatelessWidget { | |||
in person.details.entries) | |||
ListTile( | |||
title: Text( | |||
'${entry.key.name} - ${entry.value}', | |||
// TODO(kevmoo): replace `split` with `name` when min SDK is 2.15 |
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.
Should this be reverted?
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.
Good catch
'/', | ||
); | ||
|
||
void go(BuildContext context) => context.go(location, extra: this); |
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.
ShellRoute should not have go/push/pushRelacement
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.
Hmm... It probably shouldn't have a location
either.
I'm trying to add an example for ShellRouteData, but I'm stuck because the // Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// ignore_for_file: public_member_api_docs
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
part 'shell_route_example.g.dart';
void main() => runApp(App());
class App extends StatelessWidget {
App({super.key});
@override
Widget build(BuildContext context) => MaterialApp.router(
routerConfig: _router,
);
final GoRouter _router = GoRouter(routes: $appRoutes);
}
class HomeScreen extends StatelessWidget {
const HomeScreen({super.key});
@override
Widget build(BuildContext context) => Scaffold(
appBar: AppBar(title: const Text('foo')),
);
}
@TypedShellRoute<MyShellRouteData>(
routes: <TypedRoute<RouteData>>[
TypedGoRoute<FooRouteData>(path: '/foo'),
TypedGoRoute<BarRouteData>(path: '/bar'),
],
)
class MyShellRouteData extends ShellRouteData {
const MyShellRouteData();
@override
Widget builder(
BuildContext context,
GoRouterState state,
Widget navigator,
) {
return MyShellRouteScreen(child: navigator);
}
}
class FooRouteData extends GoRouteData {
const FooRouteData();
@override
Widget build(BuildContext context, GoRouterState state) {
return const FooScreen();
}
}
class BarRouteData extends GoRouteData {
const BarRouteData();
@override
Widget build(BuildContext context, GoRouterState state) {
return const BarScreen();
}
}
class MyShellRouteScreen extends StatelessWidget {
const MyShellRouteScreen({required this.child, super.key});
final Widget child;
@override
Widget build(BuildContext context) {
return Scaffold(
bottomNavigationBar: BottomNavigationBar(
items: const <BottomNavigationBarItem>[
BottomNavigationBarItem(
icon: Icon(Icons.home),
label: 'Foo',
),
BottomNavigationBarItem(
icon: Icon(Icons.business),
label: 'Bar',
),
],
onTap: (int index) {
switch (index) {
case 0:
// const FooRouteData().go(context);
break;
case 1:
// const BarRouteData().go(context);
break;
}
},
),
);
}
}
class FooScreen extends StatelessWidget {
const FooScreen({super.key});
@override
Widget build(BuildContext context) {
return const Text('Foo');
}
}
class BarScreen extends StatelessWidget {
const BarScreen({super.key});
@override
Widget build(BuildContext context) {
return const Text('Bar');
}
} |
Hi @johnpryan Do you know what's the state of this PR? |
Sorry, this isn't ready to land yet. I'm currently busy with other projects, if someone else can work on this, feel free. The issue I mentioned in the comment above is becuase there's no generator for the ShellRoute annotation. To fix it, I think you need to wire it up in lib/go-router_builder.dart, here:
so you add another GoRouterShellGenerator() or something to that array. I would also like to add more tests and a separate example for ShellRoute, similar to my comment above. |
@chunhtai I created a PR johnpryan#1 against @johnpryan 's branch: I created a
Based on your feedback, I'll add a test similar to the existing one for the I've also squashed merged the changes from master to solve the conflicts (pubspec and changelog) but maybe I shouldn't have because now the github action added all the labels... |
Hi @GP4cK thanks for helping, can you create a PR directly against the flutter:main? I will close this pr. |
Depends on #2730
Fixes flutter/flutter#111909