csells / go_router

The purpose of the go_router for Flutter is to use declarative routes to reduce complexity, regardless of the platform you're targeting (mobile, web, desktop), handling deep linking from Android, iOS and the web while still allowing an easy-to-use developer experience.
https://gorouter.dev
442 stars 96 forks source link

feature: set a title for routes (and error pages) #207

Closed Kounex closed 2 years ago

Kounex commented 2 years ago

Especially when targeting the web, it's useful to be able to define the title name for a route or the error page. Depending on which browser / device is being used, the title may be displayed in different locations (on a tab, visited sites, autocorrected urls, etc.).

The implementation in this pull request is lightweight and will only optionally wrap the Title widget, which is enabling this in the first place, if a title is being provided via the title / errorTitle property. Otherwise it will work as previously and users can still decide to do it on their own (for example if they want to make it more dynamically inside their routes)

As an example, I added titles for two routes and provided the errorTitle in main.dart (in the example folder).

csells commented 2 years ago

Why wouldn't I just do this instead?

GoRoute(path: '/foo', builder: (c,s) => Title(title: 'Foo', child: FooScreen())),
Kounex commented 2 years ago

Why wouldn't I just do this instead?

GoRoute(path: '/foo', builder: (c,s) => Title(title: 'Foo', child: FooScreen())),

This is what my implementation is doing basically. But this is effectively reducing redundant code in the actual application because I, as a user of this package, would need to do that manually in every occasion!

Additionally it assumes that people have to learn about that widget in the first place since it’s not obvious.

Lastly, I as a user of a routing package would appreciate that the package supports it out of the box since it goes hand in hand: defining the routes and giving them titles to use!

csells commented 2 years ago

But you'd have to do this manually anyway, e.g.

GoRoute(title: 'Foo', path: '/foo', builder: (c,s) => FooScreen()),

vs.

GoRoute(path: '/foo', builder: (c,s) => Title(title: 'Foo', child: FooScreen())),

The former represents a slippery slope adding Flutter features into go_router that you can already use today whereas the latter uses well known, existing Flutter features w/ go_router w/o having to invent, build, document and maintain new features redundant to existing Flutter features.

Kounex commented 2 years ago

Yes you need to do it manually anyway. Main aspects for me remain, that I would expect a routing package to offer this functionality on its own and that wrapping every view in its own codebase with the Title widget (since it' kinda mandatory especially in the web environment) is more redundant than providing the title as a single string respectively. My code, even in a small example, looks like this:

final _router = GoRouter(
  routes: [
    GoRoute(
      path: AppRoute.home.path,
      pageBuilder: (context, state) => MaterialPage<void>(
        key: state.pageKey,
        child: Title(
          title: 'Home',
          color: const Color(0xffffffff),
          child: AppRoute.home.view,
        ),
      ),
    ),
    GoRoute(
      path: AppRoute.items.path,
      pageBuilder: (context, state) => MaterialPage<void>(
        key: state.pageKey,
        child: Title(
          title: 'Items',
          color: const Color(0xffffffff),
          child: AppRoute.items.view,
        ),
      ),
      routes: [
        GoRoute(
          path: AppRoute.itemCategory.path,
          pageBuilder: (context, state) => MaterialPage<void>(
            key: state.pageKey,
            child: Title(
              title: 'Category',
              color: const Color(0xffffffff),
              child: AppRoute.itemCategory.view,
            ),
          ),
          routes: [
            GoRoute(
              path: AppRoute.itemDetail.path,
              pageBuilder: (context, state) => MaterialPage<void>(
                key: state.pageKey,
                child: Title(
                  title: 'Detail',
                  color: const Color(0xffffffff),
                  child: AppRoute.itemDetail.view,
                ),
              ),
            ),
          ],
        ),
      ],
    ),
    GoRoute(
      path: AppRoute.about.path,
      pageBuilder: (context, state) => MaterialPage<void>(
        key: state.pageKey,
        child: Title(
          title: 'About',
          color: const Color(0xffffffff),
          child: AppRoute.about.view,
        ),
      ),
    ),
  ],
  errorPageBuilder: (context, state) => MaterialPage<void>(
    key: state.pageKey,
    child: Title(
      title: '404',
      color: const Color(0xffffffff),
      child: AppRoute.unknown.view,
    ),
  ),
)

But of course your argument is valid - if you don't feel like this is the right approach and doesn't belong in this package then so be it! :)

csells commented 2 years ago

If that's your code, I'm about to save you lots of boilerplate by suggesting that you use v2.5+! In addition, might I suggest a little helper? The combo of the two reduces your code to something like this:

GoRoute(
  path: AppRoute.home.path,
  builder: (context, state) => _title('Home', AppRoute.home.view),
),

...

Widget _title(String title, Widget child) => Title(
  title: title,
  color: const Color(0xffffffff),
  child: child,
);