-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add breaking change page for completing Navigator behavior #11684
base: main
Are you sure you want to change the base?
Conversation
d5d7524
to
0425965
Compare
0425965
to
1dd2a9a
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.
Overall, this looks great. Just a couple little nits.
@Piinks, we don't generally land breaking changes until the change goes into a release. Is this going into 3.29? I see that @chunhtai suggested in the flutter/flutter issue that the breaking change page needs to land before landing the change, but that's not how we generally do it. So, short version: can I land this now?
I believe this might be the first time EVER than a non-Googler has written a breaking change page!
src/content/release/breaking-changes/navigator-complete-route.md
Outdated
Show resolved
Hide resolved
src/content/release/breaking-changes/navigator-complete-route.md
Outdated
Show resolved
Hide resolved
src/content/release/breaking-changes/navigator-complete-route.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]>
Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]>
Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]>
Thank a lot for the review, changes requested applied :). |
/gcbrun |
Visit the preview URL for this PR (updated for commit dd22da3): https://flutter-docs-prod--pr11684-feat-navigator-complete-rou-cx62m8dr.web.app |
This has never been the case. Guides for changes that have not been released yet are always listed under the "Not yet released to stable" section. |
OK, but they have generally landed into the main channel, which is what I meant. |
Ah ok! Yes. landing on the main channel in flutter/flutter, not a release. :) |
Should i change the target branch of this PR ? |
yup sorry if my previous comment in the pr confused you. Having this pr open and ready to land and announcement out in discord should be enough to land the framework change. Once that land you will need to get the version number by doing |
For the discord part, I don't have the right to publish in the announcement channel. I've put a little message asking help inside the hackers-routing channel. |
By "land the framework change" you mean the original PR where navigation is reworded right? Original PR and this one needs to use a flutter version. There for the deprecation message and here for the timeline, I'm a bit confused to understand how I need to update that, in which order and how if one PR is merged. |
just use version before the merge |
Right.
I just sent out the announcement so no worry on this part. |
methods with an optional callback function to allow developers to handle pop | ||
logic in indirect scenarios (such as `removeUntil`). | ||
|
||
## Migration guide |
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.
Could we provide code examples before and after?
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.
Done
Description of what this PR is changing or adding, and why:
A breaking change is planned along the PR bellow.
Issues fixed by this PR (if any):
flutter/flutter#157505
PRs or commits this PR depends on (if any):
flutter/flutter#157725
Presubmit checklist