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

Activate preformat block on android platform #1615

Merged
merged 32 commits into from
Dec 9, 2019

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Nov 25, 2019

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:

  1. Create a draft Post on Gutenberg Web
  2. Insert bellow content
<!-- wp:preformatted -->
<pre class="wp-block-preformatted">just a text<br>line2<br></pre>
<!-- /wp:preformatted -->
  1. Open mobile application
  2. Open drafted Post
  3. Place the caret on the end of the Post
  4. Go back (leave Editor)
  5. 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.

@marecar3 marecar3 changed the title Issue/1264 activate pre format block android Issue/1264 Activate pre format block on android platform Nov 25, 2019
@marecar3 marecar3 self-assigned this Nov 25, 2019
@marecar3 marecar3 changed the title Issue/1264 Activate pre format block on android platform Activate pre format block on android platform Nov 25, 2019
@marecar3 marecar3 added this to the 1.18 milestone Nov 25, 2019
@marecar3 marecar3 changed the title Activate pre format block on android platform Activate preformat block on android platform Nov 25, 2019
@marecar3 marecar3 marked this pull request as ready for review November 25, 2019 15:44
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

👋 @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:

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

preformat-blocks-space-newline mp4

2. Merging blocks with newlines loses newlines

Steps:

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

preformat-lose-newlines-on-merge mp4

@SergioEstevao
Copy link
Contributor

@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 marecar3 changed the base branch from develop to release/1.18.0 November 27, 2019 12:10
@marecar3 marecar3 changed the base branch from release/1.18.0 to develop November 28, 2019 13:06
@marecar3
Copy link
Contributor Author

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

@hypest
Copy link
Contributor

hypest commented Nov 28, 2019

  1. 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?

  1. Press Enter again
  2. Observe that the previously added new line is removed and the cursor remains unmoved

I wasn't able to replicate that 🤔

@marecar3
Copy link
Contributor Author

  1. 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.

  1. Press Enter again
  2. 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
Copy link
Contributor

hypest commented Nov 28, 2019

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.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

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:

  1. Add a PRE block with some text and 2 empty lines in its end
  2. Add another PRE block with some text
  3. Merge the second block by tapping backspace in its start
  4. 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.

@marecar3
Copy link
Contributor Author

marecar3 commented Dec 5, 2019

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

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

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

@marecar3
Copy link
Contributor Author

marecar3 commented Dec 6, 2019

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
Copy link
Contributor

hypest commented Dec 6, 2019

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!

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

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

}

private boolean isPreTag() {
return !TextUtils.isEmpty(mTagName) && mTagName.equals(PRE_TAG);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fixed.

@marecar3 marecar3 merged commit 035fc31 into develop Dec 9, 2019
@marecar3 marecar3 deleted the issue/1264_activate_pre_format_block_android branch December 9, 2019 17:55
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.

Preformatted block
4 participants