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
Heroes and nested Navigators #29069
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach LG
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some random questions about the comments/details
@@ -110,6 +111,14 @@ Rect _globalBoundingBoxFor(BuildContext context) { | |||
/// B to A, route A's hero's widget is, by default, placed over where route B's | |||
/// hero's widget was, and then the animation goes the other way. | |||
/// | |||
/// ### Nested Navigators | |||
/// | |||
/// If either or both routes contain nested [Navigator]s, only [Hero]s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there aren't any nested navigators we still only consider heroes from the topmost route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this explanation!
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?