-
Couldn't load subscription status.
- Fork 3.5k
[go_router] Allow any number of the same page on the stack #2339
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
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
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 started to think support the push in the declarative way may be the wrong decision. as soon as a page is pushed to the stack, the stack will not be in sync with the url any more.
In this particular use case where you want to push the same page, does it make more sense to be a pageless route? that is, pushing use the Navigator.of(context). The difference is the pageless route will belong to the top most page route and will be move/remove with that page route
|
On GitHub, after opening the top of the repository, you can open the list of pull requests and open the pull request details. If we don't want the delegate to have a Map, why not change the process so that the |
3cbf18c to
12388eb
Compare
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 nitpicks
| null, | ||
| ); | ||
|
|
||
| goRouter.push('/a'); |
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.
can you do an await tester.pumpAndSettle() in between every push to verify it does not crash?
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 the review, I fixed it in 2ee5fd6.
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
|
not sure why the check failed, can you rebase and try again? |
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>
|
This change must not be landed until the underlying problem with 4.2.1 is fixed, or it will re-break everyone that was broken by 4.2.1 (since it'll publish a new version with the same bug). @johnpryan @chunhtai please don't approve any more go_router PRs until the fix for the breakage is fixed, to avoid any accidents. |
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.
Adding a request changes until the above has been resolved to ensure this doesn't accidentally land.
Restore the function to push the same page multiple times, which was supported in v3.1.1.
Fixes flutter/flutter#107045
Refs
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.