Add wide and full alignment support for legacy template block #5433
Conversation
gigitux
added
focus: global styles
|
Size Change: +10 B (0%) Total Size: 819 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
@gigitux The alignment is working on my test 👏🏽 . I got several warnings testing this PR due to the new utility added in this PR, please check my comments for more detail.
src/Utils/StyleAttributesUtils.php
Outdated
| */ | ||
| public static function get_align_class_and_style( $attributes ) { | ||
|
|
||
| $align_attribute = $attributes['align']; |
There was a problem hiding this comment.
align may not exist in $attributes so we should check for its existence first.
src/BlockTypes/LegacyTemplate.php
Outdated
| $matches = array(); | ||
| preg_match( $pattern, $content, $matches ); | ||
|
|
||
| return preg_replace( $pattern, $matches[0] . ' ' . $align_class['class'], $content, 1 ); |
There was a problem hiding this comment.
$align_class may be null, so we need another check before using it as an array.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Mmm, it is very strange, I can't reproduce the warning
src/BlockTypes/LegacyTemplate.php
Outdated
| @@ -99,6 +102,7 @@ protected function render_single_product() { | |||
| * @return string Rendered block type output. | |||
| */ | |||
| protected function render_archive_product() { | |||
| add_filter( 'render_block', array( $this, 'get_markup_with_classes_by_attributes' ), 10, 2 ); | |||
There was a problem hiding this comment.
What do you think about calling this add_filter once in the initialize() method? I mean overriding the AbstractBlock::initialize() and adding this filter.
src/BlockTypes/LegacyTemplate.php
Outdated
| * @return string Rendered block type output. | ||
| */ | ||
| public function get_markup_with_classes_by_attributes( string $content, array $block ) { | ||
| $pattern = '/(?<=class=\")[^"]+(?=\")/'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For example, I modified the markup to simulate a wrapper-start.php template overridden by the theme.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Thanks for your great comments! I just update the PR, but I don't understand what do you mean with
|
src/BlockTypes/LegacyTemplate.php
Outdated
| $matches = array(); | ||
| preg_match( $pattern, $content, $matches ); | ||
|
|
||
| return preg_replace( $pattern, $matches[0] . ' ' . $align_class['class'], $content, 1 ); |
There was a problem hiding this comment.
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
274c858 to
bb19d24
Compare
src/BlockTypes/LegacyTemplate.php
Outdated
| if ( ! $this->is_legacy_template( $block ) ) { | ||
| return $content; | ||
| } |
There was a problem hiding this comment.
I think we can check for blockName key of the $block here instead of creating a new helper method (which checks for the template attribute).
src/BlockTypes/LegacyTemplate.php
Outdated
| * @param array $block Parsed block data. | ||
| * @return string Rendered block type output. | ||
| */ | ||
| public function get_markup_with_classes_by_attributes( string $content, array $block ) { |
There was a problem hiding this comment.
I think this should be renamed to add_alignment_class_to_wrapper for better expressing what the method does.
src/BlockTypes/LegacyTemplate.php
Outdated
| return $content; | ||
| } | ||
|
|
||
| $pattern = '/(?<=class=\")[^"]+(?=\")/'; |
There was a problem hiding this comment.
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 forLegacyTemplatewith themes that use<section>as the wrapper tag. - Here, we target the first element (any element, not limited to
<div>) that contain theclassattribute.
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.
There was a problem hiding this comment.
Ok, it makes sense!
I agree with you. This regex is not great :D
Let's wait for @mikejolley feedback!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
tjcafferkey
force-pushed
the
add/5229-add-align-support-legacy-template
branch
from
6166306 to
b4cf934
Compare
…om:woocommerce/woocommerce-gutenberg-products-block into add/5229-add-align-support-legacy-template
gigitux
force-pushed
the
add/5229-add-align-support-legacy-template
branch
from
8723818 to
f4689e6
Compare
There was a problem hiding this comment.
@gigitux The updates look good. I have some concerns about the regex, please check the review comments for more detail!
src/BlockTypes/LegacyTemplate.php
Outdated
| preg_match( $first_tag, $content, $matches ); | ||
|
|
||
| // If there is a tag, but it doesn't have a class attribute, add the class attribute. | ||
| if ( isset( $matches[0] ) && strpos( $matches[0], 'class' ) === false ) { |
There was a problem hiding this comment.
We should check for the exact class attribute here. Elements can have class as a part of attribute name or attribute value, like: data-class, data-type="class", id="fist-class".
| if ( isset( $matches[0] ) && strpos( $matches[0], 'class' ) === false ) { | |
| if ( isset( $matches[0] ) && strpos( $matches[0], ' class=' ) === false ) { |
src/BlockTypes/LegacyTemplate.php
Outdated
| preg_match( $pattern_before_tag_closing, $content, $matches ); | ||
| return preg_replace( $pattern_before_tag_closing, $matches[0] . ' class="' . $align_class_and_style['class'] . '"', $content, 1 ); |
There was a problem hiding this comment.
Can we use backreferences in preg_replace? If doing so, we can remove the preg_match above.
There was a problem hiding this comment.
I don't think with the current regex.
Because the value of match[0] is a tag element without < and >, for example:
div data-block-name="woocommerce/legacy-template" data-template="taxonomy-product_cat" id="primary"
But the regex /.+?(?=>)/ match the text before a tag closing >.
Potentially we can use str_replace, in this way:
return str_replace( $matches[0], $matches[0] . ' class="' . $align_class_and_style['class'] . '"', $content );
instead to use regex.
I don't have a strong opinion, what do you think?
There was a problem hiding this comment.
Because the value of
match[0]is a tag element without<and>, for example:
@gigitux I think we can use the fourth argument of preg_replace to only replace one time, you can check my example using your regex below:
<?php
$re = '/.+?(?=>)/m';
$str = '<div data-block-name="woocommerce/legacy-template" data-template="taxonomy-product_cat" id="primary" ><div class="inner">';
$subst = '$0 class="alignwide"';
$result = preg_replace($re, $subst, $str, 1);
echo "The result of the substitution is <pre>" . htmlspecialchars( $result ) . "</pre>";There was a problem hiding this comment.
Thanks for sharing this!
src/BlockTypes/LegacyTemplate.php
Outdated
| } | ||
|
|
||
| // If there is a tag, and it has a class already, add the class attribute. | ||
| $pattern_get_class = '/(?<=class=\")[^"]+(?=\")/'; |
There was a problem hiding this comment.
If the wrapper element uses single quotes for class, this regex will fail.
|
Thanks for your great feedback! I updated the regex. There is just a question, let me know, and I will update the PR! |
tjcafferkey
requested review from
tjcafferkey
and removed request for
tjcafferkey
gigitux
force-pushed
the
add/5229-add-align-support-legacy-template
branch
from
97ad7e7 to
0cbf68c
Compare
gigitux
force-pushed
the
add/5229-add-align-support-legacy-template
branch
from
0cbf68c to
891edf7
Compare
There was a problem hiding this comment.
@gigitux This is working like a charm 🎉 ! I've just left one comment for coding style consistency, otherwise, this is ready to be merged!
src/BlockTypes/LegacyTemplate.php
Outdated
| $matches_class = array(); | ||
| preg_match( $pattern_get_class, $content, $matches_class ); | ||
| return preg_replace( $pattern_get_class, $matches_class[0] . ' ' . $align_class_and_style['class'], $content, 1 ); |
There was a problem hiding this comment.
I think we can follow the same approach as we use at line 223rd above.
This PR adds support for the wide and full alignment for the legacy template block.
Fixes #5229
Screenshots
*editor side
*frontend side
Testing
Manual Testing
How to test the changes in this Pull Request:
Check out this branch
Product Category Page).You should test at least one between
Product Category Page,Product Archive PageandProduct Tag Page.You should test
Single Product Page.Changelog