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

Resolve Text Entry Errors in Alt Text Settings #3818

Merged
merged 5 commits into from
Sep 3, 2021

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented Aug 10, 2021

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 10, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@SiobhyB SiobhyB added the bugfix label Aug 10, 2021
@SiobhyB SiobhyB marked this pull request as ready for review August 10, 2021 17:59
@SiobhyB SiobhyB requested a review from fluiddot August 11, 2021 15:48
@SiobhyB SiobhyB force-pushed the fix/bottomsheet-text-control-error branch 2 times, most recently from ea620e4 to 9f4dcca Compare September 1, 2021 08:28
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I'm wondering why the App.js file shows that 2,999 lines have been removed 🤔 , could we take a look at this before merging?

Apart from that the changes have been tested and are approved via WordPress/gutenberg#33975 (review).

@SiobhyB SiobhyB closed this Sep 3, 2021
@SiobhyB SiobhyB force-pushed the fix/bottomsheet-text-control-error branch from bb5659a to 4570c0d Compare September 3, 2021 15:40
@SiobhyB SiobhyB reopened this Sep 3, 2021
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Sep 3, 2021

I'm wondering why the App.js file shows that 2,999 lines have been removed 🤔 , could we take a look at this before merging?

😱

Thank you for flagging, I believe that was a result of my not updating the bundle after I stopped updating the installable builds for this PR. There must have been some mix-up between that and merge conflicts somewhere down the line.

As changes to the bundle aren't required for this PR, I rewrote the commit history and updated it to only have changes to the Gutenberg reference.

Let me know if that looks okay to you! 🙇‍♀️

@fluiddot
Copy link
Contributor

fluiddot commented Sep 3, 2021

I'm wondering why the App.js file shows that 2,999 lines have been removed 🤔 , could we take a look at this before merging?

😱

Thank you for flagging, I believe that was a result of my not updating the bundle after I stopped updating the installable builds for this PR. There must have been some mix-up between that and merge conflicts somewhere down the line.

As changes to the bundle aren't required for this PR, I rewrote the commit history and updated it to only have changes to the Gutenberg reference.

Let me know if that looks okay to you! 🙇‍♀️

Good idea, since the bundle is not required better to revert those changes 👍 .

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Approved via WordPress/gutenberg#33975 (review).

NOTE: The Gutenberg ref should point to the merge commit in Gutenberg's trunk branch before merging this PR.

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

Successfully merging this pull request may close these issues.

2 participants