Activate preformat block on android platform by marecar3 · Pull Request #1615 · wordpress-mobile/gutenberg-mobile

marecar3 · 2019-11-25T11:12:19Z

Fixes Active preformat block on the Android platform

AztecEditor-Android PR: wordpress-mobile/AztecEditor-Android#869
Gutenberg PR: WordPress/gutenberg#18777
WPAndroid PR: wordpress-mobile/WordPress-Android#10868

To test:

Open the demo app
Add a Preformatted block to the content
Write around on it
Try to merge it with other blocks

Special case:

Create a draft Post on Gutenberg Web
Insert bellow content

<!-- wp:preformatted -->
<pre class="wp-block-preformatted">just a text<br>line2<br></pre>
<!-- /wp:preformatted -->

Open mobile application
Open drafted Post
Place the caret on the end of the Post
Go back (leave Editor)
Open draft Post again

Result: There shouldn't be a red screen: wordpress-mobile/AztecEditor-Android#869 (comment)

Update release notes:

If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

SergioEstevao

I tested this on a Android device and all is looking good!

mchowning

👋 @marecar3 ! This is looking pretty good. There are just a two issues I'm seeing with newline handling on Android.

Also, I think we need to update the WPAndroid branch to reference the latest gutenberg and Aztec code.

1. Adding newline when last character on a line is a space causes problems with consecutive empty lines

Steps:

Add preformat block
Enter text on line that ends with a space
Press Enter and observe a new line as expected
Press Enter again
Observe the cursor briefly jump, that a new line is added to the block, but the cursor remains at its current position.
Press Enter again
Observe that the previously added new line is removed and the cursor remains unmoved

2. Merging blocks with newlines loses newlines

Steps:

Add preformat block
Add content spanning multiple lines
Create another preformat block
(Optional) Add content to the new block
Merge the new block back into the first block by pressing backspace at the beginning of the second block
Observe that all newlines in the two blocks appear to have been replaced with spaces

SergioEstevao · 2019-11-26T22:47:50Z

@mchowning bug number 2 is a know bug, and it was happening in iOS and the web too, it's now fixed in GB master by this PR

marecar3 · 2019-11-28T13:10:46Z

Changed target branch to develop as we don't want to hurry with this feature (it still has some active issues)

hypest · 2019-11-28T13:22:52Z

Observe the cursor briefly jump, that a new line is added to the block, but the cursor remains at its current position.

This is probably caused by https://github.com/WordPress/gutenberg/blob/bb0529f59746e67dae47077bf91833e42a9081c5/packages/rich-text/src/component/index.native.js#L650. Aztec is trimming spaces in other cases but in the <pre> case it will not so, the JS code need to cater for that case too. @marecar3 can you have a look there?

Press Enter again

Observe that the previously added new line is removed and the cursor remains unmoved

I wasn't able to replicate that 🤔

marecar3 · 2019-11-28T13:30:29Z

Observe the cursor briefly jump, that a new line is added to the block, but the cursor remains at its current position.

This is probably caused by https://github.com/WordPress/gutenberg/blob/bb0529f59746e67dae47077bf91833e42a9081c5/packages/rich-text/src/component/index.native.js#L650. Aztec is trimming spaces in other cases but in the <pre> case it will not so, the JS code need to cater for that case too. @marecar3 can you have a look there?

Thanks a lot, @hypest, you opened my eyes, will let you know when I fix it.

Press Enter again

Observe that the previously added new line is removed and the cursor remains unmoved

I wasn't able to replicate that 🤔

Hey @hypest, I have fixed that issue.

hypest · 2019-11-28T13:32:34Z

it's now fixed in GB master by this PR

Already commented in the Gutenberg PR so, when that PR gets rebased the fix will be incorporated.

Update gb reference

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java

hypest

Did a first pass reviewing Marko and left a couple of semi-minor comments. I want to do more testing of the writing flow before I LGTM this.

One of the issues I noticed, which also happens on the web is that if you have a PRE block with multiple empty lines in its end, merging another PRE seems to "eat up" the last empty line.

Steps to repro:

Add a PRE block with some text and 2 empty lines in its end
Add another PRE block with some text
Merge the second block by tapping backspace in its start
Notice the merged block only having 1 empty line between the merged text. I'd expect to have 2.

As I said, this happens on the web too so, no need to address it here.

Rename method name

Update aztec version

marecar3 · 2019-12-05T18:19:14Z

Hey @hypest, I think that I have addressed all issues so this one is ready for another iteration of review.

react-native-aztec/android/build.gradle

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java

# Conflicts: # gutenberg

hypest

Made another pass Marko and only one comment needs attention, the one about the annotation.

marecar3 · 2019-12-06T18:41:01Z

Made another pass Marko and only one comment needs attention, the one about the annotation.

👋 Thanks @hypest, I have fixed the comment which points to fix annotation.
You can do another iteration of testing.

hypest · 2019-12-06T19:25:27Z

Hmm, the Android device tests consistently fail on the rotate device and continue adding blocks test. I've tried re-triggering them 3 times already 🤔

Woohoo, the 4th time was the lucky one!

mchowning

Tested and is working well. 🎉 Left one small comment, but it's not a necessary change.

mchowning · 2019-12-06T20:10:59Z

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java

