Product Title block: add support global style #5515
Conversation
gigitux
added
the
focus: global styles
gigitux
added
focus: FSE
|
Size Change: +445 B (0%) Total Size: 815 kB
ℹ️ View Unchanged
|
gigitux
force-pushed
the
add/4965-product-title
branch
from
00b3803 to
5e477db
Compare
gigitux
force-pushed
the
add/4965-product-title
branch
from
5e477db to
517f80a
Compare
| } ), | ||
| }, | ||
| color: { | ||
| text: false, |
There was a problem hiding this comment.
Can you explain why text color is disabled here? Given that we allow changing the background, it makes more sense to me to allow changing text color too.
There was a problem hiding this comment.
By #4965, the color for the text should be disabled, but I agree with you.
I added the support for text color, too, now I will add a comment on the main issue.
| const getValueOrDefault = ( value: unknown ) => { | ||
| return isString( value ) && value.length > 0 ? value : '0'; | ||
| }; |
There was a problem hiding this comment.
I don't think we should return 0 as the default value here, it should be undefined instead. The element may have spacing rules defined in CSS, using 0 will override its styling.
| return { | ||
| ...acc, | ||
| [ key ]: `${ getValueOrDefault( | ||
| spacingProperty.top | ||
| ) } ${ getValueOrDefault( | ||
| spacingProperty.right | ||
| ) } ${ getValueOrDefault( | ||
| spacingProperty.bottom | ||
| ) } ${ getValueOrDefault( spacingProperty.left ) }`, | ||
| }; |
There was a problem hiding this comment.
Same as above, we should only use shorthand when all four sides are set, otherwise, we should use single side rules.
There was a problem hiding this comment.
Thanks for this! I updated the function!
Mmm, it seems that it is a GB problem. What version are you using? |
@gigitux I used the latest |
Mmm, it is a very strange problem. I updated the PR with your feedback. I also modified the CSS, because before I hadn't edited the save function to be compliant with apiVersion 2: now everything should be ok 👍 |
@gigitux Which WP version are you using? |
WordPress 5.9 - beta 4. What about you? |
There was a problem hiding this comment.
@gigitux With a fresh installation, the global style works as expected now. I left some comments about the utilities, please take a look.
| ...getSpacingStyleInline( key, { | ||
| top: getValueOrDefault( spacingProperty.top ), | ||
| right: getValueOrDefault( spacingProperty.right ), | ||
| bottom: getValueOrDefault( spacingProperty.bottom ), | ||
| left: getValueOrDefault( spacingProperty.left ), | ||
| } ), |
There was a problem hiding this comment.
| ...getSpacingStyleInline( key, { | |
| top: getValueOrDefault( spacingProperty.top ), | |
| right: getValueOrDefault( spacingProperty.right ), | |
| bottom: getValueOrDefault( spacingProperty.bottom ), | |
| left: getValueOrDefault( spacingProperty.left ), | |
| } ), | |
| ...getSpacingStyleInline( key, spacingProperty ), |
There was a problem hiding this comment.
I think we can pass spacingProperty as the second argument then handle sides there instead. It makes the callback look cleaner.
There was a problem hiding this comment.
I think we can pass spacingProperty as the second argument then handle sides there instead. It makes the callback look cleaner.
It can be an idea, but to be honest I prefer to create a more strict typed code.
With this code (so without type), we lose some information about how the function getSpacingStyleInline works.
I mean without an object with top, right, bottom, and left key, it doesn't make sense invoke getSpacingStyleInlinefunction. With the typeRecord<string, unknown>` we lose this information and the developer is forced to read the whole function to understand what kind of data to pass.
But this is just my opinion, if you think that it is better to change it, I will do it :D
There was a problem hiding this comment.
@gigitux With your example, I don't see the need for getSpacingStyleInline, we can add the inline style directly like this. Note that we can use undefined properties just fine because they will get omitted during render.
if ( keys.includes( key ) ) {
return {
...acc,
[ `${ key }Left` ]: spacingProperty.left,
[ `${ key }Right` ]: spacingProperty.right,
[ `${ key }Top` ]: spacingProperty.top,
[ `${ key }Bottom` ]: spacingProperty.bottom,
};
}
| ...( isString( values.top ) && { [ `${ key }Top` ]: values.top } ), | ||
| ...( isString( values.right ) && { | ||
| [ `${ key }Right` ]: values.right, | ||
| } ), | ||
| ...( isString( values.bottom ) && { | ||
| [ `${ key }Bottom` ]: values.bottom, | ||
| } ), | ||
| ...( isString( values.left ) && { [ `${ key }Left` ]: values.left } ), |
There was a problem hiding this comment.
Can we use key: value here then remove the null properties? We can even return style object with null properties because they will be omitted during render. Using spread here is quite hard to read IMO.
|
@gigitux Looking at https://github.com/WordPress/gutenberg/blob/0666dee9235aedd3d1fa51c733d0c1e146227452/packages/block-library/src/button/edit.js#L26, I'm wondering if we can reuse the spacing handler in Gutenberg instead of creating our own utilities? Like what we do with Edit: By the way, we're creating custom hooks, so |
…products-block into add/4965-product-title
There was a problem hiding this comment.
@gigitux Great work on this PR 👏🏽 !
Update: Unfortunately the API __experimentalGetSpacingClassesAndStyles is not available on WP 5.8. What should we do? Should we maintain the previous code for WordPress 5.8 and use the new hook for WordPress 5.9?
As long as our custom hook is feature-gated and no fatal error throws on WP 5.8, I think we can just focus on 5.9, it's not worth the effort to support 5.8 given that most of the global style functionalities are available from 5.9. cc @Aljullu for more insights : ).
Mmm, we have got a fatal error on WP 5.8 (without GB) The error: This bug is caused by |
Agree with this. 🙂
Would it be possible to check if Then, we might probably need to do the same check here, so controls don't appear in the editor if the function doesn't exist. |
Yes! Sure! I will add this check! Thanks for the feedback! |
gigitux
force-pushed
the
add/4965-product-title
branch
3 times, most recently
from
fa8c990 to
18642e6
Compare
|
@Aljullu @dinhtungdu I just added a utility function 18642e6 to check if there is support for the spacing style support. Let me know, what do you think! |
gigitux
force-pushed
the
add/4965-product-title
branch
from
18642e6 to
a17ca36
Compare
| ...( isFeaturePluginBuild() && { | ||
| typography: { | ||
| fontSize: true, | ||
| lineHeight: true, | ||
| __experimentalFontWeight: true, | ||
| __experimentalTextTransform: true, | ||
| __experimentalFontFamily: true, | ||
| }, | ||
| } ), | ||
| ...( isFeaturePluginBuild() && { | ||
| color: { | ||
| text: true, | ||
| background: true, | ||
| link: false, | ||
| gradients: true, | ||
| __experimentalSkipSerialization: true, | ||
| }, | ||
| } ), | ||
| ...( isFeaturePluginBuild() && | ||
| hasSpacingStyleSupport() && { | ||
| spacing: { | ||
| margin: true, | ||
| __experimentalSkipSerialization: true, | ||
| }, | ||
| typography: { | ||
| fontSize: true, | ||
| }, | ||
| } | ||
| : sharedConfig.supports, | ||
| } ), |
There was a problem hiding this comment.
Did html: false get removed on purpose?
By the way, this part can be rewritten to:
...sharedConfig.supports,
...( isFeaturePluginBuild() && {
typography: {
fontSize: true,
lineHeight: true,
__experimentalFontWeight: true,
__experimentalTextTransform: true,
__experimentalFontFamily: true,
},
color: {
text: true,
background: true,
link: false,
gradients: true,
__experimentalSkipSerialization: true,
},
...( hasSpacingStyleSupport() && {
spacing: {
margin: true,
__experimentalSkipSerialization: true,
},
} ),
} ),
There was a problem hiding this comment.
@gigitux In my test with the latest trunk of Gutenberg, WP 5.9 beta 4, and WC 6.1.0-dev, the text color works for block style but doesn't work for global style.
Screen.Recording.2022-01-11.at.22.45.42.mov
Thanks for your feedback. If you save the custom color and you check the frontend, you will be able to see the custom color. The problem, in this case, is that Gutenberg injects custom style for all anchor elements on global style We are not sure that this element is an |
…products-block into add/4965-product-title
@gigitux I opened #5548 to fix this issue by overriding the style for the anchor of the title in the editor. Please take a look!
I think the title should use the same color setting regardless it's a link or not. Enable |
Thanks for the help! I didn't know that the Let me know if I can merge this PR! |
This PR adds support for global style for the
Product Titleblock.Now, the user can edit the style for:
Fixes #4965
Screenshots
without global style
with global style
Testing
Manual Testing
Check out this branch:
Gutenbergplugin on WordPress 5.9 site.Twenty Twenty-Twotheme.Single Product block(this block containsProduct Title Block) to a post.Product Title block.Resetbutton from the different sections.Stylesicon on the right-top corner.Product Title blockis shown under theBlockssection. Personalize again the block.Changelog