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

Remove willTrimSpaces check on Android #2206

Closed
wants to merge 2 commits into from

Conversation

mchowning
Copy link
Contributor

Removes willTrimSpaces check on Android. See PR description and testing steps in WordPress/gutenberg#22006.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mchowning mchowning added this to the 1.28 milestone May 1, 2020
@mchowning mchowning requested review from cameronvoell and marecar3 May 1, 2020 00:37
@mchowning mchowning self-assigned this May 1, 2020
@mchowning
Copy link
Contributor Author

I added an e2e test to cover this scenario. I'm a bit torn on whether that's a good idea or not. On one hand these out of bounds issues have been a recurrent issue on Android, but on the other hand it seems like a bit of an edge case and if we added e2e tests for every one of these our test suite would take forever to run (admittedly we're a long ways from having that problem). Interested to hear your thoughts.

@mchowning mchowning force-pushed the remove_will_trim_check_for_android branch from 165c939 to 054ad21 Compare May 1, 2020 00:42
@mchowning
Copy link
Contributor Author

mchowning commented May 1, 2020

👋 @hypest ! Could you take a look at this PR? I believe you have the most experience with the willTrimSpaces code, and I want to make sure removing it doesn't cause any regressions. 😬

@hypest
Copy link
Contributor

hypest commented May 5, 2020

Had a look and left a comment Matt. I think that it's OK to remove the willTrimSpaces method if Aztec is not removing them, though some more testing to align make sure that indeed Aztec won't trim other white space characters is needed.

In the meantime, tried the original issue and still works OK on this PR.

@SergioEstevao
Copy link
Contributor

@mchowning do you still want this to go for 1.28?

@mchowning
Copy link
Contributor Author

do you still want this to go for 1.28?

No, better to bump it.

@mchowning mchowning modified the milestones: 1.28, 1.29 May 11, 2020
@mchowning
Copy link
Contributor Author

Switching this to be a draft for the time being because I think I'll be submitting a larger change shortly that will include this.

@mchowning mchowning marked this pull request as draft May 19, 2020 17:23
@mchowning
Copy link
Contributor Author

Closing this because these changes were already handled in #2279

@mchowning mchowning closed this May 25, 2020
@mchowning mchowning deleted the remove_will_trim_check_for_android branch May 25, 2020 18:03
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.

3 participants