Migrate `veggieseasons` to `go_router` by miquelbeltran · Pull Request #1544 · flutter/samples

miquelbeltran · 2022-12-22T15:24:10Z

Migration of veggieseaons to go_router.

Applying the same solution using ShellRoute as in #1437

Uses ShellRoute to handle navigation within a CupertinoTabBar
The GoRouter is implemented with state restoration in mind. The widget test for state restoration is untouched so all the state restoration that worked before still does.
The detail screen route is always a subroute of the path that opened it. For example, opening details for item 1 from /list goes to /list/details/1, opening details for the same item from favorites is /favorites/details/1, etc. This warranties that the navigation stack is always preserved when restoring the app.
Trying to use push did not work here, like pushing /details/1 from /list, as the navigation stack was lost during restoration. This could be due to a limitation in the go_router package. The previous implementation used restorablePush which does not exist on go_router.

Pre-launch Checklist

I read the Flutter Style Guide recently, and have followed its advice.
I signed the CLA.
I read the Contributors Guide.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

miquelbeltran · 2022-12-23T15:22:30Z

@domesticmouse @johnpryan I'm having problems with the 'restoration smoke test' in the veggieseasons app when using go_router, part of the restoration tests work, but some don't. For example, the issue I am having is that the scroll position is not being restored when the route is restored, but seems that route restoration works as I am able to restore a route to a details screen.

Could you do a review on the current PR code and see if you can find anything that I may have missed?

Doing a bit of research, I've found this issue: flutter/flutter#99124 and this PR here: flutter/packages#2650 could this be related as well?

miquelbeltran · 2022-12-23T15:23:07Z

veggieseasons/lib/main.dart

