Skip to content

Conversation

johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Jul 22, 2022

Adds ShellRoute

Resolves flutter/flutter#108141

@johnpryan johnpryan requested a review from chunhtai July 22, 2022 19:43
@johnpryan johnpryan changed the title Add new route types [go_router] Add new route types Jul 22, 2022
@johnpryan johnpryan force-pushed the add-new-route-types branch 4 times, most recently from 77d95cd to 502f7e4 Compare July 22, 2022 21:14
@johnpryan johnpryan marked this pull request as ready for review July 22, 2022 21:19
Copy link

@prince-kumar prince-kumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnpryan johnpryan force-pushed the add-new-route-types branch from 8c3e40d to a637aca Compare August 1, 2022 22:16
@johnpryan johnpryan changed the title [go_router] Add new route types [go_router] Add new route types and builder for ShellRoute Aug 1, 2022
@johnpryan
Copy link
Contributor Author

johnpryan commented Aug 1, 2022

@loic-sharma @chunhtai ready for another look - I added some functionality to ShellRoute with some tests, but right now this is still a "dark" feature and not part of the public API exposed by lib/go_router.dart.

@johnpryan
Copy link
Contributor Author

After discussing this offline, I want to make some changes to this PR:

  • Remove NestedStackRoute
  • Remove StackedRoute and stick with GoRoute
  • ShellRoute now always builds a Router and Navigator. This means that sub-routes will stack on this Navigator by default, rather than requiring a NestedNavigatorRoute. We will use an additional Router to ensure that we are handling back button behavior correctly. This cannot be made optional because pageBuilder would break for child routes of ShellRoute.
  • Add a navigatorKey parameter to the GoRoute constructor. This specifies which Navigator to place this GoRoute onto. By default, GoRoutes stack onto the Navigator built by the nearest ShellRoute ancestor.
  • Add a shellNavigatorKey parameter to the ShellRoute constructor. This specifies the navigator key to use when building the Navigator associated with this ShellRoute.
  • Add a navigatorKey parameter to the ShellRoute constructor, which has the same behavior as navigatorKey on GoRoute.

@johnpryan johnpryan changed the title [go_router] Add new route types and builder for ShellRoute [go_router] Add ShellRoute Aug 10, 2022
@johnpryan johnpryan force-pushed the add-new-route-types branch from bf44e2c to 41ddc31 Compare August 10, 2022 17:28
@chunhtai chunhtai self-requested a review August 10, 2022 17:30
@loic-sharma
Copy link
Member

Should this PR target the go_router_v5 branch now?

@johnpryan
Copy link
Contributor Author

Should this PR target the go_router_v5 branch now?

This isn't a breaking change, but maybe we should if we want to avoid merge conflicts. @chunhtai WDYT?

@cedvdb
Copy link
Contributor

cedvdb commented Aug 11, 2022

I assume this pr deals with nested routers.

One annoying thing with nested routers in practice are all the overlays like dialogs, bottom sheet, etc.

By default the result will be unexpected for most users: the modal barrier will only appear on the nested route. You have to explicitely call useRoot: true or something like that.

How will those scenarios be handled ?

I said it before when @chunhtai requested feedback: those two concepts of "overlay" and navigation should not have been mixed together in my opinion. Imo a separate overlay handler class is needed, but that might be to big of a change.

@johnpryan
Copy link
Contributor Author

By default the result will be unexpected for most users: the modal barrier will only appear on the nested route. You have to explicitly call useRoot: true or something like that.
How will those scenarios be handled ?

You can specify which Navigator a route's screen will be overlaid on by specifying a navigatorKey on the GoRouter constructor or a shellNavigatorKey on the ShellRoute if you are using nested navigation. Then you specify the same navigatorKey on the GoRoute.

I said it before when @chunhtai requested feedback: those two concepts of "overlay" and navigation should not have been mixed together in my opinion. Imo a separate overlay handler class is needed, but that might be too big of a change.

Can you describe this scenario a bit more? This PR is a simplified version of go_router_prototype, which supports overlay-based navigation (GoRoute / StackedRoute) and shell-based navigation (ShellRoute). Shell-based navigation is very similar to how routing works for web-application frameworks and isn't based on overlays.

@cedvdb

This comment was marked as off-topic.

@johnpryan
Copy link
Contributor Author

@cedvdb this is getting off-topic, would you mind filing a separate issue?

@johnpryan johnpryan force-pushed the add-new-route-types branch from ad84fd9 to 20f8d96 Compare August 11, 2022 23:08
@johnpryan johnpryan force-pushed the add-new-route-types branch from 20f8d96 to 62b1d13 Compare August 11, 2022 23:09
@johnpryan johnpryan closed this Aug 12, 2022
@johnpryan johnpryan deleted the add-new-route-types branch August 12, 2022 21:40
@johnpryan
Copy link
Contributor Author

New pull request: #2453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[go_router] Add ShellRoute
6 participants