[go_router] [shell_route] Add observers parameter by angjelkom · Pull Request #2664 · flutter/packages

angjelkom · 2022-09-30T21:44:58Z

This PR adds observers parameter to the ShellRoute and passes that to the nested Navigator created by that same ShellRoute.

The need for this PR is because it fixes cases where a Hero animation doesn't work when used with nested navigation and it requires a HeroController to be used as an observer for that nested Navigator.

fixesflutter/flutter#112095

Pre-launch Checklist

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

flutter-dashboard · 2022-09-30T21:45:00Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

grimatoma

LGTM

chunhtai · 2022-11-11T20:33:18Z

Hi @angjelkom , are you still planning to work on this pr?

angjelkom · 2022-11-11T20:36:48Z

Hi @angjelkom , are you still planning to work on this pr?

Apologize, yes I will add your suggested change over the weeekend. I've been busy with work.

angjelkom · 2022-11-13T00:50:45Z

@chunhtai I will need a bit more time, as merging the latest commits from the main branch caused my example to have weird side-effects where the Hero animation doesn't work when going to the next page but when going back it works in a buggy-weird way.

So until i solve that I can't commit the change for HeroControllerScope.

chunhtai · 2022-11-30T19:35:22Z

@angjelkom are still planning on coming back to this pr?

angjelkom · 2022-12-03T13:19:11Z

@chunhtai Sorry I am too busy at the moment with work, I will for sure complete it by the end of next week!

angjelkom · 2022-12-06T19:22:38Z

@chunhtai I can't complete the pull request until #115832 its fixed, its blocking me.

I could use a lower commit but still I would need to test it against the latest commit at the end and currently that's not possible because of the above issue.

chunhtai · 2022-12-06T19:25:54Z

@angjelkom I will take a look at the issue and try to resolve it asap

ycherniavskyi · 2022-12-08T14:03:07Z

@angjelkom, the issue you mentioned has been fixed. Could you update your PR for merging? If you are too busy, I could create an analogic PR. I need it solely for FirebaseAnalyticsObserver.

angjelkom · 2022-12-08T16:53:34Z

@ycherniavskyi yes saw the merge today, I will proceed with completing the PR

angjelkom · 2022-12-08T21:16:44Z

@ycherniavskyi the PR is ready waiting for @chunhtai to review it and approve it.

arsen · 2022-12-13T21:46:09Z

any chance this will get merged soon?

angjelkom · 2022-12-14T08:08:19Z

@arsen I will get on it tonight to finish it up.

angjelkom · 2022-12-15T17:33:10Z

@chunhtai as @ycherniavskyi suggested we should use the relevant HeroController based on whether MaterialApp or CupertinoApp is used, in order to do that we need to use the RouteBuilder not the ShellRoute in order to access the context.

The issue is because the RouteBuilder gets rebuild while navigating the HeroController which again breaks the animation, the way I see it we have two options:

Option 1:

Create 3 final variables in the RouteBuilder, one using createMaterialHeroController, one using createCupertinoHeroController and a default HeroController.
Then we can use those to pass the correct controller.
This way the controllers won't be rebuild and the animation would work. (tested)

Option 2:

Have a heroController parameter to the ShellRoute so that the user passes one himself.

Let me know what would you recommend, I can do the change tonight.

chunhtai · 2023-02-09T22:49:48Z

Hi @angjelkom, it looks like @flodaniel has provided a test, can you commit to this pr so that i can merge it?

…ers_test.dart

Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>

@flodaniel

Credits to @flodaniel!

angjelkom · 2023-02-10T08:57:35Z

@flodaniel Thanks a lot for the test! @chunhtai merged it, but at the moment submit-queue check is failing for some other reason.

hannah-hyj

LGTM

lublot · 2023-02-14T14:40:54Z

Hello everyone! Is there any prediction of when it will be merged?

