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

Last char is when the length is now zero #892

Merged

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Feb 7, 2020

Experimental PR to fix the gutenberg-mobile issue: wordpress-mobile/gutenberg-mobile#1865

Testing the original issues to be sure this PR doesn't break them

From #398:

Test 1 (testing the format while the keyboard is closed): ✅
Test 2 (format on the next line): ❌ This test seems to fail anyway even on develop (d2baefd) so, ignoring for this PR
Test 3 (Backspace last char, backspace to clear format): ✅

From #357:

Test: ⚠️ issue not happening but, the format did not continue to the next line as the test steps mention.

From #362:

Test: ✅

To test

Using the gutenberg-mobile PR wordpress-mobile/gutenberg-mobile#1867 confirm that the issue of merging two paragraph blocks (wordpress-mobile/gutenberg-mobile#1865) is not happening anymore.

Make sure strings will be translated:

  • If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

@hypest
Copy link
Contributor Author

hypest commented Feb 7, 2020

The unit tests seem to fail on CI (all of them, which is weird) but they are green locally.

@hypest hypest marked this pull request as ready for review February 7, 2020 14:20
@cameronvoell
Copy link
Contributor

Confirmed this changes fixes wordpress-mobile/gutenberg-mobile#1865 on the basic case, however I found one way to still have merge on delete not work, when you add a style to the text that extends farther to the right from what you delete at the front of the text before the attempted merge. (the merge on delete never happens in the attached GIF, don't be fooled by GIF restart :-P )

I think the remaining issue would be pretty rare, and this fix is still an improvement, so we could document the remaining issue, and still merge this fix. What do you think @hypest ?

styled-text-no-merge-on-delete

@hypest
Copy link
Contributor Author

hypest commented Feb 7, 2020

Thanks Cameron, yeah, we could tackle the subcase in a separate PR, but since I'm not even 100% about the current fix, let's also hear from @khaykov about the fix, since there might be more context.

@hypest
Copy link
Contributor Author

hypest commented Feb 7, 2020

Since we need this fix on the v1.22 release of the block editor, let's move forward, wdyt @cameronvoell ?

Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

Approving since this change should fix the vast majority of instances of wordpress-mobile/gutenberg-mobile#1865.

I also recommend merging despite the failed CI Unit tests, since @mchowning and this comment mention that unit tests were working fine locally.

@mchowning
Copy link
Contributor

Going ahead and merging since the tests are passing locally and this is "blocking" the 1.22 block editor release. 🥁

@mchowning mchowning merged commit 5138fed into develop Feb 7, 2020
@mchowning mchowning deleted the fix/dont-disable-selection-listener-on-first-char-delete branch February 7, 2020 19:29
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

3 participants