Skip to content

Conversation

@tneotia
Copy link
Contributor

@tneotia tneotia commented Oct 25, 2022

Description of what this PR is changing or adding, and why:

This adds a migration guide for the new member of TextInputClient added by flutter/flutter#110052

Issues fixed by this PR (if any): Fixes #ISSUE-NUMBER

Presubmit checklist

@atsansone atsansone assigned johnpryan and unassigned johnpryan Oct 25, 2022
@atsansone atsansone requested a review from justinmc October 25, 2022 18:51
@atsansone atsansone added the review.tech Awaiting Technical Review label Oct 25, 2022
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM from a technical standpoint. I added some suggestions of details that could be added, but I think it's up to you.

Also just stating that this PR should wait until the framework PR is merged and then be updated with the version it landed in.

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.

@tneotia : A little bit of language tweaking. LGTM after those changes.

@tneotia tneotia requested a review from atsansone November 16, 2022 16:35
@LongCatIsLooong
Copy link
Contributor

Hi @atsansone could you take another look?

@justinmc
Copy link
Contributor

justinmc commented Dec 5, 2022

And @tneotia can you update the branch with main?

@tneotia
Copy link
Contributor Author

tneotia commented Dec 5, 2022

@justinmc done

@tneotia
Copy link
Contributor Author

tneotia commented Dec 14, 2022

Hi, any movement with this? Do I need to mark this as ready to review? (I was under the assumption that the PR in the SDK repo needs to be merged first, but maybe this is the holdup instead?)

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.

Thanks so much for this breaking change, @tneotia! I had a few minor nits and I can't land this without actual links and the "landed in" info. For the. missing API link, perhaps you could use a link to the master docs repo? The template explains this, but the link looks something like: {{site.master-api}}/flutter/[link_to_relevant_page].html.

@sfshaza2
Copy link
Contributor

P.S. This PR would not hold up the PR on the flutter engine repo.

@sfshaza2
Copy link
Contributor

Also, when this PR is ready to land, remove it from the Draft state.

@sfshaza2
Copy link
Contributor

sfshaza2 commented Jan 9, 2023

@tneotia, can we move this out of draft state and land? We are freezing the website by end of day today. Otherwise we will just close it and it can be reopened later.

@sfshaza2
Copy link
Contributor

sfshaza2 commented Jan 9, 2023

Closing for now.

@sfshaza2 sfshaza2 closed this Jan 9, 2023
@tneotia
Copy link
Contributor Author

tneotia commented Jan 9, 2023

The PR on Flutter SDK has not been merged yet so your review criteria wouldn't be satisfied. Hoping that it is revisited soon, then I'll reopen this :)

@sfshaza2
Copy link
Contributor

sfshaza2 commented Jan 9, 2023 via email

@justinmc
Copy link
Contributor

justinmc commented Feb 2, 2023

Reopening because the PR has landed and has been tagged with v3.8.0-1.0.pre (merge commit). @tneotia Could you add that version into this PR where you put TBDs?

@justinmc justinmc reopened this Feb 2, 2023
@tneotia tneotia marked this pull request as ready for review February 2, 2023 20:53
@tneotia
Copy link
Contributor Author

tneotia commented Mar 17, 2023

@atsansone should be good now :)

@domesticmouse
Copy link
Contributor

@sfshaza2 and @atsansone can you verify that your requested changes have been appropriately landed, thanks!

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.

Needs a few tweaks, then I can approve.

@atsansone atsansone added review.await-update Awaiting Updates after Edits and removed review.copy Awaiting Copy Review labels Apr 3, 2023
@atsansone atsansone assigned tneotia and unassigned justinmc Apr 3, 2023
@tneotia
Copy link
Contributor Author

tneotia commented Apr 3, 2023

@atsansone should be good to go now

@tneotia tneotia requested a review from atsansone April 3, 2023 21:34
@sfshaza2 sfshaza2 added st.RFM Ready to merge or land act.merge-to-next Merge PR to next stable branch release and removed review.await-update Awaiting Updates after Edits labels Apr 26, 2023
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for documenting this.

Two small changes now that this has landed:

@parlough parlough added st.RFM.% Ready to merge or land with minor changes. No further review needed. and removed st.RFM Ready to merge or land act.merge-to-next Merge PR to next stable branch release labels May 12, 2023
tneotia and others added 3 commits May 12, 2023 17:52
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for making those adjustments!

@parlough parlough dismissed stale reviews from sfshaza2 and atsansone May 14, 2023 17:26

Changes addressed, reviewer OOO

@parlough parlough merged commit de22b96 into flutter:main May 14, 2023
@Hixie
Copy link
Contributor

Hixie commented May 16, 2023

I had a conflict with this in my PR and I'm not sure I merged it correctly, can someone check https://github.com/flutter/website/pull/8683/files is correct?

@parlough
Copy link
Member

parlough commented May 16, 2023

Yep. I checked and pushed fixes for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

st.RFM.% Ready to merge or land with minor changes. No further review needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants