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

context.replace does not animate the extent of NavigationSheet #188

Open
samuelkubinsky opened this issue Jul 5, 2024 · 13 comments
Open
Labels
bug Something isn't working

Comments

@samuelkubinsky
Copy link

samuelkubinsky commented Jul 5, 2024

So when using go and replace actions when navigating, two things happen:

  1. extent changes but with delay
  2. extent does not change at all

Versions:
smooth_sheets: 0.8.1
go_router: 14.2.0

Screen.Recording.2024-07-05.at.22.31.00.mov

Blue screen has 1 proportional extent - 0.5
Red screen has 1 proportional extent - 0.9

Code
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
import 'package:smooth_sheets/smooth_sheets.dart';

void main() {
  runApp(MyApp());
}

final transitionObserver = NavigationSheetTransitionObserver();

class MyApp extends StatelessWidget {
  MyApp({super.key});

  final router = GoRouter(
    initialLocation: "/blue",
    routes: [
      ShellRoute(
          observers: [transitionObserver],
          builder: (context, state, child) {
            return NavigationSheet(
              transitionObserver: transitionObserver,
              child: Material(
                color: Theme.of(context).colorScheme.primary,
                borderRadius: BorderRadius.circular(16),
                clipBehavior: Clip.antiAlias,
                child: child,
              ),
            );
          },
          routes: [
            GoRoute(
              path: "/blue",
              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  initialExtent: Extent.proportional(0.5),
                  minExtent: Extent.proportional(0.5),
                  maxExtent: Extent.proportional(0.5),
                  child: PageBlue(),
                );
              },
            ),
            GoRoute(
              path: "/red",
              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  initialExtent: Extent.proportional(0.9),
                  minExtent: Extent.proportional(0.9),
                  maxExtent: Extent.proportional(0.9),
                  child: PageRed(),
                );
              },
            ),
          ])
    ],
  );

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      routerConfig: router,
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
    );
  }
}

class PageBlue extends StatelessWidget {
  final List<String> items = List<String>.generate(20, (index) => "Item ${index + 1}");

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('Blue (0.5)'),
        backgroundColor: Colors.blue,
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return InkWell(
            onTap: () {
              context.go("/red");
            },
            child: ListTile(
              title: Text(items[index]),
            ),
          );
        },
      ),
    );
  }
}

class PageRed extends StatelessWidget {
  final List<String> items = List<String>.generate(20, (index) => "Item ${index + 1}");

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('Red (0.9)'),
        backgroundColor: Colors.red,
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return InkWell(
            onTap: () {
              context.go("/blue");
            },
            child: ListTile(
              title: Text(items[index]),
            ),
          );
        },
      ),
    );
  }
}
@samuelkubinsky samuelkubinsky changed the title Navigation go and replace do not work reliably Navigation go and replace actions do not work reliably Jul 5, 2024
@fujidaiti
Copy link
Owner

Hi @samuelkubinsky,

I recommend specifying a key for each *Page in the go_router configuration, as the Navigator may reuse an existing Route object if possible, resulting in the transition animation not running.

              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  key: state.pageKey, // <-- this

@fujidaiti fujidaiti added the question Further information is requested label Jul 6, 2024
@samuelkubinsky
Copy link
Author

Thanks that helped. I don't think this question belong here but I didn't find an answer anywhere. How can i get rid of "push" animation when replacing a route?

@fujidaiti
Copy link
Owner

fujidaiti commented Jul 6, 2024

You can create custom transition animations by specifying a custom transition builder to *NavigationSheetPage.transitionsBuilder in the constructor. This page could be helpful to implement the transition builder.

@samuelkubinsky
Copy link
Author

samuelkubinsky commented Jul 6, 2024

How can i get rid of "push" animation when replacing a route?

I am not sure what you mean by "push animation". Can you post a screenshot or something?

Ok, my bad. Using default APIs context.go is treated with instant animation, but with this library i need to use context.replace which is not big deal, but if i use replace, extent is not animated.

Code
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
import 'package:smooth_sheets/smooth_sheets.dart';

void main() {
  runApp(MyApp());
}

final transitionObserver = NavigationSheetTransitionObserver();

class MyApp extends StatelessWidget {
  MyApp({super.key});

