Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Add wide and full alignment support for legacy template block #5433

Merged
merged 13 commits into from
Jan 7, 2022
Merged

Add wide and full alignment support for legacy template block #5433

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fe21f86
add align wide and full support for legacy template block
gigitux Dec 21, 2021
7a755d8
fix PHP warning
gigitux Dec 22, 2021
bb19d24
add a comment on get_markup_with_classes_by_attributes
gigitux Dec 23, 2021
6166306
rename function
gigitux Dec 27, 2021
012741e
add align wide and full support for legacy template block
gigitux Dec 21, 2021
987445e
fix PHP warning
gigitux Dec 22, 2021
187bb61
add a comment on get_markup_with_classes_by_attributes
gigitux Dec 23, 2021
b4cf934
rename function
gigitux Dec 27, 2021
2aeca80
Merge branch 'add/5229-add-align-support-legacy-template' of github.c…
gigitux Jan 4, 2022
f4689e6
fix regex
gigitux Jan 4, 2022
e291dc3
update regex
gigitux Jan 5, 2022
891edf7
update regex
gigitux Jan 6, 2022
6ed25c3
fix code styling
gigitux Jan 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/js/blocks/legacy-template/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ registerBlockType( 'woocommerce/legacy-template', {
'woo-gutenberg-products-block'
),
supports: {
align: false,
align: [ 'wide', 'full' ],
html: false,
multiple: false,
reusable: false,
Expand Down
39 changes: 39 additions & 0 deletions src/BlockTypes/LegacyTemplate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php
namespace Automattic\WooCommerce\Blocks\BlockTypes;

use Automattic\WooCommerce\Blocks\Utils\StyleAttributesUtils;

/**
* Legacy Single Product class
*
Expand All @@ -21,6 +23,14 @@ class LegacyTemplate extends AbstractDynamicBlock {
*/
protected $api_version = '2';

/**
* Initialize this block.
*/
protected function initialize() {
parent::initialize();
add_filter( 'render_block', array( $this, 'add_alignment_class_to_wrapper' ), 10, 2 );
}

/**
* Render method for the Legacy Template block. This method will determine which template to render.
*
Expand Down Expand Up @@ -180,4 +190,33 @@ protected function render_archive_product() {
wp_reset_postdata();
return ob_get_clean();
}

/**
* Get HTML markup with the right classes by attributes.
* This function appends the classname at the first element that have the class attribute.
* Based on the experience, all the wrapper elements have a class attribute.
*
* @param string $content Block content.
* @param array $block Parsed block data.
* @return string Rendered block type output.
*/
public function add_alignment_class_to_wrapper( string $content, array $block ) {
if ( ( 'woocommerce/' . $this->block_name ) !== $block['blockName'] ) {
return $content;
}

$pattern = '/(?<=class=\")[^"]+(?=\")/';
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the BlockTypesController::add_data_attributes, I notice the ways we target the element there and in this method are different:

  • In BlockTypesController::add_data_attributes, we target only the openning <div>. This may not work for LegacyTemplate with themes that use <section> as the wrapper tag.
  • Here, we target the first element (any element, not limited to <div>) that contain the class attribute.

IMO, we need to use the same mechanism to target wrapper elements in both places. I'm tagging @mikejolley here for more insights because he originally created BlockTypesController::add_data_attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it makes sense!
I agree with you. This regex is not great :D
Let's wait for @mikejolley feedback!

Copy link
Member

Choose a reason for hiding this comment

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

In the case of add_data_attributes we know we're dealing with a div element because thats how the block markup is saved to the post. If the opening element is unknown, your regex is probably better, but I wonder if you can still target only the opening element rather than the first class found which could be deeper? I am not going to pretend to be regex expert, because I am not :D

Copy link
Contributor Author

@gigitux gigitux Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

but I wonder if you can still target only the opening element rather than the first class found which could be deeper? I am not going to pretend to be regex expert, because I am not :D

It can be a good idea, but I'm pretty sure that the regex will be hard to write and read. I'm not a fan of complex regex, but if you think that it makes sense to work on it, I can do it!

Copy link
Member

Choose a reason for hiding this comment

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

It might not be desirable to add that class deeper in case it breaks other parts of the layout 👍🏻

Are there any alternatives considered for this additional class inserted by regex? Would it be a disaster to wrap in another div with the class, or does that not work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any alternatives considered for this additional class inserted by regex? Would it be a disaster to wrap in another div with the class, or does that not work here?

It works without problem, but I don't know if we can wrap a block in another div. If we can follow this path, I think that it is the best solution.

image

Copy link
Member

Choose a reason for hiding this comment

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

works without problem, but I don't know if we can wrap a block in another div. If we can follow this path, I think that it is the best solution.

I think wrapping inside another element can make it harder (and confusing) for themes developers to style the page because the opening element now is not the first element in the wrapper-start.php but the alignment div.

Copy link
Contributor Author

@gigitux gigitux Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

With f4689e6, I implemented a series of regexes.

First of all, I pick the first tag HTML, after that:

  • if the first tag already has the class attribute, I just add the new class.
  • if the first tag doesn't have the class attribute, I add the class attribute + the new class

Let me know what do you think :D

$attributes = (array) $block['attrs'];
$align_class_and_style = StyleAttributesUtils::get_align_class_and_style( $attributes );
$matches = array();
preg_match( $pattern, $content, $matches );

if ( ! isset( $matches[0] ) || ! isset( $align_class_and_style['class'] ) ) {
return $content;
}

return preg_replace( $pattern, $matches[0] . ' ' . $align_class_and_style['class'], $content, 1 );
}


}
53 changes: 53 additions & 0 deletions src/Utils/StyleAttributesUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,59 @@ public static function get_background_color_class_and_style( $attributes ) {
return null;
}

/**
* Get class and style for align from attributes.
*
* @param array $attributes Block attributes.
*
* @return (array | null)
*/
public static function get_align_class_and_style( $attributes ) {
dinhtungdu marked this conversation as resolved.
Show resolved Hide resolved

$align_attribute = isset( $attributes['align'] ) ? $attributes['align'] : null;

if ( ! $align_attribute ) {
return null;
};

if ( 'wide' === $align_attribute ) {
return array(
'class' => 'alignwide',
'style' => null,
);
}

if ( 'full' === $align_attribute ) {
return array(
'class' => 'alignfull',
'style' => null,
);
}

if ( 'left' === $align_attribute ) {
return array(
'class' => 'alignleft',
'style' => null,
);
}

if ( 'right' === $align_attribute ) {
return array(
'class' => 'alignright',
'style' => null,
);
}

if ( 'center' === $align_attribute ) {
return array(
'class' => 'aligncenter',
'style' => null,
);
}

return null;
}

/**
* Get classes and styles from attributes.
*
Expand Down