flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
164.98k stars 27.18k forks source link

`StatefulShellRoute.indexedStack` should have a `routes` field for allowing navigations that go on top of the `StatefulShellRoute` #140273

Open feinstein opened 9 months ago

feinstein commented 9 months ago

Use case

I have a StatefulShellRoute.indexedStack with a bottom navigator. When I click each of the bottom nav tabs, the main screen (above the bottom nav) changes with the correct routes, but the app we are developing has a requirement that some buttons in the home screen will lead to new screens that should navigate to be "on top" of the main screen (the screen that has the bottom nav).

Currently we can't do this using go_router, since we can't define child routes for the StatefulShellRoute.indexedStack, only branches, that will replace what's in the middle of the screen.

Proposal

Add a routes filed to the StatefulShellRoute.indexedStack constructor, so we can defile child routes that will replace the whole screen when navigated to.

chunhtai commented 9 months ago

you can nested child routes under the same branch, or just use push when buttom nav is clicked depends on which behavior do you want.

feinstein commented 9 months ago

I tried both, and the new route is always on top of the bottom navigator. That's because in the indexed stack, we usually make a widget that puts the current page in a branch, on top of the bottom nav bar.

Also, there will be no animation, since the indexed stack just replaces the current branch.

I want a "normal" navigation, where the new page will take the whole screen and animate with the default platform transition between pages.

Currently, as far as I know, will only work if I do this in a hacky way, removing the bottom nav bar for specific pages and adding transitions for them, or using the old Navigator.

chunhtai commented 9 months ago

I am not 100% certain what's the expected and actual behavior. can you post a repro so that I can see if this is something we need to support or there is existing feature to cover it

Chonli commented 9 months ago

I am not sure understand your problem but if you have route at same level of StatefulShellRoute.indexedStack and use push(..) instead of go(..), your new page became on the top and have back buttom to return to StatefulShellRoute.

feinstein commented 8 months ago

I tried to put up a very quick example of the desired behaviour.

ExamplePage is being reused as all pages for simplicity. HomePage holds the bottom navigation. The home screen has 2 bottom tabs, and I want to navigate to another screen that should go back to the home page.

Both goNamed and pushNamed have the same effect, they go to a page that takes the whole screen, but there's no navigation animation (I expected one in a push) and there's no back button (I also expected one in a push). The Navigator.of(context).push does the push animation, but there's no back button as well.

Reproducible code ```dart import 'package:flutter/material.dart'; import 'package:go_router/go_router.dart'; void main() { runApp(const MyApp()); } class MyApp extends StatelessWidget { const MyApp({super.key}); @override Widget build(BuildContext context) { return MaterialApp.router( title: 'Flutter Demo', theme: ThemeData( colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple), useMaterial3: true, ), routerConfig: router, ); } } class HomePage extends StatelessWidget { const HomePage({ super.key, required this.navigationShell, }); final StatefulNavigationShell navigationShell; void handleDestinationSelected(int index) { navigationShell.goBranch( index, initialLocation: index == navigationShell.currentIndex, ); } @override Widget build(BuildContext context) { return Scaffold( appBar: AppBar(), body: navigationShell, bottomNavigationBar: NavigationBar( onDestinationSelected: handleDestinationSelected, selectedIndex: navigationShell.currentIndex, destinations: const [ NavigationDestination( icon: Icon(Icons.grid_view_outlined), label: 'Home', ), NavigationDestination( icon: Icon(Icons.ad_units), label: 'Tab 2', ), ], ), ); } } final navKey = GlobalKey(); final router = GoRouter( navigatorKey: navKey, initialLocation: '/home', routes: [ GoRoute( path: '/new_screen', name: 'new_screen', builder: (context, state) => const ExamplePage(title: 'new screen'), ), StatefulShellRoute.indexedStack( pageBuilder: (context, state, navigationShell) { // Fade transition coming from the SplashScreen. return NoTransitionPage( key: state.pageKey, child: HomePage(navigationShell: navigationShell), ); }, branches: [ StatefulShellBranch( routes: [ GoRoute( path: '/home', name: 'home', builder: (context, state) => const ExamplePage(title: 'home'), ), ], ), StatefulShellBranch( routes: [ GoRoute( path: '/tab2', name: 'tab2', builder: (context, state) => const ExamplePage(title: 'tab2'), ), ], ), ], ), ], ); class ExamplePage extends StatelessWidget { const ExamplePage({ super.key, required this.title, }); final String title; @override Widget build(BuildContext context) { return Scaffold( body: Center( child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Text(title), ElevatedButton( onPressed: () => context.goNamed('new_screen'), child: const Text('Go named to new screen'), ), ElevatedButton( onPressed: () => context.pushNamed('new_screen'), child: const Text('Push named to new screen'), ), ElevatedButton( onPressed: () => navKey.currentState!.push(MaterialPageRoute( builder: (context) => const ExamplePage(title: 'second screen (whole screen)'), )), child: const Text('Navigator 1.0 push to the second screen (desired behavior, but still misses a back arrow)'), ), ], ), ), ); } } ```
omar-hanafy commented 8 months ago

