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

Remove amp-fit-text block setting #5729

Merged
merged 7 commits into from
Jan 12, 2021

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Dec 22, 2020

Summary

Removes the amp-fit-text setting and related controls from the block panel and also removes the related block attributes from text blocks that had the setting enabled.

Fixes #4557

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@pierlon pierlon added Editor WS:Core Work stream for Plugin core labels Dec 22, 2020
@pierlon pierlon added this to the v2.1 milestone Dec 22, 2020
@pierlon pierlon self-assigned this Dec 22, 2020
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #5729 (015bb11) into develop (ac8cfe9) will increase coverage by 0.08%.
The diff coverage is 7.69%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5729      +/-   ##
=============================================
+ Coverage      74.14%   74.22%   +0.08%     
  Complexity      5498     5498              
=============================================
  Files            201      201              
  Lines          16653    16623      -30     
=============================================
- Hits           12347    12339       -8     
+ Misses          4306     4284      -22     
Flag Coverage Δ Complexity Δ
javascript 75.84% <7.69%> (+2.50%) 0.00 <0.00> (ø)
php 74.17% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
includes/admin/class-amp-editor-blocks.php 67.44% <ø> (ø) 17.00 <0.00> (ø)
assets/src/block-editor/helpers/index.js 29.10% <7.69%> (+0.44%) 0.00 <0.00> (ø)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2020

Plugin builds for 015bb11 are ready 🛎️!

Copy link
Contributor

@johnwatkins0 johnwatkins0 left a comment

Choose a reason for hiding this comment

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

One minor suggestion.

assets/src/block-editor/helpers/index.js Outdated Show resolved Hide resolved
@westonruter
Copy link
Member

Testing with this post_content:

<!-- wp:paragraph {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><p>paragraph</p></amp-fit-text>
<!-- /wp:paragraph -->

<!-- wp:heading {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><h2>teasdasd</h2></amp-fit-text>
<!-- /wp:heading -->

<!-- wp:code {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><pre class="wp-block-code"><code>test</code></pre></amp-fit-text>
<!-- /wp:code -->

<!-- wp:quote {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="5" max-font-size="73" height="90"><blockquote class="wp-block-quote"><p>quote</p></blockquote></amp-fit-text>
<!-- /wp:quote -->

Before the changes in this PR, it looks like this:

Non-AMP AMP
Screen Shot 2020-12-28 at 22 47 31 image

Upon upgrading to the build in this PR, accessing the editor results in block validation errors for the Code block and the Quote block:

image

image

Interesting the Header block and the Paragraph block automatically strip out the <amp-fit-text> tags, but they remain for Quote and Code:

<!-- wp:paragraph -->
<p>paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:heading -->
<h2>teasdasd</h2>
<!-- /wp:heading -->

<!-- wp:code -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><pre class="wp-block-code"><code>test</code></pre></amp-fit-text>
<!-- /wp:code -->

<!-- wp:quote -->
<amp-fit-text layout="fixed-height" min-font-size="5" max-font-size="73" height="90"><blockquote class="wp-block-quote"><p>quote</p></blockquote></amp-fit-text>
<!-- /wp:quote -->

Is the presence of block validation errors expected? I suppose so. Attempting block recovery for those two blocks is successful:

<!-- wp:paragraph -->
<p>paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:heading -->
<h2>teasdasd</h2>
<!-- /wp:heading -->

<!-- wp:code -->
<pre class="wp-block-code"><code>test</code></pre>
<!-- /wp:code -->

<!-- wp:quote -->
<blockquote class="wp-block-quote"><p>quote</p></blockquote>
<!-- /wp:quote -->

And then the result is identical AMP and non-AMP, and no trace of amp-fit-text.

Is this all working as intended?

@westonruter
Copy link
Member

Granted, there are probably no instances (or very few instances) of these blocks being used with fit-text.

@pierlon
Copy link
Contributor Author

pierlon commented Dec 29, 2020

Is the presence of block validation errors expected?

No, it should be be able to deprecate gracefully without any errors. I'll take a look at the example you provided.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@pierlon
Copy link
Contributor Author

pierlon commented Jan 11, 2021

I've fixed the issue in bdb65aa. The code and quote blocks were not being deprecated as expected due to an additional prop being added to the element after the block is saved. I've tracked it down to the core/generated-class-name/save-props filter which adds a generated className prop if the block supports the className feature (which all blocks do by default). The paragraph and heading blocks explicitly do not support the feature, which is why the issue did not occur on those blocks.

@westonruter
Copy link
Member

Fixed indeed!

Input:

<!-- wp:paragraph {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><p>paragraph</p></amp-fit-text>
<!-- /wp:paragraph -->

<!-- wp:heading {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><h2>teasdasd</h2></amp-fit-text>
<!-- /wp:heading -->

<!-- wp:code {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="4" max-font-size="172" height="172"><pre class="wp-block-code"><code>test</code></pre></amp-fit-text>
<!-- /wp:code -->

<!-- wp:quote {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="5" max-font-size="73" height="90"><blockquote class="wp-block-quote"><p>quote</p></blockquote></amp-fit-text>
<!-- /wp:quote -->

Output:

<!-- wp:paragraph -->
<p>paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:heading -->
<h2>teasdasd</h2>
<!-- /wp:heading -->

<!-- wp:code -->
<pre class="wp-block-code"><code>test</code></pre>
<!-- /wp:code -->

<!-- wp:quote -->
<blockquote class="wp-block-quote"><p>quote</p></blockquote>
<!-- /wp:quote -->

No block validation errors:

image

@westonruter
Copy link
Member

@pierlon Question: Is there a reason why you went with removeClassFromAmpFitTextBlocks in JS as opposed to using the existing ObsoleteBlockAttributeRemover?

@pierlon
Copy link
Contributor Author

pierlon commented Jan 11, 2021

The ObsoleteBlockAttributeRemover service would not be applicable in this context as the block deprecation is handled on the client side and so the added class attribute will never be seen by the service until the post is saved, and by that time the "This block contains invalid content" notice would already be displayed.

@westonruter
Copy link
Member

westonruter commented Jan 12, 2021

See also successful block migration messages:

image

@westonruter westonruter merged commit 89a2258 into develop Jan 12, 2021
@westonruter westonruter deleted the remove/4557-amp-fit-text-setting branch January 12, 2021 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editor WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate/remove AMP fit-text setting
3 participants