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 2 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
33 changes: 33 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,15 @@ class LegacyTemplate extends AbstractDynamicBlock {
*/
protected $api_version = '2';


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

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

/**
* Get HTML markup with the right classes by attributes.
*
* @param string $content Block content.
* @param array $block Parsed block data.
* @return string Rendered block type output.
*/
public function get_markup_with_classes_by_attributes( string $content, array $block ) {
dinhtungdu marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be renamed to add_alignment_class_to_wrapper for better expressing what the method does.

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

The global/wrapper-start.php can be overridden in the theme, so the wrapper may not have the class attribute. Although I never see any wrapper element that doesn't have class, it's still a valid case. We don't need to cover that case in our logic, but a note for the theme developer will be nice here.

Copy link
Member

Choose a reason for hiding this comment

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

For example, I modified the markup to simulate a wrapper-start.php template overridden by the theme.

Screen Shot 2021-12-22 at 11 25 37

Copy link
Contributor Author

@gigitux gigitux Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The global/wrapper-start.php can be overridden in the theme, so the wrapper may not have the class attribute.

Great catch! I don't think about this case.

but a note for the theme developer will be nice here.

Do you mean to add a comment or what else?

Copy link
Member

@dinhtungdu dinhtungdu Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to cover that case in our logic

I want to clarify on this. I meant that we may not need to address that issue in our logic if it takes too much effort because it's a rare case. Personally, I never see any wrapper that doesn't have class. But if you can solve it, go ahead ; ).

Do you mean to add a comment or what else?

I was thinking about adding a comment to your callback get_markup_with_classes_by_attributes.

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

if ( ! isset( $matches[0] ) ) {
return $content;
}

return preg_replace( $pattern, $matches[0] . ' ' . $align_class['class'], $content, 1 );
Copy link
Member

Choose a reason for hiding this comment

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

$align_class may be null, so we need another check before using it as an array.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still getting a warning on this line:

Warning: Trying to access array offset on value of type null in /Users/tenup/Sites/woofse/app/public/wp-content/plugins/woocommerce-gutenberg-products-block/src/BlockTypes/LegacyTemplate.php on line 213

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, it is very strange, I can't reproduce the warning

}

}
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