Add global style for Product Categories List block #5133
Conversation
rubikuserbot
requested review from
a team and
dinhtungdu
and removed request for
a team
gigitux
force-pushed
the
add/4965-global-style-product-categories-list
branch
from
530b89d to
7655afc
Compare
src/Utils/AttributesUtils.php
Outdated
| /** | ||
| * AttributesUtils class used for getting class and style from attributes. | ||
| */ | ||
| class AttributesUtils { |
There was a problem hiding this comment.
I'm not sure about this name. Maybe is it better GlobalStyleUtils?
There was a problem hiding this comment.
Agree, or maybe a mix between the two: StyleAttributesUtil? 🤔
There was a problem hiding this comment.
StyleAttributesUtils makes more sense to me, because the global part is automatically generated.
|
Size Change: +881 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
gigitux
added
block: product categories list
There was a problem hiding this comment.
Good job @gigitux adding global styles to the Product Categories List block. The utils class you created can be very useful for other PHP-rendered blocks. 👏
The link color support doesn't work with theme colors.
I think that's because we need to add this class to the block: wp-block-woocommerce-product-categories. By default, Gutenberg appends a class like this to all blocks, but because we are rendering it in PHP, we need to do it manually. I think once you add it, the link styles will apply correctly.
When the user changes the font size, the updated block is not rendered.
I'm not able to reproduce this issue (tested with Gutenberg trunk and theme Quadrat).
I added some other comments below, but overall the approach looks good to me. 👌
| @@ -13,6 +13,7 @@ import './style.scss'; | |||
| import Block from './block.js'; | |||
|
|
|||
| registerBlockType( 'woocommerce/product-categories', { | |||
| apiVersion: 2, | |||
There was a problem hiding this comment.
With apiVersion: 2, you will need to pass useBlockProps() to the edit component. Similarly to how it's done here:
(I think you will need to replace the fragment <> with a wrapping div)
src/BlockTypes/ProductCategories.php
Outdated
|
|
||
| $output = '<div class="' . esc_attr( $classes ) . '">'; | ||
| $output = '<div class="' . esc_attr( $classes ) . '" style="' . $styles . '">'; |
There was a problem hiding this comment.
We should escape every variable added to the output:
| $output = '<div class="' . esc_attr( $classes ) . '" style="' . $styles . '">'; | |
| $output = '<div class="' . esc_attr( $classes ) . '" style="' . esc_attr( $styles ) . '">'; |
src/Utils/AttributesUtils.php
Outdated
| /** | ||
| * AttributesUtils class used for getting class and style from attributes. | ||
| */ | ||
| class AttributesUtils { |
There was a problem hiding this comment.
Agree, or maybe a mix between the two: StyleAttributesUtil? 🤔
src/Utils/AttributesUtils.php
Outdated
| public static function get_font_size_class_and_style( $attributes ) { | ||
|
|
||
| $font_size = $attributes['fontSize']; | ||
| $custom_font_size = $attributes['style']['typography']['fontSize']; |
There was a problem hiding this comment.
If you set define( 'WP_DEBUG', 'True' ); in your site's wp-config.php, you will probably see errors appear on the screen because of this line. That's because typography might be missing inside the style object.
I think you will need to use array_key_exists() (or something similar?) every time you try to access an attribute inside $attributes['style'].
src/Utils/AttributesUtils.php
Outdated
| return array( | ||
| 'class' => null, | ||
| 'style' => null, | ||
| ); |
There was a problem hiding this comment.
In this case, should we just return null?
src/BlockTypes/ProductCategories.php
Outdated
| foreach ( $categories as $category ) { | ||
| $output .= ' | ||
| <li class="wc-block-product-categories-list-item"> | ||
| <a href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> | ||
| <a style="' . $link_color_class_and_style['style'] . '" class="' . $link_color_class_and_style['class'] . '" href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> |
There was a problem hiding this comment.
Here too, we should escape all attributes:
| <a style="' . $link_color_class_and_style['style'] . '" class="' . $link_color_class_and_style['class'] . '" href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> | |
| <a style="' . esc_attr( $link_color_class_and_style['style'] ) . '" class="' . esc_attr( $link_color_class_and_style['class'] ) . '" href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> |
src/BlockTypes/ProductCategories.php
Outdated
| $font_size_class_and_style = AttributesUtils::get_font_size_class_and_style( $attributes ); | ||
|
|
||
| $classes = array_filter( | ||
| [ $line_height_class_and_style['class'], $text_color_class_and_style['class'], $font_size_class_and_style['class'] ], |
There was a problem hiding this comment.
$line_height_class_and_style might be null (according to this line), so here you will also need to check if class exists before using it.
Same for the other objects.
src/Utils/AttributesUtils.php
Outdated
| * | ||
| * @param array $attributes Block attributes. | ||
| * | ||
| * @return string |
There was a problem hiding this comment.
We should update this comment to match the actual return value types.
src/Utils/AttributesUtils.php
Outdated
| return array_filter( | ||
| $classes_and_styles, | ||
| function ( $var ) { | ||
| return ! is_null( $var ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
array_filter default callbackup already removes all falsy items, so we can use it without the callback.
| return array_filter( | |
| $classes_and_styles, | |
| function ( $var ) { | |
| return ! is_null( $var ); | |
| } | |
| ); | |
| return array_filter( $classes_and_styles ); |
gigitux
force-pushed
the
add/4965-global-style-product-categories-list
branch
2 times, most recently
from
02dd6e6 to
fe77684
Compare
Add global style for product categories list block
gigitux
force-pushed
the
add/4965-global-style-product-categories-list
branch
from
fe77684 to
eeb0d39
Compare
|
Thanks for your review. I fix the code.
|
There was a problem hiding this comment.
@gigitux Great work on this! The block style is working on my end, both editor and the front end. But the global style only works in the editor. We have this issue because the wrapper element of the block on the frontend doesn't have wp-block-woocommerce-product-categories class.
src/BlockTypes/ProductCategories.php
Outdated
| foreach ( $categories as $category ) { | ||
| $output .= ' | ||
| <li class="wc-block-product-categories-list-item"> | ||
| <a href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> | ||
| <a style="' . esc_attr( $link_color_style ) . '" href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> |
|
|
||
| $output = '<div class="' . esc_attr( $classes ) . '">'; | ||
| $output = '<div class="wp-block-woocommerce-product-categories ' . esc_attr( $classes ) . '" style="' . esc_attr( $styles ) . '">'; |
There was a problem hiding this comment.
There is a possibility that the style property will be empty. Do you think that is it necessary to exclude style property when the variable $styles is empty?
The same concept for class property and variable $classes
There was a problem hiding this comment.
@gigitux Empty style or class attribute is fine to me. But if you prefer a cleaner HTML output, go for it. I prefer empty attributes because of the readability. We will need some if condition checks to exclude the empty attributes, which makes the code harder to read IMO.
There was a problem hiding this comment.
We will need some if condition checks to exclude the empty attributes, which makes the code harder to read IMO.
I agree with you! In any case, I preferred to point it out to you
There was a problem hiding this comment.
@gigitux This is working great on my end, both local and global block styles are working. I left some comments about coding standards and minor issues, please take a look.
src/BlockTypes/ProductCategories.php
Outdated
| 'fontSize' => $this->get_schema_string(), | ||
| 'lineHeight' => $this->get_schema_string(), | ||
| 'style' => array( 'type' => 'object' ), | ||
|
|
There was a problem hiding this comment.
This empty line should be removed.
src/BlockTypes/ProductCategories.php
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Extra empty lines here should be removed too.
src/BlockTypes/ProductCategories.php
Outdated
| $string_classes = implode( ' ', $classes ); | ||
| $string_styles = implode( ' ', $styles ); | ||
|
|
||
| return array( $string_classes, $string_styles ); |
There was a problem hiding this comment.
You're changing the return data type of this function, so you should update the comment docs block as well.
By the way, this method now returns an array of classes and styles, then the method name should be updated to reflect the return data.
src/BlockTypes/ProductCategories.php
Outdated
| $font_size_class = isset( $font_size_class_and_style['class'] ) ? $font_size_class_and_style['class'] : null; | ||
|
|
||
| $classes = array_filter( | ||
| [ $line_height_class, $text_color_class, $font_size_class ] |
There was a problem hiding this comment.
We should use only one array style. Same for line 141 below.
There was a problem hiding this comment.
What do you mean with use only one array style. Maybe do you mean that I should use array?
| [ $line_height_class, $text_color_class, $font_size_class ] | |
| array($line_height_class, $text_color_class, $font_size_class) |
There was a problem hiding this comment.
@gigitux Yeah, I mean the syntax, we should use one style to define arrays.
src/BlockTypes/ProductCategories.php
Outdated
| <a style="' . esc_attr( $link_color_style ) . '" href="' . esc_attr( get_term_link( $category->term_id, 'product_cat' ) ) . '">' . $this->get_image_html( $category, $attributes ) . esc_html( $category->name ) . '</a> | ||
| ' . $this->getCount( $category, $attributes ) . ' |
There was a problem hiding this comment.
I'm not sure why the indentation of these lines needs to change. They look correct.
src/BlockTypes/ProductCategories.php
Outdated
| @@ -1,11 +1,15 @@ | |||
| <?php | |||
| namespace Automattic\WooCommerce\Blocks\BlockTypes; | |||
|
|
|||
| use Automattic\WooCommerce\Blocks\Utils\StyleAttributesUtils; | |||
|
|
|||
|
|
|||
src/BlockTypes/ProductCategories.php
Outdated
| @@ -301,11 +338,15 @@ protected function renderList( $categories, $attributes, $uid, $depth = 0 ) { | |||
| protected function renderListItems( $categories, $attributes, $uid, $depth = 0 ) { | |||
| $output = ''; | |||
|
|
|||
| $link_color_class_and_style = StyleAttributesUtils::get_link_color_class_and_style( $attributes ); | |||
|
|
|||
| $link_color_style = $link_color_class_and_style['style']; | |||
There was a problem hiding this comment.
This throws a warning on my end: Warning: Trying to access array offset on value of type null in /wp-content/plugins/woocommerce-gutenberg-products-block/src/BlockTypes/ProductCategories.php on line 343.
gigitux
force-pushed
the
add/4965-global-style-product-categories-list
branch
from
f2096f6 to
bbd1bfa
Compare
There was a problem hiding this comment.
Hi @gigitux I'm not sure if you finished updating the PR but I left some more comments on the latest changes.
src/BlockTypes/ProductCategories.php
Outdated
| @@ -90,9 +99,10 @@ protected function render( $attributes, $content ) { | |||
| * Get the list of classes to apply to this block. | |||
There was a problem hiding this comment.
This comment needs to be updated as well.
src/Utils/StyleAttributesUtils.php
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Consecutive empty lines here.
src/Utils/StyleAttributesUtils.php
Outdated
| line_height => self::get_line_height_class_and_style( $attributes ), | ||
| text_color => self::get_text_color_class_and_style( $attributes ), | ||
| font_size => self::get_font_size_class_and_style( $attributes ), | ||
| link_color => self::get_link_color_class_and_style( $attributes ), |
There was a problem hiding this comment.
The array keys need to be string
src/Utils/StyleAttributesUtils.php
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Consecutive empty lines here.
src/Utils/StyleAttributesUtils.php
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Consecutive empty lines here.
src/BlockTypes/ProductCategories.php
Outdated
| $line_height_class_and_style = StyleAttributesUtils::get_line_height_class_and_style( $attributes ); | ||
| $text_color_class_and_style = StyleAttributesUtils::get_text_color_class_and_style( $attributes ); | ||
| $font_size_class_and_style = StyleAttributesUtils::get_font_size_class_and_style( $attributes ); | ||
|
|
||
| $line_height_class = isset( $line_height_class_and_style['class'] ) ? $line_height_class_and_style['class'] : null; | ||
| $text_color_class = isset( $text_color_class_and_style['class'] ) ? $text_color_class_and_style['class'] : null; | ||
| $font_size_class = isset( $font_size_class_and_style['class'] ) ? $font_size_class_and_style['class'] : null; | ||
|
|
||
| $classes = array_filter( | ||
| array( $line_height_class, $text_color_class, $font_size_class ) | ||
| ); | ||
|
|
||
| $line_height_style = isset( $line_height_class_and_style['style'] ) ? $line_height_class_and_style['style'] : null; | ||
| $text_color_style = isset( $text_color_class_and_style['style'] ) ? $text_color_class_and_style['style'] : null; | ||
| $font_size_style = isset( $font_size_class_and_style['style'] ) ? $font_size_class_and_style['style'] : null; | ||
|
|
||
| $styles = array_filter( | ||
| array( $line_height_style, $text_color_style, $font_size_style ) | ||
| ); |
There was a problem hiding this comment.
I'm wondering if we can use StyleAttributesUtils:get_classes_and_styles_by_attributes here? Something like this:
| $line_height_class_and_style = StyleAttributesUtils::get_line_height_class_and_style( $attributes ); | |
| $text_color_class_and_style = StyleAttributesUtils::get_text_color_class_and_style( $attributes ); | |
| $font_size_class_and_style = StyleAttributesUtils::get_font_size_class_and_style( $attributes ); | |
| $line_height_class = isset( $line_height_class_and_style['class'] ) ? $line_height_class_and_style['class'] : null; | |
| $text_color_class = isset( $text_color_class_and_style['class'] ) ? $text_color_class_and_style['class'] : null; | |
| $font_size_class = isset( $font_size_class_and_style['class'] ) ? $font_size_class_and_style['class'] : null; | |
| $classes = array_filter( | |
| array( $line_height_class, $text_color_class, $font_size_class ) | |
| ); | |
| $line_height_style = isset( $line_height_class_and_style['style'] ) ? $line_height_class_and_style['style'] : null; | |
| $text_color_style = isset( $text_color_class_and_style['style'] ) ? $text_color_class_and_style['style'] : null; | |
| $font_size_style = isset( $font_size_class_and_style['style'] ) ? $font_size_class_and_style['style'] : null; | |
| $styles = array_filter( | |
| array( $line_height_style, $text_color_style, $font_size_style ) | |
| ); | |
| foreach( StyleAttributesUtils::get_classes_and_styles_by_attributes( $attributes ) as $key => $item ) { | |
| // We skip link_color for wrapper element. | |
| if ( 'link_color' === $key ) { | |
| continue; | |
| } | |
| if ( ! empty( $item['class'] ) ) { | |
| $classes[] = $item['class']; | |
| } | |
| if ( ! empty( $item['style'] ) ) { | |
| $styles[] = $item['style']; | |
| } | |
| } |
There was a problem hiding this comment.
I'm wondering if we can use StyleAttributesUtils:get_classes_and_styles_by_attributes here? Something like this:
Maybe in the future, we could add more attributes and if this happens we should update this foreach. I would prefer to use an immutable approach. What do you think?
There was a problem hiding this comment.
I agree with an immutable approach. But I think we still need too much code to calculate classes/styles string to use in markup. I have an idea to refactor the util in #5277, please take a look!
Done and approved! Thanks for your great feedback!
gigitux
force-pushed
the
add/4965-global-style-product-categories-list
branch
from
4f67958 to
2b96117
Compare
* Add global style for product categories list block woocommerce#4965 Add global style for product categories list block * add support for link color * add feature flag * fix code style and PHP warning * update doc comment * remove empty space * refactor StyleAttributesUtils (woocommerce#5277) Co-authored-by: Tung Du <[email protected]>
* Add global style for product categories list block woocommerce#4965 Add global style for product categories list block * add support for link color * add feature flag * fix code style and PHP warning * update doc comment * remove empty space * refactor StyleAttributesUtils (woocommerce#5277) Co-authored-by: Tung Du <[email protected]>
This is a part of #4965.
This PR:
What missing
Link color support
The link color support doesn't work with theme colors. We are investigating this problem (internal discussionp1636652209321000-slack-C02FL3X7KR6)With
useBlockPropshook, I managed to make link color work 👍Font size
When the user changes the font size, the updated block is not rendered. We are investigating this problem (internal discussionp1636643690311900-slack-C02FL3X7KR6)When the user changes the font size from the post/page editor everything works correctly. It seems that it doesn't work when the user changes the font size from the site editor (the font size is not applied to any element on the page)With the last version of Gutenberg, I'm not able to reproduce this problem 👍
before - editor
before - page
current implementation - new options in the editor
current implementation - new options in the editor
current implementation - block with custom styles
Testing
Manual Testing
How to test the changes in this PR:
Check out this branch:
GutenbergpluginTT1 BlocksthemeSite EditorChangelog