Add global style for Active Filters block #5465
Conversation
gigitux
added
block: active filters
rubikuserbot
requested review from
a team and
Aljullu
and removed request for
a team
|
Size Change: +50 B (0%) Total Size: 819 kB
ℹ️ View Unchanged
|
gigitux
force-pushed
the
add/4965-active-filters
branch
from
af20a74 to
833144f
Compare
There was a problem hiding this comment.
This is working great! However, I think we can simplify the code. In blocks that save some elements in the save function, like this case, I think using useBlockProps() is enough, and then there is no need to parse the attributes to generate class names and styles.
| 'wc-block-active-filters__title', | ||
| textColorClass | ||
| ) } | ||
| style={ textColorStyle } |
There was a problem hiding this comment.
Is it necessary to pass the style and the textColorClass? I would expect the heading element to simply inherit the color of the parent without us needing to do anything special.
| <div | ||
| className={ classnames( | ||
| 'wc-block-active-filters', | ||
| textColorClass |
There was a problem hiding this comment.
Same here, if we remove this class, would it just work? Color will be inherited from the parent div wrapper.
| className={ textColorClass } | ||
| style={ textColorStyle } |
| const blockProps = useBlockProps( { | ||
| className, | ||
| } ); | ||
|
|
||
| const textColorClassAndStyle = getTextColorClassAndStyleFromAttributeObject( | ||
| attributes | ||
| ); |
There was a problem hiding this comment.
Same here, I think we can remove those two variables and all styles will be applied by default.
From your comments, I understand that you prefer that the child components inherit the style from the parent component and not that we pass the class or style to each child? |
gigitux
force-pushed
the
add/4965-active-filters
branch
2 times, most recently
from
0a5fe54 to
4de894e
Compare
|
Thanks for letting me know. |
gigitux
force-pushed
the
add/4965-active-filters
branch
from
4de894e to
613d755
Compare
There was a problem hiding this comment.
Thanks for updating this PR @gigitux!
From your comments, I understand that you prefer that the child components inherit the style from the parent component and not that we pass the class or style to each child?
Right, from my understanding that's how global styles are expected to be used in most cases. The code becomes much simpler because we don't need to apply any 'custom solutions', all the magic is handled by Gutenberg. 🙂
I left a couple of comments but I'm pre-approving this PR because everything else is looking good.
| } ) => { | ||
| const TagName = `h${ headingLevel }`; | ||
| return ( | ||
| <TagName className={ className }> | ||
| <TagName style={ style } className={ className }> |
There was a problem hiding this comment.
Nit: given that this prop is no longer used, I think we should remove it?
src/BlockTypesController.php
Outdated
| @@ -191,7 +191,7 @@ protected function get_block_types() { | |||
| /** | |||
| * This disables specific blocks in Widget Areas by not registering them. | |||
| */ | |||
| if ( in_array( $pagenow, [ 'widgets.php', 'themes.php', 'customize.php' ], true ) ) { | |||
| if ( in_array( $pagenow, [ 'widgets.php', 'themes.php', 'customize.php' ], true ) && ! empty( 'gutenberg-edit-site' !== $_GET['page'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification | |||
There was a problem hiding this comment.
Just to clarify, is this empty() to account for the case that $_GET['page'] is not defined? In that case, I think you should check first that $_GET['page'] is defined and then do the comparison. Something like this:
&& ( empty( $_GET['page'] ) || 'gutenberg-edit-site' !== $_GET['page'] )
gigitux
force-pushed
the
add/4965-active-filters
branch
from
2269c01 to
f308d9f
Compare
This is a part of #4965.
This PR:
editor side
frontend side
Testing
Manual Testing
How to test the changes in this PR:
Check out this branch:
Gutenbergplugin.TT1 Blockstheme.Filters Product By Attribute)Clearbutton from Styles > Text color window.Stylesicon on the right-top corner.Blockssection .Changelog