Centralize style & support mappings for blocks #25185
Conversation
oandregal
requested review from
ellatrix,
spacedmonkey,
TimothyBJacobs and
youknowriad
as code owners
| @@ -385,17 +409,9 @@ function gutenberg_experimental_global_styles_get_block_data() { | |||
| * @return array Containing a set of css rules. | |||
| */ | |||
| function gutenberg_experimental_global_styles_flatten_styles_tree( $styles ) { | |||
| $mappings = array( | |||
There was a problem hiding this comment.
No changes here, only extracted to a separate function.
| '--wp--style--color--link' => array( '__experimentalColor', 'linkColor' ), | ||
| 'background-color' => array( '__experimentalColor' ), |
There was a problem hiding this comment.
These keys were renamed to use the same conventions as client-side (camelCase over kebab-case), making everything more coherent and less computation-demanding.
| ! supports.includes( TEXT_COLOR ) && | ||
| ! supports.includes( BACKGROUND_COLOR ) && | ||
| ! supports.includes( GRADIENT_COLOR ) && | ||
| ! supports.includes( 'color' ) && |
There was a problem hiding this comment.
These keys are the ones that were updated in the server (gutenberg_experimental_global_styles_get_support_keys changes). This way we use the common convention for the styles properties everywhere.
|
Size Change: -240 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
| settings.push( { | ||
| colorValue: getProperty( name, [ 'color', 'text' ] ), | ||
| colorValue: getProperty( name, STYLE_PROPERTY.color ), |
There was a problem hiding this comment.
Personally, this is the major advantage of centralization, and what pushed me to create this PR. Controls don't need to know what's the path of the property they want to access.
There was a problem hiding this comment.
If the idea is to let control not need to know the shape of the property they want to access, I guess getProperty could receive the property instead of the path, and be responsible for resolving the property.
Of course, we may need to change something that does not contain a property e.g: a preset. So I guess in the future we may have a get and setter that receives the property directly and one that receives the paths.
But for now, what we have is ok.
packages/blocks/src/api/constants.js
Outdated
|
|
||
| export const LINK_COLOR = '--wp--style--color--link'; | ||
|
|
||
| export const STYLE_PROPERTY = { |
There was a problem hiding this comment.
I think this is a good place for this kind of info, as it speaks about how the block attributes are structured. However, I don't feel super strongly about this. If this is a blocker I'm fine moving it to any other place.
packages/blocks/src/api/index.js
Outdated
| LINK_COLOR, | ||
| PADDING_SUPPORT_KEY, | ||
| STYLE_PROPERTY, | ||
| } from './constants'; |
There was a problem hiding this comment.
I'm concerned about this personally, it makes it an API and I don't really know whether these keys will remain as is, be combined... Why do we need this to be an API? (specially the _key ones)
There was a problem hiding this comment.
mmm, my rationale was that the support keys are actually part of the API. This is why: if we had specific accessors (hasLineHeightSupport, hasFontSizeSupport, etc), consumers wouldn't need anything else to use them; however, by having general accessors (such as hasBlockSupport( feature )) we also need to provide the consumer with a list of the available keys they can use. So far, they inspect the block.json for this. I'd think providing constants for them would make it easier for us to modify their names later on if we need to. Said this, I realize we don't expose any other feature, so I won't hold this opinion too strongly and would be fine to move these back to block-editor, if that's preferred.
For the style keys, I agree with Jorge that we need a single source of truth, as they're both used in block-editor and edit-site. Exporting them here makes more sense from my POV.
There was a problem hiding this comment.
in which situation we need to access these keys outside the hooks themselves which are part of block-editor package?
There was a problem hiding this comment.
I reverted the support keys to block-editor. I still think that we should offer a feature list as I mentioned, but I realized that it doesn't seem a good idea to start with the experimental keys.
| settings.push( { | ||
| colorValue: getProperty( name, [ 'color', 'text' ] ), | ||
| colorValue: getProperty( name, STYLE_PROPERTY.color ), |
There was a problem hiding this comment.
If the idea is to let control not need to know the shape of the property they want to access, I guess getProperty could receive the property instead of the path, and be responsible for resolving the property.
Of course, we may need to change something that does not contain a property e.g: a preset. So I guess in the future we may have a get and setter that receives the property directly and one that receives the paths.
But for now, what we have is ok.
packages/blocks/src/api/constants.js
Outdated
|
|
||
| export const LINK_COLOR = '--wp--style--color--link'; | ||
|
|
||
| export const STYLE_PROPERTY = { |
There was a problem hiding this comment.
I guess we should prefix the constant with experimental.
|
@jorgefilipecosta @youknowriad thanks for the feedback! I'm reasonably happy with this clean-up. It helped to clarify the global styles provider API and consolidate the data structure we use in both places. 💪 |
| getProperty: ( context, path ) => | ||
| get( userStyles?.[ context ]?.styles, path ), | ||
| setProperty: ( context, path, newValue ) => { | ||
| getStyleProperty: ( context, propertyName ) => |
There was a problem hiding this comment.
I think we probably also need the old setters, otherwise for example to configure presets using the UI how are we going to do it? We also need setters and getters in global styles that don't map to a CSS property.
There was a problem hiding this comment.
So far, I don't believe we need anything else. Note that with the old format you could only change properties within styles anyway.
My understanding is that we'll need to create new presets for users, but I'm not sure how that's going to work because we're going to need access to both theme and user presets at the same time (unlike the current styles or features where we merge both). Perhaps in that case we could have a getPreset( global, color, user )/setPreset( global, color, newValue )? I guess we can cross that bridge when we get to it.
There was a problem hiding this comment.
Yes true we could only change style paths anyway. So I guess the change is positive :)
Fixes #25051
Alternative to #25145
This Pull Request seeks to clarify what info we take from which source. Specifically, we extract info from the block (support keys and the style attribute) as well as from theme.json (style properties that follow the same paths as the style attribute).