Hey @feinstein , I really like your idea, I was having the exact same problem, and I solved this by adding the same root navigator key to the routes that I need them to be pushed globally.

But I think this is a temporary, because by defining route And add to it it's root key you made it global only.

So if you wish to use both global, and to be bushed inside the current navigation bar screen, then you gotta create two separate routes for that. One that has its route navigator key, And the other to be normal one.

You might consider this a temporary solution until a proper work around for this is made.

If I would suggest an idea I would suggest something similar to this to force the route to be on top of the statefulshellroute.

'''dart context.pushIgnoreShellRoute Or context.push(..., ignoreShellRoute: false) '''

feinstein commented 3 months ago

Adding more context here... If I have a page that needs to cover a StatefulShellRoute.indexedStack, I can push to that page, but if I have a deep link to that page, then it won't work.

Also, if I have a redirect logic that wants to go to that route, it also won't work, because I can't return a path to that route from the redirect, and have that route pushed on top of the StatefulShellRoute.

feinstein commented 3 months ago

@chunhtai do you mind if I try to submit a PR adding a routes parameter under the shell routes?

chunhtai commented 3 months ago
final router = GoRouter(
  navigatorKey: navKey,
  initialLocation: '/home',
  routes: [
    StatefulShellRoute.indexedStack(
      pageBuilder: (context, state, navigationShell) {
        // Fade transition coming from the SplashScreen.
        return NoTransitionPage<void>(
          key: state.pageKey,
          child: HomePage(navigationShell: navigationShell),
        );
      },
      branches: [
        StatefulShellBranch(
          routes: [
            GoRoute(
              path: '/home',
              name: 'home',
              builder: (context, state) => const ExamplePage(title: 'home'),
            ),
          ],
        ),
        StatefulShellBranch(
          routes: [
            GoRoute(
              path: '/tab2',
              name: 'tab2',
              builder: (context, state) => const ExamplePage(title: 'tab2'),
              routes: [
                    GoRoute(
                     parentNavigatorKey: navKey,
                     path: 'new_screen',
                     name: 'new_screen',
                     builder: (context, state) => const ExamplePage(title: 'new screen'),
                    ),
              ]
            ),
          ],
        ),
      ],
    ),
  ],
);

Is this something you want?

yehorh commented 3 months ago

@chunhtai


/login
+--------------------------------+
|Login                           |
|                                |
|                                |
|                                |
|   Username: [___________]      |
|                                |
|                                |
|   Password: [___________]      |
|                                |
|                                |
|                                |
|                                |
|                                |
|  [ Login ]                     |
|                                |
|                                |
+--------------------------------+

/app/tab1
+--------------------------------+
|Top Nav Bar                [🔔] |
+--------------------------------+
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
+--------------------------------+
| *[tab1]*   [tab2]    [tab3]    |
+--------------------------------+

/app/tab2
+--------------------------------+
|Top Nav Bar                [🔔] |
+--------------------------------+
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
+--------------------------------+
|  [tab1]  *[tab2]*    [tab3]    |
+--------------------------------+

