Heroes and nested Navigators#29069
Conversation
|
@spkersten What do you think about this approach? Can you check that this change works for your use cases? |
goderbauer
added
framework
|
@goderbauer From reading the code and description this looks like it will fit our use case. I haven't had time to actually test it though. |
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
LGTM, just some random questions about the comments/details
| /// | ||
| /// ### Nested Navigators | ||
| /// | ||
| /// If either or both routes contain nested [Navigator]s, only [Hero]s |
There was a problem hiding this comment.
If there aren't any nested navigators we still only consider heroes from the topmost route?
There was a problem hiding this comment.
The important bit is that in the nested navigator case, its the topmost route of the nested navigator (and not the topmost route of the navigator controlling the hero transition)
There was a problem hiding this comment.
I rephrased this a bit and emphasized nested navigators.
| if (Navigator.of(hero) == navigator) { | ||
| addHero(hero, tag); | ||
| } else { | ||
| // The nearest navigator to the Hero is not the Navigator that is |
There was a problem hiding this comment.
Thanks for this explanation!
5 participants
Description
Before this change nested Navigators were causing trouble for Heroes: If the inner Navigator had two PageRoutes on its stack with a Hero tagged "superman" in each of them (because that Hero was supposed to animate between the two PageRoutes) and you were pushing a PageRoute on the outer navigator it would fail because the from route (the one with the nested Navigator) had two Heroes with the same tag. That's legal in the context of the inner Navigator, but was causing trouble for the outer Navigator (it would assert).
This PR changes which Heroes are considered for animation for nested Navigators only: If the source or destination route for a potential Hero transition contain nested Navigators, only the Heroes in the top-most Route (which has to be a PageRoute) of that nested Navigator are considered for animation.
This is preserving the (existing) behavior of animating a Hero from a PageRoute in the outer Navigator to a PageRoute in the inner navigator (I'll add a test for this).
It's an alternative to the approach suggested in #28420.
Related Issues
Fixes #28042
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?