+          },
+          debugLogDiagnostics: true,
+          routes: [
+            ShellRoute(


Using a ShellRoute to present the contents for the navigation tabs.

miquelbeltran · 2022-12-23T15:24:53Z

veggieseasons/lib/screens/home.dart

+    final index = _getSelectedIndex(GoRouter.of(context).location);
    return RestorationScope(
      restorationId: restorationId,
-      child: CupertinoTabScaffold(


CupertinoTabScaffold is gone, and is replaced by a CupertinoPageScaffold that contains the ShellRoute child and the CupertinoTabBar instead.

miquelbeltran · 2022-12-23T15:26:21Z

veggieseasons/lib/main.dart

+              builder: (context, state, child) {
+                return HomeScreen(
+                  restorationId: 'home',
+                  child: child,


pass the ShellRoute child to the HomeScreen, will be displayed inside the CupertinoTabScaffold

miquelbeltran · 2022-12-27T15:49:16Z

After a lot of trial and error, I think that GoRouter is the main cause on why the state of the screens is not correctly restored, I have created a new issue on the Flutter repo with all the info I could collect as well as widget tests that can reproduce the issue: flutter/flutter#117683

My conclusion is that simply put, GoRouter doesn't restore the state of the screens it creates inside its router configuration, and I fail to see what is needed to be done for that to work. I couldn't find any examples or documentation about this, so I am following up, and I'll try to update any relevant docs.

domesticmouse · 2023-01-12T01:32:28Z

@johnpryan love to get your eyes on this

domesticmouse · 2023-02-06T03:32:06Z

@johnpryan PTAL

miquelbeltran · 2023-02-20T08:22:25Z

According to the responses in flutter/flutter#117683 (comment) this should work with a combination of using pageBuilder instead of builder and having a restoration id on the MaterialPage/CupertinoPage widget.

I will have a look and retake this

domesticmouse · 2023-02-21T01:55:01Z

Looks like we have some lint fixes required. dart fix --apply?

miquelbeltran · 2023-02-21T06:32:40Z

Yes, I am still fixing the unit tests. Part of the restoration tests are working, except for the details screen.

The problem is with the go_router push function. Previously, the code used restorablePush from the Navigator API to open the details screen, however looks like go_router does not implement something like that, and I need to find an alternative for this.

Otherwise, we are getting some progress here.

domesticmouse · 2023-02-21T06:35:38Z

Cool, ping me when this is ready for review. Thanks!

miquelbeltran · 2023-02-21T13:49:27Z

veggieseasons/lib/main.dart

+final _rootNavigatorKey = GlobalKey<NavigatorState>();
+final _shellNavigatorKey = GlobalKey<NavigatorState>();


Different navigator key helps to open the detail pages inside or outside the shell navigator as a top modal screen

miquelbeltran · 2023-02-21T13:50:50Z

veggieseasons/lib/main.dart

    );
  }
+
+  GoRoute _buildDetailsRoute() {


The details route is shared between the /list, /favorites and /search paths, but works exactly the same

miquelbeltran · 2023-02-21T13:52:47Z

veggieseasons/lib/widgets/fade_transition_page.dart

+
+import 'package:flutter/cupertino.dart';
+
+class FadeTransitionPage<T> extends Page<T> {


From https://github.com/flutter/samples/blob/3a0a652984e9b974342d172b9f0ffa161d0dcb2f/navigation_and_routing/lib/src/widgets/fade_transition_page.dart with the added restorationId

Hmm, maybe CustomTransitionPage needs add support for the restorationId, but this LGTM for now.

miquelbeltran · 2023-02-21T13:53:52Z

veggieseasons/lib/widgets/veggie_headline.dart

    return GestureDetector(
-      onTap: () => DetailsScreen.show(Navigator.of(context), veggie.id),
+      onTap: () {
+        context.go('${GoRouter.of(context).location}/details/${veggie.id}');


Allows to dynamically call this from either /favorites or /search, and builds /favorites/details/1 or /search/details/1

Yeah, go_router doesn't have support for relative routes (flutter/flutter#108177), it might be a good idea to add a comment here to explain what this line is doing.

miquelbeltran · 2023-02-21T13:54:52Z

@domesticmouse ready for review now, I updated the PR description with my implementation details

domesticmouse

LGTM

domesticmouse · 2023-02-21T22:59:26Z

PTAL @johnpryan

johnpryan

LGTM overall, but the fade transitions aren't working for me.

johnpryan · 2023-02-22T21:21:20Z

veggieseasons/lib/main.dart

+                  pageBuilder: (context, state) {
+                    return FadeTransitionPage(
+                      restorationId: 'route.list',
+                      child: Builder(builder: (context) {


I'm curious, why are you using a Builder for these routes?

Right, they don't seem to be necessary here!

johnpryan · 2023-02-22T21:21:55Z

veggieseasons/lib/main.dart

+                GoRoute(
+                  path: '/list',
+                  pageBuilder: (context, state) {
+                    return FadeTransitionPage(


I'm not seeing a fade transition when I tap on destinations in the bottom navigation bar - is that what this is supposed to do?

Edit: All good now! Turns out I had to pass the state.pageKey to the FadeTransitionPage in order to work.

johnpryan · 2023-02-22T21:24:13Z

veggieseasons/lib/widgets/fade_transition_page.dart

+
+import 'package:flutter/cupertino.dart';
+
+class FadeTransitionPage<T> extends Page<T> {


Hmm, maybe CustomTransitionPage needs add support for the restorationId, but this LGTM for now.

johnpryan · 2023-02-22T21:25:49Z

veggieseasons/lib/widgets/veggie_headline.dart

    return GestureDetector(
-      onTap: () => DetailsScreen.show(Navigator.of(context), veggie.id),
+      onTap: () {
+        context.go('${GoRouter.of(context).location}/details/${veggie.id}');


Yeah, go_router doesn't have support for relative routes (flutter/flutter#108177), it might be a good idea to add a comment here to explain what this line is doing.

miquelbeltran · 2023-02-23T14:06:22Z

Screencast.from.23.02.2023.15.05.40.webm

FYI the transition animation is now fixed.

johnpryan

LGTM - the CI failure looks unrelated to this change.

domesticmouse · 2023-02-23T20:49:24Z

LGTM - the CI failure looks unrelated to this change.

There's an open bug tracking that failure, I don't know how to fix it.

domesticmouse · 2023-02-23T21:07:03Z

@miquelbeltran land when you are ready

miquelbeltran added 8 commits December 22, 2022 15:05

add go_router

7ce8a9f

wip migration to go_router

0caacca

small fixes

489c5a9

home screen cleanup

47907b4

remove unused

0e8f2d3

small fixes

69ddfe4

details should be fullscreen dialog

197f4ee

remove comment

9f3fda1

miquelbeltran commented Dec 23, 2022

View reviewed changes

fix navigation outside the shell by using the correct navigation keys

fb30b28

miquelbeltran mentioned this pull request Dec 27, 2022

[go_router] Fail to restore widget states after "restart and restore" when using go_router flutter/flutter#117683

Open

domesticmouse requested a review from johnpryan January 12, 2023 01:32

Merge branch 'master' of https://github.com/flutter/samples into pr/1544

dbf0c38

domesticmouse changed the title ~~WIP migrate veggieseasons to go_router~~ WIP migrate veggieseasons to go_router Feb 6, 2023

miquelbeltran added 5 commits February 20, 2023 13:44

Merge branch 'main' into mb_veggieseasons_gorouter

2b3d4f7

add restoration id to all pages

760cae5

test passing, but parts are commented out, wip

c212d94

uncommented more test code

aa8085c

Add TODOs

f782fe1

fix lint issues

57f03ce

fix tests

b778d7d

miquelbeltran changed the title ~~WIP migrate veggieseasons to go_router~~ Migrate veggieseasons to go_router Feb 21, 2023

use FadeTransitionPage

acadc91

miquelbeltran commented Feb 21, 2023

View reviewed changes

miquelbeltran marked this pull request as ready for review February 21, 2023 13:54

miquelbeltran requested a review from domesticmouse February 21, 2023 13:54

domesticmouse approved these changes Feb 21, 2023

View reviewed changes

johnpryan approved these changes Feb 22, 2023

View reviewed changes

miquelbeltran added 4 commits February 23, 2023 14:00

remove unnecessary builders

d13bcfd

FadeTransitionPage same as CustomTransitionPage

a3531da

add comments regarding relative routes

6f86894

add missing pageKey

13926ae

add missing const

23242fc

johnpryan approved these changes Feb 23, 2023

View reviewed changes

miquelbeltran merged commit 8c06626 into flutter:main Feb 24, 2023

miquelbeltran deleted the mb_veggieseasons_gorouter branch February 24, 2023 07:18

		final _rootNavigatorKey = GlobalKey<NavigatorState>();
		final _shellNavigatorKey = GlobalKey<NavigatorState>();


		import 'package:flutter/cupertino.dart';

		class FadeTransitionPage<T> extends Page<T> {

Conversation

miquelbeltran commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

miquelbeltran commented Dec 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miquelbeltran commented Dec 27, 2022

Uh oh!

domesticmouse commented Jan 12, 2023

Uh oh!

domesticmouse commented Feb 6, 2023

Uh oh!

miquelbeltran commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domesticmouse commented Feb 21, 2023

Uh oh!

miquelbeltran commented Feb 21, 2023

Uh oh!

domesticmouse commented Feb 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miquelbeltran commented Feb 21, 2023

Uh oh!

domesticmouse left a comment

Choose a reason for hiding this comment

Uh oh!

domesticmouse commented Feb 21, 2023

Uh oh!

johnpryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miquelbeltran Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miquelbeltran commented Feb 23, 2023

Uh oh!

johnpryan left a comment

Choose a reason for hiding this comment

Uh oh!

domesticmouse commented Feb 23, 2023

Uh oh!

domesticmouse commented Feb 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

miquelbeltran commented Dec 22, 2022 •

edited

Loading

miquelbeltran commented Feb 20, 2023 •

edited

Loading

miquelbeltran Feb 23, 2023 •

edited

Loading