/app/tab3
+--------------------------------+
|Top Nav Bar                [🔔] |
+--------------------------------+
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
+--------------------------------+
|  [tab1]   [tab2]   *[tab3]*    |
+--------------------------------+

/app/notifications
+--------------------------------+
| [<] Notifications              |
+--------------------------------+
|  - Notification 1              |
|    Short description...        |
|                                |
|  - Notification 2              |
|    Short description...        |
|                                |
|  - Notification 3              |
|    Short description...        |
|                                |
|                                |
|                                |
|                                |
|                                |
|                                |
+--------------------------------+

Navigation tree

/login
/app
  ├── StatefulShellRoute
  │   ├── content
  │   │   ├── tab1
  │   │   │   └── subroutes
  │   │   │       └── 'product-:id'
  │   │   ├── tab2
  │   │   └── tab3
  │   └── subroutes
  │       └── notifications

I would like to request the following application behavior:

/login as a separate route. /app/tab1, /app/tab2, /app/tab3 should display a screen in a StatefulShellRoute as currently implemented. When navigating to /app/notifications, it should add the notifications screen on top of the StatefulShellRoute and preserve the state of the selected path within the StatefulShellRoute.

This is necessary because the notifications screen is available not within a specific tab but as a part of the main scaffold screen.

If we declare the notifications screen as a subroute of one of the tab screens and navigate to it using go, it will reset the view of the tab the user is currently on.

I hope I was able to convey the problem with the current lack of subroutes in StatefulShellRoute.

Using the push method is not acceptable as it excludes the declarative approach and deep link navigation.

feinstein commented 3 months ago

@chunhtai no, because if I go to new_screen it will be inside the bottom navigator, but I want new_screen to open full screen, while maintaining the navigation stack intact.

One example is Instagram, if you click the + button on the navigation bar, it opens a full screen page for taking a picture and the bottom navigation bar is not visible anymore.

This limitation doesn't allow me to redirect or deeplink to a full screen page that is a sub route of a StatefulShellRoute.indexedStack.

chunhtai commented 3 months ago

if I go to new_screen it will be inside the bottom navigator, but I want new_screen to open full screen

It should be a full screen since there is a parentnavkey points to the root navigator. Let me know otherwise, in that case it is a bug

@yehorh

Yes I can see this be useful, but if user directly do go(/app/notifications) and do a pop what should be displayed on the screen

feinstein commented 3 months ago

@chunhtai thank you, I did some tests and I can see this works for almost all cases but the case where the immediate child of the StatefulShellRoute.indexedStack (which is the Instagram example I told you about). It triggers an assertion error:

'package:go_router/src/route.dart': Failed assertion: line 472 pos 11: 'route.parentNavigatorKey == null || route.parentNavigatorKey == navigatorKey': sub-route's parent navigator key must either be null or has the same navigator key as parent's key

I expected that it would go to that page as a full screen page, and when poping it will pop to the previously selected branch, restoring the navigation stack as it was before. In case this was the first navigation, indeed there's nothing to back for, but maybe we can make the default being the first index? Or provide a callback that decides what to do on those cases? Or your current proposal for changing the redirect to guards could also help in this case, with a guard against navigating from the full screen page to nothing and prevent the app from closing.

Otherwise, how can we do a UI like Instagram where one of the items in the bottom nav bar navigates directly to a full page, without using push and not being able to deeplink there? Using "go" would navigate to a route that's parallel to the StatefulShellRoute.indexedStack and thus lose the back button.

yehorh commented 3 months ago

if I go to new_screen it will be inside the bottom navigator, but I want new_screen to open full screen

It should be a full screen since there is a parentnavkey points to the root navigator. Let me know otherwise, in that case it is a bug

@yehorh

Yes I can see this be useful, but if user directly do go(/app/notifications) and do a pop what should be displayed on the screen

@chunhtai The last state of the StatefulShellRoute or the initial view if it was routed from WidgetsBinding.instance.platformDispatcher.defaultRouteName.

StatefulShellRoute has its own path /app and can manage its own state; it can also redirect if it doesn't have an internal state.

