Skip to content

Last char is when the length is now zero#892

Merged
mchowning merged 1 commit intodevelopfrom
fix/dont-disable-selection-listener-on-first-char-delete
Feb 7, 2020
Merged

Last char is when the length is now zero#892
mchowning merged 1 commit intodevelopfrom
fix/dont-disable-selection-listener-on-first-char-delete

Conversation

@hypest
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants