Skip to content

[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

Closed

Conversation

johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Feb 22, 2023

Depends on #2730

Fixes flutter/flutter#111909

@johnpryan johnpryan requested a review from chunhtai as a code owner February 22, 2023 20:43
@flutter-dashboard
Copy link

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be reverted?

Copy link
Contributor Author

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);
Copy link
Contributor

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

Copy link
Contributor Author

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.

@johnpryan
Copy link
Contributor Author

I'm trying to add an example for ShellRouteData, but I'm stuck because the .g.dart file isn't getting generated:

// 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');
  }
}

@chunhtai chunhtai changed the title Add ShellRoute support to go_router_builder [go_router_builder] Add ShellRoute support to go_router_builder Feb 23, 2023
@chunhtai
Copy link
Contributor

chunhtai commented Mar 9, 2023

Hi @johnpryan Do you know what's the state of this PR?

@johnpryan
Copy link
Contributor Author

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:

/// Supports `package:build_runner` creation and configuration of
/// `go_router`.
///
/// Not meant to be invoked by hand-authored code.
Builder goRouterBuilder(BuilderOptions options) => SharedPartBuilder(
      const <Generator>[GoRouterGenerator()],
      'go_router',
    );

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.

@GP4cK
Copy link
Contributor

GP4cK commented Mar 10, 2023

@chunhtai I created a PR johnpryan#1 against @johnpryan 's branch:

I created a GoRouterShellGenerator by duplicating the code from the GoRouterGenerator. It's exactly the same except that I just replaced TypedGoRoute with TypedShellRoute and GoRouteData with ShellRouteData. I should probably refactor it to avoid having duplicated code. I could:

  1. create an abstract class named (?) which both GoRouterGenerator and GoRouterShellGenerator would implement
  2. add 2 String parameters to GoRouterGenerator (typedRoute and routeData) and create the generator like this: GoRouterGenerator(typedRoute: 'TypedGoRoute', routeData: 'GoRouteData') and GoRouterGenerator(typedRoute: 'TypedShellRoute', routeData: 'ShellRouteData')
  3. (?)

Based on your feedback, I'll add a test similar to the existing one for the GoRouterGenerator.

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...

@chunhtai
Copy link
Contributor

Hi @GP4cK thanks for helping, can you create a PR directly against the flutter:main? I will close this pr.

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_builder] Add go_router v5 support to go_router_builder
3 participants