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

Image Block: Move any is-style and custom classnames up to deprecated alignment wrapper #39330

Merged
merged 12 commits into from
Mar 17, 2022

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Mar 10, 2022

What?

For non theme.json themes any is-style or custom classnames added to an image block in combination with a specified alignment will be copied to the deprecated div alignment wrapper that is added back to the frontend.

Why?

There are many themes/plugins that are relying on these custom classnames being on the same element as the wp-block-image class which is also moved to this deprecated wrapper div.

How?

The block className attribute is applied to the <div> wrapper that is added.

Testing Instructions

  • Add an image to a site with TwentyTwentyOne theme running and select one of the border styles and also set the block to align right
  • Check that the image style displays as expected in the frontend.
  • Check that phpunit/block-supports/layout-test.php tests pass

Screenshots

Before:
Screen Shot 2022-03-14 at 3 47 06 PM

After:
Screen Shot 2022-03-14 at 3 35 42 PM

@glendaviesnz glendaviesnz force-pushed the fix/image-align-wrapper-missing-classes branch from 00e7255 to c9f02f9 Compare March 14, 2022 02:11
@glendaviesnz glendaviesnz marked this pull request as ready for review March 14, 2022 02:13
@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Mar 14, 2022

@youknowriad I have reduced this to a single regex call and added some unit tests. @mcsf I would appreciate some 👀 on the revised regex if/when you have a minute - it appears to work as expected, but there may very well be a more efficient/robust way to add the optional/non-capturing group for the block className attrib string.

@glendaviesnz glendaviesnz added [Type] Bug An existing feature does not function as intended and removed [Status] In Progress Tracking issues with work in progress labels Mar 14, 2022
@glendaviesnz glendaviesnz changed the title Image Block: Move an is-style and custom classnames up to deprecated alignment wrapper Image Block: Move any is-style and custom classnames up to deprecated alignment wrapper Mar 14, 2022
phpunit/block-supports/layout-test.php Outdated Show resolved Hide resolved
phpunit/block-supports/layout-test.php Show resolved Hide resolved
$image_with_align = '/(^\s*<figure\b[^>]*)\bwp-block-image\b([^"]*\b(?:alignleft|alignright|aligncenter)\b[^>]*>.*<\/figure>)/U';
function gutenberg_restore_image_outer_container( $block_content, $block ) {
$block_classnames = isset( $block['attrs']['className'] ) ? $block['attrs']['className'] : '';
$image_with_align = '/(^\s*<figure\b[^>]*)\bwp-block-image\b([^"]*\b(?:alignleft|alignright|aligncenter).*)\b(?:' . $block_classnames . ')([^>]*>.*<\/figure>)/U';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that the classnames show up in a different order than the regex?

Copy link
Member

Choose a reason for hiding this comment

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

Should the $accepted_args on the add_filter below be now 2?

E.g.,

add_filter( 'render_block_core/image', 'gutenberg_restore_image_outer_container', 10, 2 );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the $accepted_args on the add_filter below be now 2

True, fixed, thanks

Copy link
Contributor Author

@glendaviesnz glendaviesnz Mar 15, 2022

Choose a reason for hiding this comment

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

Is it possible that the classnames show up in a different order than the regex?

@youknowriad I don't know what the chances are of this, but anything is possible! I have refactored this to hopefully account for any order of the classnames, which also made it easier to get rid of those extra spaces as an added bonus, and have also managed to reduce it to a single regex 🎉 I will do a bit more testing on the regex tomorrow though to make sure all the important cases are covered.

@ramonjd
Copy link
Member

ramonjd commented Mar 14, 2022

Here's what I'm seeing.

From the editor (using TwentyTwentyOne):

<figure class="wp-block-image alignleft size-full is-resized custom-user-class is-style-twentytwentyone-border">
    <a href="http://localhost:8888/wp-content/uploads/2022/02/issued-sound.jpg">
        <img src="http://localhost:8888/wp-content/uploads/2022/02/issued-sound.jpg" alt="" class="wp-image-79" width="367" height="571"/>
    </a>
</figure>
<div class="wp-block-image custom-user-class is-style-twentytwentyone-border">
    <figure class="wp-duotone-8c00b7-fcff41-1  alignleft size-full is-resized ">
        <a href="http://localhost:8888/wp-content/uploads/2022/02/issued-sound.jpg">
            <img src="http://localhost:8888/wp-content/uploads/2022/02/issued-sound.jpg" alt="" class="wp-image-79" width="367" height="571">
        </a>
    </figure>
</div>

Are other modifying classnames such as size-full is-resized meant to also be on the <figure />?

@glendaviesnz
Copy link
Contributor Author

@davecpage just pinging you as you worked on something similar with the group block. No problem if you don't have time to look, but if you have time to cast your 👀 over the regex added to this PR, it would be good to get another opinion on it. Hopefully, the unit tests explain what should be happening.

@glendaviesnz
Copy link
Contributor Author

Are other modifying classnames such as size-full is-resized meant to also be on the <figure />?

I just tried a 5.9 install with no GB plugin, so prior to the wrapper div being removed, and it appears that only the wp-block-image class and the block.attributes.className classes that get put on the outer alignment wrapper:
Screen Shot 2022-03-15 at 4 30 24 PM

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

@glendaviesnz glendaviesnz force-pushed the fix/image-align-wrapper-missing-classes branch from 4164a3d to d930299 Compare March 15, 2022 21:27
@ramonjd
Copy link
Member

ramonjd commented Mar 15, 2022

This is working pretty well out of the box for me using a very contrived example for a single image.

Tested multiple images on a page.

Good call to capture class= in the regex to avoid false positives in preceding attributes too!

Where I give the image left, right or center alignment the HTML is modified as expected:

Editor

<figure class="wp-block-image alignright size-medium is-style-twentytwentyone-border is-style__test a-custom__style-123" id="wp-block-image">
     <img src="http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-200x300.jpg" alt="Alt text wp-block-image" class="wp-image-22" title="wp-block-image"/>
</figure>

Frontend

<div class="wp-block-image is-style-twentytwentyone-border wp-block-image is-style__test a-custom__style-123">
    <figure class="wp-duotone-a60072-67ff66-1 alignright size-medium" id="wp-block-image">
        <img width="200" height="300" src="http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-200x300.jpg" alt="Alt text wp-block-image" class="wp-image-22" title="wp-block-image" srcset="http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-200x300.jpg 200w, http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-683x1024.jpg 683w, http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-768x1152.jpg 768w, http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-1024x1536.jpg 1024w, http://localhost:4759/wp-content/uploads/2022/03/kramervskramer-1365x2048.jpg 1365w, http://localhost:4759/wp-content/uploads/2022/03/kramervskramer.jpg 1400w" sizes="(max-width: 200px) 100vw, 200px">
    </figure>
</div>

Just checking that the regex should not take alignfull into account.

@glendaviesnz
Copy link
Contributor Author

Just checking that the regex should not take alignfull into account.

Good questions. I loaded 5.9 without plugin to double-check and alignwide and alignfull didn't previously add the additional wrapper div, only alignleft, aligncenter, alignright did that, so only those 3 need to be covered by this.

@glendaviesnz glendaviesnz force-pushed the fix/image-align-wrapper-missing-classes branch from d930299 to a497ccc Compare March 16, 2022 20:08
@glendaviesnz glendaviesnz merged commit c23ede3 into trunk Mar 17, 2022
@glendaviesnz glendaviesnz deleted the fix/image-align-wrapper-missing-classes branch March 17, 2022 04:40
@github-actions github-actions bot added this to the Gutenberg 12.9 milestone Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants