fujidaiti / smooth_sheets

Sheet widgets with smooth motion and great flexibility.
https://pub.dev/packages/smooth_sheets
MIT License
180 stars 14 forks source link

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

Open samuelkubinsky opened 2 weeks ago

samuelkubinsky commented 2 weeks ago

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

![Video demonstrating this]

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 items = List.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 items = List.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]), ), ); }, ), ); } } ```
fujidaiti commented 2 weeks ago

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
samuelkubinsky commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 ```dart 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 items = List.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 items = List.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 commented 2 weeks ago

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.

![Video]

fujidaiti commented 1 week ago

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

samuelkubinsky commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

![Video]

Code ```dart 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; 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; 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 items = List.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 items = List.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 commented 1 week ago

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

https://github.com/fujidaiti/smooth_sheets/issues/188#issuecomment-2211964927 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 commented 1 week ago

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 commented 1 week ago

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