Skip to content
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

Merged
merged 9 commits into from
Feb 11, 2025

Conversation

EArminjon
Copy link
Contributor

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

  • This PR is marked as draft with an explanation if not meant to land until a future stable release.
  • This PR doesn’t contain automatically generated corrections (Grammarly or similar).
  • This PR follows the Google Developer Documentation Style Guidelines — for example, it doesn’t use i.e. or e.g., and it avoids I and we (first person).
  • This PR uses semantic line breaks of 80 characters or fewer.

@EArminjon EArminjon requested review from sfshaza2, antfitch, parlough and a team as code owners February 6, 2025 09:33
@EArminjon EArminjon force-pushed the feat-navigator-complete-route branch 2 times, most recently from 0425965 to 1dd2a9a Compare February 6, 2025 09:46
Copy link
Contributor

@sfshaza2 sfshaza2 left a 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!

@sfshaza2 sfshaza2 changed the title Add doc Add breaking change page for completing Navigator behavior Feb 6, 2025
@EArminjon
Copy link
Contributor Author

Thank a lot for the review, changes requested applied :).

@sfshaza2
Copy link
Contributor

sfshaza2 commented Feb 6, 2025

/gcbrun

@flutter-website-bot
Copy link
Collaborator

flutter-website-bot commented Feb 6, 2025

Visit the preview URL for this PR (updated for commit 153343c):

https://flutter-docs-prod--pr11684-feat-navigator-complete-rou-cx62m8dr.web.app

@Piinks
Copy link
Contributor

Piinks commented Feb 6, 2025

we don't generally land breaking changes until the change goes into a release.

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.

@sfshaza2
Copy link
Contributor

sfshaza2 commented Feb 6, 2025

OK, but they have generally landed into the main channel, which is what I meant.

@Piinks
Copy link
Contributor

Piinks commented Feb 6, 2025

Ah ok! Yes. landing on the main channel in flutter/flutter, not a release. :)

@EArminjon
Copy link
Contributor Author

Should i change the target branch of this PR ?

@chunhtai
Copy link
Contributor

chunhtai commented Feb 6, 2025

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 git describe --tags in the latest branch to fill out the timeline section and finally land this migration guide

@EArminjon
Copy link
Contributor Author

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.

@EArminjon
Copy link
Contributor Author

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.

@chunhtai
Copy link
Contributor

chunhtai commented Feb 6, 2025

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

@chunhtai
Copy link
Contributor

chunhtai commented Feb 6, 2025

By "land the framework change" you mean the original PR where navigation is reworded right?

Right.

For the discord part, I don't have the right to publish in the announcement channel

I just sent out the announcement so no worry on this part.

Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! But, @EArminjon, can you also add this page to the index page? Under "Not yet released". Then I can land!

@EArminjon EArminjon force-pushed the feat-navigator-complete-route branch from c5003b6 to da8bb3a Compare February 11, 2025 07:57
@EArminjon
Copy link
Contributor Author

EArminjon commented Feb 11, 2025

Hello @sfshaza2 ,

Doc updated.

The timeline section is not updated, if i well understand i need to wait that the original PR is merged to update this one right ?

@sfshaza2
Copy link
Contributor

/gcbrun

Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sfshaza2
Copy link
Contributor

/gcbrun

@sfshaza2
Copy link
Contributor

/gcbrun

@sfshaza2 sfshaza2 merged commit b3770d6 into flutter:main Feb 11, 2025
9 checks passed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty a lot !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants