-
Notifications
You must be signed in to change notification settings - Fork 219
Stock Indicator block: Add support for global style #5525
Conversation
…mmerce-gutenberg-products-block into add/4965-stock-indicator
Stock indicator block: add support for global style
Size Change: +385 B (0%) Total Size: 817 kB
ℹ️ View Unchanged
|
…products-block into add/4965-stock-indicator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block and global styles were both working in my test. But I have some concerns about the color based on stock statuses. Please check the review comments for more detail.
/** | ||
* External dependencies | ||
*/ | ||
import { useBlockProps } from '@wordpress/block-editor'; | ||
import classnames from 'classnames'; | ||
|
||
type Props = { | ||
attributes: Record< string, unknown > & { | ||
className?: string; | ||
}; | ||
}; | ||
|
||
export const Save = ( { attributes }: Props ): JSX.Element => { | ||
return ( | ||
<div | ||
{ ...useBlockProps.save( { | ||
className: classnames( 'is-loading', attributes.className ), | ||
} ) } | ||
/> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is for the Sale badge block. Was it added by accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
&--in-stock { | ||
color: $in-stock-color; | ||
} | ||
&--out-of-stock { | ||
color: $no-stock-color; | ||
} | ||
&--low-stock, | ||
&--available-on-backorder { | ||
color: $low-stock-color; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are removing the feature that store owners actually need here. The current Supports API doesn't allow multiple colors, so I don't think it's a good idea to remove styles for stock statuses. Instead, we should accept the limitation of the Supports API. In the other words, ignoring the color, only add global style support for typography.
We can also create custom inspector settings to control colors for each state, and it will be local block style only, not global style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are removing the feature that store owners actually need here. The current Supports API doesn't allow multiple colors, so I don't think it's a good idea to remove styles for stock statuses.
Before proceeding, I asked the rest of the team. Please check this discussion (p1641461102003800-slack-C02FL3X7KR6), maybe we can continue the discussion there.
}, | ||
} ), | ||
typography: { | ||
fontSize: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me to support all typography properties for this block instead of only font size. At least, font-weight and text-transform should be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense to me. I tested the Post Categories block from Gutenberg and it offers many more typography options than what we offer in Stock indicator.
I guess the same could be said about the Product Category List, Product Summary and Product Tag List.
@vivialice what are your thoughts on this? Should we enable all typography properties in those blocks?
…products-block into add/4965-stock-indicator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to main issue #4965, we should enable only font-size, but yeah we can enable them easily.
I agree to follow the requirements. We can merge this PR as it is for now and add more properties later. By doing so, we don't block the release of the Global Style project.
Thanks for the review! |
This PR is blocked by #5515This PR adds support for global style for the
Summary Product block
.Now, the user can edit the style for:
#4965
Screenshots
without global style
with global style
Testing
Manual Testing
Check out this branch:
WordPress 5.9
, install and enable theGutenberg
plugin.Twenty Twenty-Two
theme.Single Product block
(this block containsStock Indicator block
) to a post.Stock Indicator block
.Reset
button from the different sections.Styles
icon on the right-top corner.Stock Indicator block
is shown under theBlocks
section. Personalize again the block.Changelog