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

[DO NOT MERGE] Adds page apis migration guide #9873

Closed
wants to merge 1 commit into from

Conversation

chunhtai
Copy link
Contributor

waiting for flutter/flutter#137792

Presubmit checklist

@flutter-website-bot
Copy link
Collaborator

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

https://flutter-docs-prod--pr9873-page-apis-8eoyjmvl.web.app

@atsansone atsansone added the act.await-dev-pr Needs dev PR to merge before merging docs label Nov 29, 2023
Copy link
Contributor

@atsansone atsansone left a comment

Choose a reason for hiding this comment

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

@chunhtai : I'll approve so you don't need to go through another round, but would recommend accepting these suggestions. Thanks!

## Context

The `onPopPage` was added for cleaning up pages after a page is about to be popped.
The developer can veto pop by returning false in the callback. This does not work well
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Condition, then instruction per GSG.
Write in active voice per GSG.

Suggested change
The developer can veto pop by returning false in the callback. This does not work well
To veto pop, the developer can return false in the callback. This does not work well

Copy link
Contributor

Choose a reason for hiding this comment

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

A global request. Rather than say "the developer", can you replace with "you"? Or reword to avoid the pronoun entirely?

Copy link

Choose a reason for hiding this comment

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

Simple and straight

Suggested change
The developer can veto pop by returning false in the callback. This does not work well
To veto pop, return `false` in the callback. This does not work well


The `onPopPage` was added for cleaning up pages after a page is about to be popped.
The developer can veto pop by returning false in the callback. This does not work well
with other popping mechanisms in the framework, such as [`PopScope`] and cupertino back
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is the cupertino mention an object or something else? Might be good to link to something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change this even further:

Use the onPopPage method to clean up after a page is about to be popped.
You can veto the pop by returning false in the callback. This method doesn't work well
with other popping mechanisms in the framework,
such as [PopScope][] or CupertinoNavigationBarBackButton.

with other popping mechanisms in the framework, such as [`PopScope`] and cupertino back
gestures.

The page APIs needed to be refactored in order to integrate them together.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggstion: Condition, then instruction per GSG.

Suggested change
The page APIs needed to be refactored in order to integrate them together.
To integrate the other pop mechanisms together, the page APIs need to be refactored.

Comment on lines +24 to +25
The `onPopPage` is replaced by `onDidRemovePage` and no longer had the ability to veto
a pop. In the `onDidRemovePage`, one is only responsible for updating the [`pages`].
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Write in active voice per GSG. Google Style doesn't recommend using a property as a noun on its own. It should modify what it's changing.

Suggested change
The `onPopPage` is replaced by `onDidRemovePage` and no longer had the ability to veto
a pop. In the `onDidRemovePage`, one is only responsible for updating the [`pages`].
The `onDidRemovePage` property replaces the `onPopPage` property.
It can no longer veto a pop.
In the `onDidRemovePage`, one is only responsible for updating the [`pages`].

Comment on lines +27 to +28
The veto mechanism is moved to the `Page.canPop` and `Page.onPopInvoked` similar to how
one uses the `PopScope` widget.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Write in active voice per GSG.

Suggested change
The veto mechanism is moved to the `Page.canPop` and `Page.onPopInvoked` similar to how
one uses the `PopScope` widget.
The veto mechanism moves to the `Page.canPop` and `Page.onPopInvoked`.
These function similar to how one uses the `PopScope` widget.

@atsansone atsansone added the st.RFM.% Ready to merge or land with minor changes. No further review needed. label Nov 29, 2023
@sfshaza2
Copy link
Contributor

I'd like to make further changes to this, @chunhtai. Perhaps you can make some changes, so far, and I can review it again? Definitely don't land.

@atsansone atsansone added review.await-update Awaiting Updates after Edits and removed st.RFM.% Ready to merge or land with minor changes. No further review needed. labels Nov 30, 2023
@sfshaza2
Copy link
Contributor

sfshaza2 commented Dec 4, 2023

@chunhtai, this PR has become a bit of a mess. I'm closing it. Can you open a fresh copy?

thx!

@sfshaza2 sfshaza2 closed this Dec 4, 2023
@sfshaza2 sfshaza2 removed review.await-update Awaiting Updates after Edits act.await-dev-pr Needs dev PR to merge before merging docs labels Dec 4, 2023
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.

None yet

5 participants