…ng a `ShellRoute` with `observers` (#5563) [This pr](#2664) added the ability to provide a list of `NavigatorObservers` to a `ShellRoute`. The equivalent was never added to `go_router_builder`, so this pr provides that functionality. This is a workaround to [this issue](flutter/flutter#112196) where navigator observers added to a `GoRouter` at the top level do not fire in `ShellRoutes`. With this change, you can provide `NavigatorObservers` directly to the `ShellRoute`

@flodaniel

* [go_router] [shell_route] Add observers parameter * [go_router] [shell_route] Add observers parameter test * Added Licence for shell_route_observers_test.dart * [go_router] [shell_route] Added type annotation to shell_route_observers_test.dart * [go_router] [shell_route] Use `HeroControllerScope` for nested Navigator * Use the correct HeroController based on the App type. * Cache the HeroController for the nested Navigator. * Clean up previous cache to prevent memory leak. * Added better cache-clearing policy for the HeroController cache. * Fixed Typos Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com> * Fixed Typos Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com> * [go_router] [shell_route] Added a better Hero test Credits to @flodaniel! --------- Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>

…ng a `ShellRoute` with `observers` (flutter#5563) [This pr](flutter#2664) added the ability to provide a list of `NavigatorObservers` to a `ShellRoute`. The equivalent was never added to `go_router_builder`, so this pr provides that functionality. This is a workaround to [this issue](flutter/flutter#112196) where navigator observers added to a `GoRouter` at the top level do not fire in `ShellRoutes`. With this change, you can provide `NavigatorObservers` directly to the `ShellRoute`

@flodaniel

* [go_router] [shell_route] Add observers parameter * [go_router] [shell_route] Add observers parameter test * Added Licence for shell_route_observers_test.dart * [go_router] [shell_route] Added type annotation to shell_route_observers_test.dart * [go_router] [shell_route] Use `HeroControllerScope` for nested Navigator * Use the correct HeroController based on the App type. * Cache the HeroController for the nested Navigator. * Clean up previous cache to prevent memory leak. * Added better cache-clearing policy for the HeroController cache. * Fixed Typos Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com> * Fixed Typos Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com> * [go_router] [shell_route] Added a better Hero test Credits to @flodaniel! --------- Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>

…ng a `ShellRoute` with `observers` (flutter#5563) [This pr](flutter#2664) added the ability to provide a list of `NavigatorObservers` to a `ShellRoute`. The equivalent was never added to `go_router_builder`, so this pr provides that functionality. This is a workaround to [this issue](flutter/flutter#112196) where navigator observers added to a `GoRouter` at the top level do not fire in `ShellRoutes`. With this change, you can provide `NavigatorObservers` directly to the `ShellRoute`

flutter-dashboardBot added the needs tests label Sep 30, 2022

github-actionsBot added the p: go_router label Sep 30, 2022

angjelkom mentioned this pull request Sep 30, 2022
[go_router] Hero animation don't work between pages nested on ShellRouteflutter/flutter#112095
Closed

grimatoma reviewed Oct 6, 2022
View reviewed changes

chunhtai self-requested a review October 13, 2022 21:46

chunhtai reviewed Oct 14, 2022
View reviewed changes

Comment threadpackages/go_router/lib/src/builder.dart Outdated

chunhtai mentioned this pull request Oct 19, 2022
[go_router] Nested stateful navigation with ShellRoute #2650
Closed
11 tasks

chunhtai self-requested a review October 20, 2022 21:48

davidmigloz mentioned this pull request Nov 5, 2022
[go_router] ShellRoutes seem to cause NavigatorObserver to not fire (5.0.1) flutter/flutter#112196
Closed

angjelkom force-pushed the shell-route-observers branch from efd5ca8 to 4011341Compare November 14, 2022 20:01

angjelkom force-pushed the shell-route-observers branch 4 times, most recently from 4b28692 to f11123dCompare December 8, 2022 19:31

ycherniavskyi reviewed Dec 8, 2022
View reviewed changes

Comment threadpackages/go_router/lib/src/route.dart Outdated
Comment threadpackages/go_router/lib/src/builder.dart Outdated

angjelkom force-pushed the shell-route-observers branch from f11123d to 915f6b2Compare December 9, 2022 08:01

Jackand others added 11 commits February 10, 2023 09:44

[go_router] [shell_route] Add observers parameter
7ae50d0

[go_router] [shell_route] Add observers parameter test
9dea8d0

Added Licence for shell_route_observers_test.dart
47855f7

[go_router] [shell_route] Added type annotation to shell_route_observ…
f5eb3c3
…ers_test.dart

[go_router] [shell_route] Use HeroControllerScope for nested Navigator
d286e84

Use the correct HeroController based on the App type.
9ab2c65

Cache the HeroController for the nested Navigator.
2e94a5a

Clean up previous cache to prevent memory leak.
c3d4348

Added better cache-clearing policy for the HeroController cache.
46a1b19

Fixed Typos
40bb607
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>

Fixed Typos
e72e2a7
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>

angjelkom force-pushed the shell-route-observers branch from 516bfef to 544fd68Compare February 10, 2023 08:46

[go_router] [shell_route] Added a better Hero test
c34d480
Credits to @flodaniel!

angjelkom force-pushed the shell-route-observers branch from 544fd68 to c34d480Compare February 10, 2023 08:54

chunhtai requested a review from hannah-hyj February 13, 2023 16:38

hannah-hyj approved these changes Feb 13, 2023
View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 14, 2023

auto-submitBot merged commit 278b489 into flutter:mainFeb 14, 2023

JasCodes mentioned this pull request Apr 18, 2023
[ go_router_builder ] The element type 'String?' can't be assigned to the map value type 'String'. flutter/flutter#125010
Closed
2 tasks

ClaireDavis mentioned this pull request Dec 4, 2023
[go_router_builder] add support to go_router_builder for initializing a ShellRoute with observers#5563
Merged
11 tasks

Uh oh!

Conversation

angjelkom commented Sep 30, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboardBot commented Sep 30, 2022

Uh oh!

grimatoma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai commented Nov 11, 2022

Uh oh!

angjelkom commented Nov 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angjelkom commented Nov 13, 2022

Uh oh!

chunhtai commented Nov 30, 2022

Uh oh!

angjelkom commented Dec 3, 2022

Uh oh!

angjelkom commented Dec 6, 2022

Uh oh!

chunhtai commented Dec 6, 2022

Uh oh!

ycherniavskyi commented Dec 8, 2022

Uh oh!

angjelkom commented Dec 8, 2022

Uh oh!

angjelkom commented Dec 8, 2022

Uh oh!

Uh oh!

Uh oh!

arsen commented Dec 13, 2022

Uh oh!

angjelkom commented Dec 14, 2022

Uh oh!

angjelkom commented Dec 15, 2022

Uh oh!

chunhtai commented Feb 9, 2023

Uh oh!

angjelkom commented Feb 10, 2023

Uh oh!

hannah-hyj left a comment

Choose a reason for hiding this comment

Uh oh!

lublot commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

angjelkom commented Sep 30, 2022•
edited
Loading

angjelkom commented Nov 11, 2022•
edited
Loading