Fixes #1839 #1840
Fixes #1839 #1840
Conversation
kane-knowby
changed the title
|
Hey @kane-knowby Thank you for your hard work, I just added some notes, please share your thoughts. |
Hey thanks for taking a look! Unfortunately I can't see any notes/comments, were they on the PR or somewhere else? |
|
Hey @kane-knowby my bad, forgot to commit them |
| @override | ||
| @optionalTypeArgs | ||
| Future<bool> pop<T extends Object?>([T? result]) async { | ||
| final NavigatorState? navigator = _navigatorKey.currentState; | ||
| if (navigator == null) return SynchronousFuture<bool>(false); | ||
| if (await navigator.maybePop<T>(result)) { | ||
| // On the web, popping will not update the navigation history |
There was a problem hiding this comment.
This might introduce hard-to-trace breaking changes for some users, I suggest we flag it, users who require this behavior will pass some flag (e.g useBackForPopOnWeb, 'maybe shorter') to the root router, or maybe override one.
| /// Get the top [Route<dynamic>] of Navigator | ||
| /// Used to assess the current route and popDisposition | ||
| Route<dynamic> get _navigatorTopRoute { | ||
| late Route<dynamic> route; |
There was a problem hiding this comment.
can't we rely on pagelessRoutesObserver.current instead of using popUntil?
| @@ -20,6 +20,12 @@ class NavigationHistoryImpl extends NavigationHistory { | |||
| void onNewUrlState(UrlState newState, {bool notify = true}) { | |||
| super.onNewUrlState(newState, notify: notify); | |||
| if (_currentUrl == newState.url) return; | |||
| // If the previous url is the same as the new url | |||
| // then we can infer that the screen was popped and remove the last entry | |||
There was a problem hiding this comment.
maybe we should explicitly remove the last entry inside onPopPage ( just added the method to StackRouter) instead of assuming the entry is a result of the pop event, can't think of a particular reason why the current setup might not work in some cases but, I have my doubts, in the browser history you can have the same url added multiple times with different states, we might have a similar implementation in the future.
Proposed fix for #1839