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

[flutter_adaptive_scaffold] Go router sample for AdaptiveScaffold #7452

Merged
merged 19 commits into from
Sep 4, 2024

Conversation

martijn00
Copy link
Contributor

@martijn00 martijn00 commented Aug 20, 2024

This implements a sample of using GoRouter with AdaptiveScaffold. It also helps testing advanced adaptive scenarios.

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#129850

Pre-launch Checklist

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

@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, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@martijn00
Copy link
Contributor Author

@chunhtai can you have a look at this one too?

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@martijn00
Copy link
Contributor Author

@Renzo-Olivares can you have a look?

@martijn00
Copy link
Contributor Author

@hanskokx could you pitch in with your research findings on this sample?

@hanskokx
Copy link

hanskokx commented Aug 29, 2024

From my experience, AdaptiveScaffold works optimally when paired with AdaptiveLayout. However, there is an issue in flutter_adaptive_scaffold version 0.2.x where the leadingExtendedNavRail does not show up (I've been meaning to file a bug on this). Below is the approach I've utilized in production over the last few years:

class MainApp extends StatelessWidget {
  const MainApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
        routeInformationProvider: AppRouter.router.routeInformationProvider,
        routeInformationParser: AppRouter.router.routeInformationParser,
        routerDelegate: AppRouter.router.routerDelegate,
      ),
    );
  }
}


enum AppRoute {
  root("/", isProtected: false),
  home("/splash", isProtected: false),
  home("/welcome", isProtected: false),
  home("/login", isProtected: false),
  home("/register", isProtected: false),
  home("/home"),
  ;

  final String path;
  final bool isProtected;

  const AppRoute(this.path, {this.isProtected = true});
}

class AppRouter {
  static final router = GoRouter(
    errorBuilder: (context, state) => const ErrorScreen(),
    navigatorKey: GlobalKey<NavigatorState>(debugLabel: "root"),
    initialLocation: AppRoute.splash.path,
    debugLogDiagnostics: kDebugMode,
    redirect: (BuildContext context, GoRouterState state) {
      final bool routeIsProtected = AppRoute.values.any(
        (AppRoute route) =>
            state.uri.toString().contains(route.path) && route.isProtected,
      );

      final bool unauthenticated = AuthenticationService.I.status ==
          AuthenticationStatus.unauthenticated;

      if (unauthenticated && routeIsProtected) {
        return state.namedLocation(
          AppRoute.welcome.name,
          queryParameters: <String, String>{"from": state.uri.toString()},
        );
      }
      return null;
    },
    routes: <RouteBase>[
      GoRoute(
        path: AppRoute.root.path,
        redirect: (_, __) => AppRoute.home.path,
      ),
      GoRoute(
        name: AppRoute.splash.name,
        path: AppRoute.splash.path,
        pageBuilder: (context, state) => MaterialPage(
          key: state.pageKey,
          child: SplashScreen(),
        );
      ),
      // * Welcome
      GoRoute(
        name: AppRoute.welcome.name,
        path: AppRoute.welcome.path,
        pageBuilder: (context, state) {
          return MaterialPage(
            key: state.pageKey,
            child: const WelcomeScreen(),
          );
        },
        routes: [
          // Login
          GoRoute(
            name: AppRoute.login.name,
            path: AppRoute.login.path,
            builder: (context, state) => LoginScreen();
            routes: [
              // Forgot password
              GoRoute(
                name: AppRoute.forgotPassword.name,
                path: AppRoute.forgotPassword.path,
                builder: (context, state) => AppScaffold(
                    key: state.pageKey,
                    body: const ForgotPasswordScreen(),
                  );
              ),
            ],
          ),
          // Register
          GoRoute(
            name: AppRoute.register.name,
            path: AppRoute.register.path,
            builder: (context, state) => const RegisterScreen();
          ),
        ],
      ),

      StatefulShellRoute.indexedStack(
        builder: (context, state, navigationShell) => AppScaffoldShell(
          navigationShell: navigationShell,
        ),
        branches: <StatefulShellBranch>[
          // Home branch
          StatefulShellBranch(
            initialLocation: AppRoute.home.path,
            navigatorKey: GlobalKey<NavigatorState>(debugLabel: "home"),
            routes: [
              GoRoute(
                name: AppRoute.home.name,
                path: AppRoute.home.path,
                pageBuilder: (context, state) {
                  return NoTransitionPage(
                    child: AppScaffold(
                      key: state.pageKey,
                      body: HomeScreen(
                        key: GlobalKey(debugLabel: "Home Screen"),
                      ),
                    ),
                  );
                },
              ),
            ],
          ),
        ],
      ),
    ],
  );
}

