Fix OpenContainer source(closed) widget staying hidden when the OpenContainer route is removed when the OpenContainer route is pushed from a different Route #260
Conversation
shihaohong
left a comment
shihaohong
left a comment
There was a problem hiding this comment.
Thanks for putting forward this PR!
One issue it does have is that when the open container route is popped into the closed container route, the animation transition is the default one for the app and not the OpenContainer one. Is that possible?
| Navigator.pushReplacement<void, void>( | ||
| container, | ||
| MaterialPageRoute<void>(builder: (_) => const Placeholder()) | ||
| ); |
There was a problem hiding this comment.
nit:
| Navigator.pushReplacement<void, void>( | |
| container, | |
| MaterialPageRoute<void>(builder: (_) => const Placeholder()) | |
| ); | |
| Navigator.pushReplacement<void, void>( | |
| container, | |
| MaterialPageRoute<void>(builder: (_) => const Placeholder()), | |
| ); |
| await tester.tap(find.text('Closed')); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| final Element container = tester.element(find.byType(OpenContainer, skipOffstage: false)); |
There was a problem hiding this comment.
nit:
| final Element container = tester.element(find.byType(OpenContainer, skipOffstage: false)); | |
| final Element container = tester.element( | |
| find.byType(OpenContainer, skipOffstage: false), | |
| ); |
| }, | ||
| ); | ||
|
|
||
| testWidgets( |
There was a problem hiding this comment.
Add a comment here to help with context:
| testWidgets( | |
| // Regression test for https://github.com/flutter/flutter/issues/72238. | |
| testWidgets( |
| ); | ||
|
|
||
| testWidgets( | ||
| 'OpenContainer works when the route is removed', |
There was a problem hiding this comment.
Maybe:
| 'OpenContainer works when the route is removed', | |
| 'OpenContainer's source widget is visible in closed container route if open container ' | |
| 'route is pushed from not using the OpenContainer itself', |
I'm trying to make this description more helpful, but I think that the link to the issue does help enough
shihaohong
changed the title
I am not sure if i follow, can you explain a little bit more? |
|
Normal behavior (with open container transition animation): Fix behavior (no open container transition animation, uses default transition animation instead of the open container transition, but the hidden closed widget issue is fixed): |
|
Do you mean when going from page B back to page A, it is using the default transition of page B? If we want to make page B pop to use the open container animation, it means we will need to change the secondary animation of page B if it is on top of a page that has an open container. I can't think of a way to do that. The animation is built into the route object, and it is not mutable at this point. We will need to refactor it out to make it work. Another issue is that if there is more than one open container, we couldn't tell which one is the correct one either. in terms of the design, which is the correct behavior? The open container route is replaced by a new route, does user expect the new route still transition like the open container route? |
|
@chunhtai I'm not exactly sure what is considered the "correct" behavior either -- I brought it up because the behavior I was describing was what I expected, but this discussion can move elsewhere or until someone else is unhappy with it. It probably shouldn't stop this PR from being merged. |
|
Also, some of the formatting checks are failing. The logs describe how to run the code formatter: https://github.com/flutter/packages/pull/260/checks?check_run_id=1602579853 |
|
@shihaohong Thanks for review, I have addressed all comments and fixed the format |
I think a reverse openContainer animation should be played. The normal openContainer and accordingly 'closeContainer' animation. |
3 participants
The open container hide itself based on animation status of the container route. however, if the route is removed due to pushreplacement or pushremoveuntil, its animation controller will be disposed without status changes. this will hide the open container forever. This pr fixes it.
fixes flutter/flutter#72238