fix: Check active entries on parent navigator for hasActiveRouteBelow
#146428
Conversation
github-actions
bot
added
framework
VictorOhashi
force-pushed
the
fix/nested-nav-hasActiveRouteBelow
branch
from
11abed4
to
9e722c6
Compare
github-actions
bot
added
the
f: material design
|
@chunhtai Can you have a look? |
|
(Triage) Hey @VictorOhashi it looks like there are some failing checks here, can you take a look? |
VictorOhashi
force-pushed
the
fix/nested-nav-hasActiveRouteBelow
branch
from
89c6b9c
to
1e86a53
Compare
VictorOhashi
force-pushed
the
fix/nested-nav-hasActiveRouteBelow
branch
from
7d505ef
to
f13fd5b
Compare
Fixed |
| @@ -3790,6 +3816,11 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res | |||
| @override | |||
| void didChangeDependencies() { | |||
| super.didChangeDependencies(); | |||
| final NavigatorState? parentNavigator = context.findAncestorStateOfType<NavigatorState>(); | |||
There was a problem hiding this comment.
This won't trigger rebuild when it changes
There was a problem hiding this comment.
How to proper get the NavigatorState?
Can be something like this?
NavigatorState? get _parentNavigator => context.findAncestorStateOfType<NavigatorState>();or there are some InheritedWidget that expose the NavigatorState?
There was a problem hiding this comment.
There is currently no way to listen for navigator changes as this pr is the first time that we expose property that requires navigator look up.
| /// Whether there is at least one active route underneath this route. | ||
| @protected | ||
| bool get hasActiveRouteBelow { | ||
| if (_navigator == null) { | ||
| return false; | ||
| } | ||
|
|
||
| final bool hasActiveParentRouteBelow = _hasActiveParentRouteBelow(_navigator!._parentNavigator); |
There was a problem hiding this comment.
in nested case, it seem weird that you tap the inner appbar but then the outer navigator gets pop. I think this behavior may have different expectation based on use case.
There was a problem hiding this comment.
If we have one structure like this:
- MaterialApp
- Navigator (MainShellRoute)
- MainScreen
- Navigator (AnotherShellRoute)
- AnotherScreen
- Navigator (MainShellRoute)
When AnotherScreen is popped, since his parent Navigator (AnotherShellRoute) don't have any child page it should be popped as well, no?
And what would you recommend to do in cases where we have multiple Navigators?
Today my work around is force adding a BackButton to every page that can't include it auto, which for me is not desirable.
There was a problem hiding this comment.
As a user I would expect the back button should only pop content in the area that it surrounds. If AnotherShellRoute only taken part of the screen and there isn't a route in AnotherShellRoute to be popped, there shouldn't be a back button in AnotherShellRoute.
It would be a different story if MainScreen is just an empty container and AnotherShellRoute take up the whole screen though, but in that case why would you need the MainShellRoute in the first place?
There was a problem hiding this comment.
This is also why I am hesitating on this approach. I worry people may have different expectation based on their use cases
There was a problem hiding this comment.
My use case AnotherShellRoute is used to add the NavigationRail, something like this:
other possible solution would be changing on go_router package, abstracting this rule on "MaterialPage/CupertinoPage" and Navigator just for the ShellRoute.
There was a problem hiding this comment.
I as a user felt a bit weird about if there is a back button on page1 and press it pop the entire page includes the sidebar, but I do understand people may have special UX requirement. In this case, I think you should have a special backbutton implement in your code base to handle this use case.
This is just my opinion though, so I may be wrong. but as of now, I am not confident moving forward with this pr since we are just 2 data points. We can revisit this if we have more people asking to change or preserve this behavior.
WDYT?
This should fix cases when we have nested navigators.
Fix: #144687
Pre-launch Checklist
///).