  final router = GoRouter(
    initialLocation: "/blue",
    routes: [
      ShellRoute(
          observers: [transitionObserver],
          builder: (context, state, child) {
            return NavigationSheet(
              transitionObserver: transitionObserver,
              child: Material(
                color: Theme.of(context).colorScheme.primary,
                borderRadius: BorderRadius.circular(16),
                clipBehavior: Clip.antiAlias,
                child: child,
              ),
            );
          },
          routes: [
            GoRoute(
              path: "/blue",
              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  key: state.pageKey,
                  initialExtent: const Extent.proportional(0.5),
                  minExtent: const Extent.proportional(0.5),
                  maxExtent: const Extent.proportional(0.5),
                  child: PageBlue(),
                );
              },
            ),
            GoRoute(
              path: "/red",
              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  key: state.pageKey,
                  initialExtent: const Extent.proportional(0.9),
                  minExtent: const Extent.proportional(0.9),
                  maxExtent: const Extent.proportional(0.9),
                  child: PageRed(),
                );
              },
            ),
          ])
    ],
  );

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      routerConfig: router,
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
    );
  }
}

class PageBlue extends StatelessWidget {
  final List<String> items = List<String>.generate(20, (index) => "Item ${index + 1}");

  PageBlue({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('SmoothSheets Replace'),
        backgroundColor: Colors.blue,
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return InkWell(
            onTap: () {
              context.replace("/red");
            },
            child: ListTile(
              title: Text(items[index]),
            ),
          );
        },
      ),
    );
  }
}

class PageRed extends StatelessWidget {
  final List<String> items = List<String>.generate(20, (index) => "Item ${index + 1}");

  PageRed({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('SmoothSheets Replace'),
        backgroundColor: Colors.red,
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return InkWell(
            onTap: () {
              context.replace("/blue");
            },
            child: ListTile(
              title: Text(items[index]),
            ),
          );
        },
      ),
    );
  }
}

@samuelkubinsky
Copy link
Author

samuelkubinsky commented Jul 6, 2024

You can create custom transition animations by specifying a custom transition builder to *NavigationSheetPage.transitionsBuilder in the constructor. This page could be helpful to implement the transition builder.

Thanks for the tip. I tried some animations and I got them to work but I ultimately lost pop by user gesture. Wouldn't it be better if this library used default animations? So out of the box it behaves as you would expect and if you want something custom, then you can tweak it to your liking.

Screen.Recording.2024-07-06.at.23.15.42.mov

@fujidaiti
Copy link
Owner

Can you tell me the expected behavior? Also i want to know the code of your transitionsBuilder.

@samuelkubinsky
Copy link
Author

samuelkubinsky commented Jul 7, 2024

I'm not sure which comment are you referring to, so I'm going to describe both issues.

1.) On context.replace invoke, expected behaviour would be new screen replacing old one with no animation, but extent animating to initialExtent of new screen.
2.) Here I did not use any custom transitions. In Default Push and Default Pop I used DraggableScrollableSheet with MaterialPage to demonstrate expected (native) screen transitions. In SmoothSheets Push and SmoothSheets Pop I used NavigationSheet with ScrollableNavigationSheetPage to demonstrate current screen transitions.

@fujidaiti
Copy link
Owner

fujidaiti commented Jul 7, 2024

I tried some animations and I got them to work but I ultimately lost pop by user gesture.

Oh, sorry. I replied to this (↑) comment.

Here I did not use any custom transitions. In Default Push and Default Pop I used DraggableScrollableSheet with MaterialPage to demonstrate expected (native) screen transitions. In SmoothSheets Push and SmoothSheets Pop I used NavigationSheet with ScrollableNavigationSheetPage to demonstrate current screen transitions.

You mean the expected behavior is that the sheet uses the native (material or cupertino) transition animation, while keeping the sheet height animation is also enabled? If so, the following custom transition builder could be a solution (not tested yet):

    // This is exactly the same implementation as MaterialPageRoute
    final theme = Theme.of(context).pageTransitionsTheme;
    return theme.buildTransitions<T>(ModalRoute.of(context) as PageRoute<dynamic>, context, animation, secondaryAnimation, child);

wouldn't it be better if this library used default animations? So out of the box it behaves as you would expect and if you want something custom, then you can tweak it to your liking.

I agree with this idea. I can't remember why I didn't so :(

@samuelkubinsky
Copy link
Author

Provided code works excellently, thanks! And I think this should be a default transition. So the only issue I'm having with this library is this one:

1.) On context.replace invoke, expected behaviour would be new screen replacing old one with no animation, but extent animating to initialExtent of new screen.