+    }
+
+    private boolean isPreTag() {
+        return !TextUtils.isEmpty(mTagName) && mTagName.equals(PRE_TAG);


This is just a nice-to-have, but I think this could be shortened to be a bit more clear by just reversing the equals check so you get a null and empty check for free since PRE_TAG is a constant:

return PRE_TAG.equals(mTagName);

Good point. Fixed.

marecar3 mentioned this pull request Nov 25, 2019

[RNMobile] - Activate preformat on android WordPress/gutenberg#18720

Closed

6 tasks

marecar3 changed the title ~~Issue/1264 activate pre format block android~~ Issue/1264 Activate pre format block on android platform Nov 25, 2019

marecar3 self-assigned this Nov 25, 2019

marecar3 requested review from SergioEstevao, etoledom, hypest and mchowning November 25, 2019 14:35

marecar3 changed the title ~~Issue/1264 Activate pre format block on android platform~~ Activate pre format block on android platform Nov 25, 2019

marecar3 added this to the 1.18 milestone Nov 25, 2019

marecar3 added the [OS] Android label Nov 25, 2019

marecar3 changed the title ~~Activate pre format block on android platform~~ Activate preformat block on android platform Nov 25, 2019

marecar3 mentioned this pull request Nov 25, 2019

Gutenberg/1442 activate preformatted block wordpress-mobile/WordPress-Android#10846

Closed

2 tasks

marecar3 marked this pull request as ready for review November 25, 2019 15:44

SergioEstevao approved these changes Nov 26, 2019

View reviewed changes

mchowning suggested changes Nov 26, 2019

View reviewed changes

marecar3 mentioned this pull request Nov 27, 2019

[RNMobile] Activate preformat on android WordPress/gutenberg#18777

Merged

6 tasks

marecar3 changed the base branch from develop to release/1.18.0 November 27, 2019 12:10

marecar3 mentioned this pull request Nov 28, 2019

Gutenberg/activate preformat block wordpress-mobile/WordPress-Android#10868

Merged

2 tasks

marecar3 requested review from cameronvoell and maxme November 28, 2019 13:05

marecar3 changed the base branch from release/1.18.0 to develop November 28, 2019 13:06

marecar3 added 4 commits November 28, 2019 15:20

Added missing preformat block implementation

ff2805c

update gb reference

25bcd4d

Make sure to call EndOfBufferMarkerAdder watcher when tag is PRE

f4097fd

Update gb reference

Update release notes

a03ee8b

hypest reviewed Dec 5, 2019

View reviewed changes

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java Outdated Show resolved Hide resolved

hypest reviewed Dec 5, 2019

View reviewed changes

marecar3 added 4 commits December 5, 2019 18:20

Removed unused import

1796926

Rename method name

Split condition in a smaller parts to make it easier for reading

a665093

Update aztec version

Update gb reference

a3ac7d1

Merge branch 'develop' into issue/1264_activate_pre_format_block_android

52bb6f0

marecar3 added 2 commits December 6, 2019 13:40

Merge branch 'develop' into issue/1264_activate_pre_format_block_android

dd76dc6

Update gb reference

f4e610a

hypest reviewed Dec 6, 2019

View reviewed changes

react-native-aztec/android/build.gradle Outdated Show resolved Hide resolved

hypest reviewed Dec 6, 2019

View reviewed changes

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java Outdated Show resolved Hide resolved

Merge branch 'develop' into issue/1264_activate_pre_format_block_android

d23a3cb

# Conflicts: # gutenberg

hypest reviewed Dec 6, 2019

View reviewed changes

marecar3 added 2 commits December 6, 2019 19:32

Add AndroidX NonNull annotation

a3e6700

Update gb ref

7e1db03

mchowning approved these changes Dec 6, 2019

View reviewed changes

marecar3 mentioned this pull request Dec 7, 2019

Allow preformatted background alpha and tidying to be set from child classes wordpress-mobile/AztecEditor-Android#869

Merged

1 task

marecar3 added 2 commits December 7, 2019 07:53

Optimize isPreTag condition

c964d00

Update aztec version to v1.3.36

4b5fed3

rachelmcr mentioned this pull request Dec 9, 2019

Testing: Some UI tests are flaky #1662

Closed

marecar3 added 4 commits December 9, 2019 16:28

Update gb ref to master

7ac2038

Merge branch 'develop' into issue/1264_activate_pre_format_block_android

69468f2

Update gb ref

ed582a6

Update gb ref

ba942f7

marecar3 merged commit 035fc31 into develop Dec 9, 2019

marecar3 deleted the issue/1264_activate_pre_format_block_android branch December 9, 2019 17:55

This was referenced Dec 10, 2019

Release v1.19.0 #1667

Merged

Gutenberg: update to v1.19.0 wordpress-mobile/WordPress-Android#10926

Merged

Conversation

marecar3 commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioEstevao left a comment

Choose a reason for hiding this comment

Uh oh!

mchowning left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1. Adding newline when last character on a line is a space causes problems with consecutive empty lines

2. Merging blocks with newlines loses newlines

Uh oh!

SergioEstevao commented Nov 26, 2019

Uh oh!

marecar3 commented Nov 28, 2019

Uh oh!

hypest commented Nov 28, 2019

Uh oh!

marecar3 commented Nov 28, 2019

Uh oh!

hypest commented Nov 28, 2019

Uh oh!

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

marecar3 commented Dec 5, 2019

Uh oh!

Uh oh!

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

marecar3 commented Dec 6, 2019

Uh oh!

hypest commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mchowning left a comment

Choose a reason for hiding this comment

Uh oh!

mchowning Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

marecar3 Dec 7, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marecar3 commented Nov 25, 2019 •

edited

Loading

mchowning left a comment •

edited

Loading

hypest commented Dec 6, 2019 •

edited

Loading