chunhtai commented 3 months ago

for a real world example the StatefulShellRoute doesn't have a path

so if you have this

GoRoute(
   path: '/',
   routes: [
     StatefulShellRoute(
        branch: [...],
        routes: [
           GoRoute(path: 'new_screen'),
        ],
      ),
   ],
)

if you go('/new_screen') that is fine, but if you pop, would you display statefulShellRoute of the default branch or just display '/' which is the GoRoute?

Whatever we define the behavior, it seems like a magic...

yehorh commented 3 months ago

@chunhtai

If we give a path and name to StatefulShellRoute, we can consider it as a separate route with nested navigators. When navigating to it (if StatefulShellRoute not exist in widget tree), update the URL with the route (branch) that is its default.

I don't know how else to solve the problem of declarative routing and opening other screens without breaking the routing within StatefulShellRoute or removing it entirely from the Widget tree. I tried redirects, adding the necessary screens to each subroute, and building navigation using push. However, each of these approaches has serious drawbacks or issues that make them unsuitable.

There can be many such screens: settings, notifications, support.

feinstein commented 3 months ago

@chunhtai how about the StatefulShellRoute has a property of initialRoute, just like the GoRouter has. In case the navigation stack can't be dertmined, that's the route it will use to select its branch.

tolo commented 1 week ago

I didn’t initially see how at least the original issue couldn’t be solved using a pushed route on the parent navigator. But when additional requirements, like deep links, were thrown into the mix, I can see the complexity. And regarding the assertion error, not sure why the root routes of a shell route cannot have a different parent navigator key. @chuntai, do you know what the reasoning is? (Have a nagging feeling that I should know this, and that I ran into this when implementing the initial StatefulShellRoute).

Regardless, I still think a solution for modal routes over a stateful botton nav can be solved though (certainly room for improvements of course) - have a look at this simple example I put together for instance: https://gist.github.com/tolo/0ee208acebc66ce76db7df9a2f270578

One could also go one level deeper, and nest two StatefulShellRoutes for even more flexibility. See this expanded and more advanced example for instance: https://gist.github.com/tolo/939b88e4c0a8d82e487b697c6922850a

However I also definitely see the need of an easier way of dealing with the current state of StatefulShellRoute. In the PR around preloading of branches, I’ve added support for specifying the GlobalKey used by StatefulNavigationShell. But while this may be a necessary first step, I do see the need of being able to do to this more declaratively. For instance, it would be nice if one could do something like this:

GoRouter.of(context).go('/myshell/1'); // Navigate to current state of branch 1
// OR: GoRouter.of(context).go('/myshell'); // Navigate to current state of the current branch of the shell

Instead of:

StatefulNavigationShell.of(context).goBranch(1);
// Or even:
_myShellStateKey.currentState?.goBranch(1);

If we added a new path field to StatefulShellRoute for this purpose, it might even be possible to remove the method goBranch eventually. We could also introduce (I think this was actually initially part of the original PR) a field for initial branch index (we already have initial location on the branches). This could mean that we could both support the scenario of going to the desired initial location of a shell route, as well as returning to the current state of it.

Example:

StatefulShellRoute.indexedStack(
  shellRoutePath: '/myshell'
  initialBranchIndex: 1,
  ...

There may be more subtleties to this that I’m not aware of right now, but might be worth pursuing.

BenjiFarquhar commented 1 week ago

What did we decide with this? I don't think push is an option as it is not deep linkable. It's a common need, to display some of the routes in a ShellRoute as full screen on mobile, due to limited space. The parentNavigationKey trick doesn't work. I also have 2 levels deep nested shells and i think the parentNavigationKey doesn't allow going up more than one level. There is an exception.

tolo commented 1 week ago

I think modal/pushed will always need some special handling in the context of deep linking, but it can be managed like in the first example gist I referenced above.

The second example gist I referenced above uses stateful shell routes in two levels, but without parentNavigationKey - and this also works.

But the even though the parentNavigationKey is probably not usable here, the limitations around it (assertion exceptions) should probably be investigated.