extension LandscapeExtension on BuildContext {
  bool get isLandscape =>
      MediaQuery.of(this).orientation == Orientation.landscape ||
      layoutSize != LayoutSize.small;
  LayoutSize get layoutSize {
    if (Breakpoints.large.isActive(this)) {
      return LayoutSize.large;
    } else if (Breakpoints.medium.isActive(this)) {
      return LayoutSize.medium;
    } else {
      return LayoutSize.small;
    }
  }
}

enum LayoutSize { small, medium, large }


class AppScaffold extends StatelessWidget {
  final Widget body;
  final Widget? secondaryBody;

  const AppScaffold({
    required this.body,
    Key? key,
    this.secondaryBody,
  }) : super(key: key ?? const ValueKey("ScaffoldWithNestedNavigation"));

  @override
  Widget build(BuildContext context) {
    return AdaptiveLayout(
      internalAnimations: false,
      body: SlotLayout(
        config: <Breakpoint, SlotLayoutConfig>{
          Breakpoints.small: SlotLayout.from(
            key: const Key("Body Small"),
            builder: (context) {
              final Widget child = secondaryBody ?? body;
              return context.isLandscape ? SafeArea(child: child) : child;
            },
          ),
          Breakpoints.mediumAndUp: SlotLayout.from(
            key: const Key("Body Medium and Up"),
            builder: (context) => context.isLandscape ? SafeArea(child: body) : body;
          ),
        },
        secondaryBody: SlotLayout(
          config: <Breakpoint, SlotLayoutConfig>{
            Breakpoints.small: SlotLayout.from(
              key: const Key("Body Small"),
              builder: null,
            ),
            Breakpoints.mediumAndUp: SlotLayout.from(
              key: const Key("Body Medium"),
              builder: (context) => secondaryBody,
            ),
          },
        ),
      ),
    );
  }
}

class AppScaffoldShell extends StatelessWidget {
  final StatefulNavigationShell navigationShell;
  const AppScaffoldShell({
    required this.navigationShell,
    Key? key,
  }) : super(key: key ?? const ValueKey("ScaffoldWithNestedNavigation"));

  @override
  Widget build(BuildContext context) {
    return AdaptiveScaffold(
      selectedIndex: navigationShell.currentIndex,
      onSelectedIndexChange: onNavigationEvent,
      destinations: [
        NavigationDestination(...),
        NavigationDestination(...),
        NavigationDestination(...),
      ],
      body: (_) => navigationShell,
    );
  }

  void onNavigationEvent(int index) {
    navigationShell.goBranch(
      index,
      initialLocation: index == navigationShell.currentIndex,
    );
  }
}

@martijn00
Copy link
Contributor Author

Thanks! I'm curious about the leadingExtendedNavRail bug. I'm working on new implementations that would make things easier. Currently the AdaptiveScaffold implementation is way to complicated.

https://docs.google.com/document/d/1DMFfGrHv_V1Je6_wrfyu0iY3ngJgn317OscfNbuRMRA/pub

Let me know if you have feedback on the doc.

@hanskokx
Copy link

Thanks! I'm curious about the leadingExtendedNavRail bug.

I've recorded the behavior I'm seeing in 0.1.12 vs 0.2.2. I haven't had time yet to dig into it and figure out what changed and/or what's broken. It's been on my todo list but I'm right at the end of this project and have needed to focus on getting it shipped.

0.1.12.mov
0.2.2.mov