@samuelkubinsky samuelkubinsky changed the title Navigation go and replace actions do not work reliably context.replace does not animate the extent Jul 7, 2024
@samuelkubinsky
Copy link
Author

samuelkubinsky commented Jul 7, 2024

To be fair, extent is animated but only sometimes - after some manual scrolling in a screen.

Screen.Recording.2024-07-07.at.18.19.06.mov
Code
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
import 'package:smooth_sheets/smooth_sheets.dart';

void main() {
  runApp(MyApp());
}

final transitionObserver = NavigationSheetTransitionObserver();

class MyApp extends StatelessWidget {
  MyApp({super.key});

  final router = GoRouter(
    initialLocation: "/blue",
    routes: [
      ShellRoute(
          observers: [transitionObserver],
          builder: (context, state, child) {
            return NavigationSheet(
              transitionObserver: transitionObserver,
              child: Material(
                color: Theme.of(context).colorScheme.primary,
                borderRadius: BorderRadius.circular(16),
                clipBehavior: Clip.antiAlias,
                child: child,
              ),
            );
          },
          routes: [
            GoRoute(
              path: "/blue",
              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  key: state.pageKey,
                  initialExtent: const Extent.proportional(0.5),
                  minExtent: const Extent.proportional(0.5),
                  maxExtent: const Extent.proportional(0.5),
                  child: PageBlue(),
                  transitionsBuilder: (context, animation, secondaryAnimation, child) {
                    final theme = Theme.of(context).pageTransitionsTheme;
                    final route = ModalRoute.of(context) as PageRoute<dynamic>;
                    return theme.buildTransitions(route, context, animation, secondaryAnimation, child);
                  },
                );
              },
            ),
            GoRoute(
              path: "/red",
              pageBuilder: (context, state) {
                return ScrollableNavigationSheetPage(
                  key: state.pageKey,
                  initialExtent: const Extent.proportional(0.9),
                  minExtent: const Extent.proportional(0.5),
                  maxExtent: const Extent.proportional(0.9),
                  child: PageRed(),
                  transitionsBuilder: (context, animation, secondaryAnimation, child) {
                    final theme = Theme.of(context).pageTransitionsTheme;
                    final route = ModalRoute.of(context) as PageRoute<dynamic>;
                    return theme.buildTransitions(route, context, animation, secondaryAnimation, child);
                  },
                );
              },
            ),
          ])
    ],
  );

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      routerConfig: router,
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
    );
  }
}

class PageBlue extends StatelessWidget {
  final List<String> items = List<String>.generate(20, (index) => "Item ${index + 1}");

  PageBlue({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Initial 0.5'),
        backgroundColor: Colors.blue,
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return InkWell(
            onTap: () {
              context.replace("/red");
            },
            child: ListTile(
              title: Text(items[index]),
            ),
          );
        },
      ),
    );
  }
}

class PageRed extends StatelessWidget {
  final List<String> items = List<String>.generate(20, (index) => "Item ${index + 1}");

  PageRed({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Initial 0.9'),
        backgroundColor: Colors.red,
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return InkWell(
            onTap: () {
              context.replace("/blue");
            },
            child: ListTile(
              title: Text(items[index]),
            ),
          );
        },
      ),
    );
  }
}

@fujidaiti
Copy link
Owner

So the only issue I'm having with this library is this one:

#188 (comment) On context.replace invoke, expected behaviour would be new screen replacing old one with no animation, but extent animating to initialExtent of new screen.

Can't we simply use context.go? Or is there a reason it has to be replace?

@samuelkubinsky
Copy link
Author

go uses transition between pages whereas replace does not. In my app, I have bottom navigation bar from which I wanted to switch between sheet screens without transition, as this is default behaviour on iOS (UITabBarController).

I still think this could be fixed but I ultimately gave up on Cupertino transitions and started using ZoomPage on all platforms as it looks better in a sheet. Thanks for your wonderful work!

@fujidaiti fujidaiti changed the title context.replace does not animate the extent context.replace does not animate the extent of NavigationSheet Jul 9, 2024
@fujidaiti fujidaiti added bug Something isn't working and removed question Further information is requested labels Jul 9, 2024
@fujidaiti
Copy link
Owner

I will keep this issue open as a bug report, not a question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants