Skip to content
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] Add parentNavigatorKey to ShellRoute #111678

Closed
johnpryan opened this issue Sep 15, 2022 · 7 comments · Fixed by flutter/packages#4201
Closed

[go_router] Add parentNavigatorKey to ShellRoute #111678

johnpryan opened this issue Sep 15, 2022 · 7 comments · Fixed by flutter/packages#4201
Assignees
Labels
c: proposal A detailed proposal for a change to Flutter p: go_router The go_router package P3 Issues that are less important to the Flutter project package flutter/packages repository. See also p: labels.

Comments

@johnpryan
Copy link
Contributor

johnpryan commented Sep 15, 2022

Right now, ShellRoute does not support parentNavigatorKey. The behavior would be the same as GoRoute. For example, in this configuration, navigating to /a/b/c would place a new page on top of the root ShellRoute's page for routes b and c:

ShellRoute(
  routes: [
    GoRoute(
      path: '/a',
      routes: [
        ShellRoute(
          parentNavigatorKey: root
          routes: [
            GoRoute(
              path: 'b'
              routes: [
                GoRoute(
                  path: 'c',
                ),
              ],
            ),
          ],
        ),
      ],
    ),
  ],
)
@johnpryan johnpryan added the p: go_router The go_router package label Sep 15, 2022
@darshankawar darshankawar added p: first party package flutter/packages repository. See also p: labels. c: proposal A detailed proposal for a change to Flutter labels Sep 21, 2022
@stuartmorgan stuartmorgan added the P3 Issues that are less important to the Flutter project label Sep 27, 2022
@chunhtai
Copy link
Contributor

Hi Hangyu, can you take a look, this should be an easy fix

@Walrus117
Copy link

This would be an extremely helpful feature for displaying sheet routes for example. This way the animation of the ShellRoute entering would control the entrance of the sheet whilst the secondaryAnimation could transition out the sheet below.

Looking at line 182 to line 199 of
https://github.com/flutter/packages/blob/main/packages/go_router/lib/src/builder.dart

seems to imply this would be possible. Right now the ShellRoute is configured to use the NavigatorKey whilst the GoRoute is configured to accept the property of its route.parentNavigatorKey.

@Walrus117
Copy link

Further to my earlier comment, I have successfully implemented this into the code. I added a property to ShellRoute: parentNavigatorKey and altered the builder file to use this key if it is declared. I will continue testing and get back to you with the recommended updates if it proves viable.

@chunhtai
Copy link
Contributor

@88RIK88 Feel free to open a pr if you are up for it and pin me for review

@Walrus117
Copy link

@chunhtai Sorry for the delay. It works perfectly. I'm not accustomed to GitHub and how a PR works but I do understand code. This change requires a one field to be added to ShellBaseRoute 'parentNavigatorKey' and then adding the same line as a super to ShellRoute and anything else that extends ShellBaseRoute. It then requires a one line modification to builder.dart in the main go router library. It works perfectly and I have test all situations I can think of. It has never failed. How do we submit a PR? (ps don't use the library in my profile, I've modified it locally so I could debug and check with GoRouter 7.1. I've looked at the code for 8.0 and there is nothing that will break it that I can see.

@Walrus117
Copy link

Walrus117 commented Jun 12, 2023

In builder.dart

Under the _buildRecursive method after else if (route is ShellRouteBase) { the following needs to be the parentNavigatorKey line needs to be changed to final GlobalKey<NavigatorState> parentNavigatorKey = route.parentNavigatorKey ?? navigatorKey;

in route.dart

Under ShellRouteBase add the following property
final GlobalKey<NavigatorState>? parentNavigatorKey;
Then update the constructor to reflect this and add it as a super.parentNavigatorKey to ShellRoute and any other ShellRouteBase extension.

That's all that is needed to make this work. I don't know much about PR's but that's got to be a really simple one. The functionality to make this work was almost already in GoRouter. I don't know if there is a reason why they didn't include it but from all my tests, this works every time and is a very small change.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: proposal A detailed proposal for a change to Flutter p: go_router The go_router package P3 Issues that are less important to the Flutter project package flutter/packages repository. See also p: labels.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants