-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Insert content text input client breaking change #7692
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
Insert content text input client breaking change #7692
Conversation
justinmc
left a comment
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 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.
atsansone
left a comment
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.
@tneotia : A little bit of language tweaking. LGTM after those changes.
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
|
Hi @atsansone could you take another look? |
|
And @tneotia can you update the branch with main? |
|
@justinmc done |
|
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?) |
sfshaza2
left a comment
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 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.
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
|
P.S. This PR would not hold up the PR on the flutter engine repo. |
|
Also, when this PR is ready to land, remove it from the Draft state. |
|
@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. |
|
Closing for now. |
|
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 :) |
|
Sounds good!
…On Mon, Jan 9, 2023 at 2:36 PM Tanay Neotia ***@***.***> wrote:
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 :)
—
Reply to this email directly, view it on GitHub
<#7692 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKS4PKMABCOX7S2EWMJWACLWRSHG5ANCNFSM6AAAAAAROHBUHY>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
|
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? |
…-input-client # Conflicts: # src/release/breaking-changes/index.md
|
@atsansone should be good now :) |
|
@sfshaza2 and @atsansone can you verify that your requested changes have been appropriately landed, thanks! |
atsansone
left a comment
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.
Needs a few tweaks, then I can approve.
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
…-input-client # Conflicts: # src/release/breaking-changes/index.md
|
@atsansone should be good to go now |
parlough
left a comment
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 documenting this.
Two small changes now that this has landed:
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
src/release/breaking-changes/insert-content-text-input-client.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Parker Lougheed <parlough@gmail.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
parlough
left a comment
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 making those adjustments!
Changes addressed, reviewer OOO
|
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? |
|
Yep. I checked and pushed fixes for it. |
Description of what this PR is changing or adding, and why:
This adds a migration guide for the new member of
TextInputClientadded by flutter/flutter#110052Issues fixed by this PR (if any): Fixes #ISSUE-NUMBER
Presubmit checklist