I'm working on new implementations that would make things easier. Currently the AdaptiveScaffold implementation is way to complicated.

Yeah, that would be great! As you can see from the code I provided before, the true implementation requires far more work than is reasonable. (I even cut a bunch out of that example, such as where I'm disabling animations because they look wonky.) A more streamlined approach would be greatly appreciated.

https://docs.google.com/document/d/1DMFfGrHv_V1Je6_wrfyu0iY3ngJgn317OscfNbuRMRA/pub
Let me know if you have feedback on the doc.

It would be beneficial to have a feature that allows users to resize panes, possibly through a drag handle that becomes visible when tapping between panes. Naturally, this feature would be optional.

Adding panes dynamically would be a useful feature. Consider a list within a list scenario: the first pane displays a list of items, and upon selection, a second pane appears, showing further details and another list. Selecting an item from this secondary list could lead to a deeper list in a new pane or reveal more details about the item. By the third level, the initial pane could automatically hide to prevent screen clutter. (Feel free to disregard if this seems far-fetched.)

@martijn00
Copy link
Contributor Author

@hanskokx I think you are correct this is a bug. Probably in this code:

final bool isHeightActive = isDesktop ||
    orientation == Orientation.portrait ||
    (orientation == Orientation.landscape && andUp
        ? isWidthActive || height >= lowerBoundHeight
        : height >= lowerBoundHeight && height < upperBoundHeight);

I have to look into that to figure out where it is.

@martijn00
Copy link
Contributor Author

@hanskokx because the device size is a width of 841 and a height of 668 it can't be active on mediumLarge because the height starts at 900 but cannot be active at medium because the width doesn't match. Therefor it falls back to standard which is not good in this situation.

I'm not sure yet how to handle this, but my feeling is the breakpoint to apply is mediumLarge. This could be done by lowering the height to maybe 480? The specs are just not very clear on this.

@hanskokx
Copy link

@martijn00 Do you know what PR/commit the behavior was changed in? That might provide some insights into why the change was made and illuminate a path forward.

@martijn00
Copy link
Contributor Author

@hanskokx in this pr #7300

@hanskokx
Copy link

@hanskokx in this pr #7300

I wonder if you are a prophet 😅. #7300 (comment)
I agree that your work in refactoring into the Breakpoints enum is the best place to start looking. Perhaps we should spin up a new bug ticket and migrate the conversation there, to keep this PR focused on the sample?

@martijn00
Copy link
Contributor Author

Yeah i agree a new issue is better. I'll look into adding a test for this case.

@martijn00
Copy link
Contributor Author

@hanskokx Here is a PR for it: #7549

@martijn00
Copy link
Contributor Author

@gspencergoog can you add: "override: no changelog needed" label.

@martijn00
Copy link
Contributor Author

@chunhtai can you check again?

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 4, 2024
@auto-submit auto-submit bot merged commit f408578 into flutter:main Sep 4, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 4, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 4, 2024
flutter/packages@848d7e9...e93995a

2024-09-04 [email protected] [google_maps_flutter_android] Convert `JointType` to an enum (flutter/packages#7558)
2024-09-04 [email protected] [flutter_adaptive_scaffold] Go router sample for AdaptiveScaffold (flutter/packages#7452)
2024-09-04 [email protected] [flutter_adaptive_scaffold] Fix breakpoint not being active in certain cases like foldables (flutter/packages#7549)
2024-09-03 [email protected] [google_sign_in_android] Downgrade Guava version from `33.3.0` to `32.0.1` (flutter/packages#7573)
2024-09-03 [email protected] [google_maps_flutter] Remove unused MapKit imports from iOS example apps (flutter/packages#7522)
2024-09-03 [email protected] [interactive_media_ads] Adds support for pausing and resuming Ad playback and skipping an Ad (flutter/packages#7285)
2024-09-03 [email protected] [rfw] Upgrade missed example app (flutter/packages#7545)
2024-09-03 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump exoplayer_version from 1.4.0 to 1.4.1 in /packages/video_player/video_player_android/android (flutter/packages#7564)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: flutter_adaptive_scaffold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants