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

Allow preformatted background alpha and tidying to be set from child classes #869

Merged
merged 11 commits into from
Dec 7, 2019

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Nov 7, 2019

Fix

This PR adds a method that can change preformatted background alpha color from the child class
and also adds method which can control tidying in AztecParser (we want to avoid tidying for preblock.

Related PR's:
Gutenberg PR: WordPress/gutenberg#18777
Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1615
WPAndroid PR: wordpress-mobile/WordPress-Android#10868

Test

Test 1:

  1. Run AztecEditor Demo app and check that background color isn't changed (should have 75% of alpha)
  2. Run AztecEditor Demo app and check that <br> tags on the end of the text are removed from pre after app is started.

Test 2:

  1. Run Gutenberg mobile PR: 1264 - Activate Preformatted block gutenberg-mobile#1442
    where value is set to 0, and see that background is transparent.
  2. Run Gutenberg mobile PR: 1264 - Activate Preformatted block gutenberg-mobile#1442
    with pre block with multiple <br> tags on the end of the text and check that they aren't removed.

Review

@daniloercoli

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.

@marecar3 marecar3 self-assigned this Nov 7, 2019
@marecar3 marecar3 changed the title Aztec preformatted block improvements Allow preformatted background alpha and tidying to be set from child classes Nov 7, 2019
@daniloercoli
Copy link
Contributor

I hope I have tested the right hashes :) but i'm still seeing problems with the BR tags at the end of the PRE block.
I see that some of those BR tags are removed when opened in GB-Mobile. Below is a test Preformatted block i was using during tests:

<!-- wp:preformatted -->
<pre class="wp-block-preformatted">This is a prefortmatted block
not a new 


but not that old








d









</pre>
<!-- /wp:preformatted -->

and when loading it in GB-mobile i see the following (sorry for the low-res picture):

photo5920352306182860876

@daniloercoli
Copy link
Contributor

Adding another thing i found on the problem reported above.

If i set the caret beside the d letter on the last line of text, last line before the empty line, and tap Enter.Key I see the caret going down and then back immediately beside the d letter.

@marecar3
Copy link
Contributor Author

marecar3 commented Nov 8, 2019

Adding another thing i found on the problem reported above.

If i set the caret beside the d letter on the last line of text, last line before the empty line, and tap Enter.Key I see the caret going down and then back immediately beside the d letter.

Hey Danilo, me and @mchowning have experienced a similar problem with quote block on the latest develop: wordpress-mobile/gutenberg-mobile#1555
Not 100% sure if the root cause is a same?

@marecar3
Copy link
Contributor Author

marecar3 commented Nov 8, 2019

I hope I have tested the right hashes :) but i'm still seeing problems with the BR tags at the end of the PRE block.
I see that some of those BR tags are removed when opened in GB-Mobile. Below is a test Preformatted block i was using during tests:

<!-- wp:preformatted -->
<pre class="wp-block-preformatted">This is a prefortmatted block
not a new 


but not that old








d









</pre>
<!-- /wp:preformatted -->

and when loading it in GB-mobile i see the following (sorry for the low-res picture):

photo5920352306182860876

Hey @daniloercoli, I have tested with your example:
Screen Shot 2019-11-08 at 12 10 37 PM

and I didn't find that BR tags are removed when GB-mobile is loaded

ezgif com-video-to-gif (10)

@hypest
Copy link
Contributor

hypest commented Nov 8, 2019

Tried with WPAndroid and noticed an issue with these steps:

  1. Start a post on the web with:
<!-- wp:preformatted -->
<pre class="wp-block-preformatted">just a text<br>line2<br></pre>
<!-- /wp:preformatted -->
  1. Open the post in the mobile editor
  2. Place the caret at the end of "line2"
  3. Exit the editor without any actual editing
  4. Notice the post marked as "Local changes"
  5. Open the post again, it looks OK.
  6. Switch to html and notice the <br>s are not there
    6b. Open the post on the web and notice a new autosave available. Check the revision viewer and notice that the <br>s have been replaced with newlines.

Is that <br> replacement expected?

@daniloercoli
Copy link
Contributor

daniloercoli commented Nov 8, 2019

You are totally right @marecar3 - it was a problem on my side with me not updating the Aztec hash on WP-Android. I guess i messed up with things when pointing to your branches. Sorry about that.

I've also found another problem when merging an empty para block with the PRE block above.
See the video here: https://cloudup.com/cWRJW-rtDLm

Steps to repro:

  1. Create a preformatted block with empty lines in it
  2. Create new paragraph below it
  3. Tap Backspace to merge those
  4. You will see a preformatted paragraph with all the new lines/ br tags got removed

Edited: It seems the web is affected by the issue mentioned above. So not a problem introduced in this PR.

@SergioEstevao
Copy link

You are totally right @marecar3 - it was a problem on my side with me not updating the Aztec hash on WP-Android. I guess i messed up with things when pointing to your branches. Sorry about that.

I've also found another problem when merging an empty para block with the PRE block above.
See the video here: https://cloudup.com/cWRJW-rtDLm

Steps to repro:

  1. Create a preformatted block with empty lines in it
  2. Create new paragraph below it
  3. Tap Backspace to merge those
  4. You will see a preformatted paragraph with all the new lines/ br tags got removed

This is a problem in Gutenberg code base it also happens on the web.

@daniloercoli
Copy link
Contributor

Oh yes, we have mentioned that on Slack on Friday, but forgot to update the description here.

At this point, if the parent PR passes testing phase we can merge this one.

@hypest
Copy link
Contributor

hypest commented Nov 12, 2019

I tried #869 (comment) again but now I see that if I re-open the post the editor red-screens with block validation error.

daniloercoli added a commit to wordpress-mobile/gutenberg-mobile that referenced this pull request Nov 12, 2019
…ith invalid PRE block content sent to the JS side under some circumstances, even without actually changing the content of the block. Ref: wordpress-mobile/AztecEditor-Android#869 (comment)
@marecar3
Copy link
Contributor Author

marecar3 commented Dec 6, 2019

👋 @hypest, if there are no new requests from your side, maybe we can merge this PR and start with a new tag release for Aztec Android?

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.

LGTM Marko! Feel free to merge, create a release and update the WPAndroid and gutenberg-mobile PRs, thanks!

@hypest
Copy link
Contributor

hypest commented Dec 6, 2019

Oh, can you also link to the related WPAndroid and gutenberg-mobile PRs in the description @marecar3 ? Thanks!

@marecar3
Copy link
Contributor Author

marecar3 commented Dec 7, 2019

Oh, can you also link to the related WPAndroid and gutenberg-mobile PRs in the description @marecar3 ? Thanks!

Thanks for the hint. Now they are added.

@marecar3 marecar3 merged commit 7b38c34 into develop Dec 7, 2019
@marecar3 marecar3 deleted the experiment/aztec_preformatted_block branch December 7, 2019 06:04
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.

4 participants