Use same mappings for the style properties everywhere #24320
Conversation
oandregal
requested review from
ajitbohra,
ellatrix,
Soean,
spacedmonkey,
talldan,
TimothyBJacobs and
youknowriad
as code owners
lib/global-styles.php
Outdated
| 'line-height' => array( '__experimentalLineHeight' ), | ||
| $mappings = array_merge( | ||
| gutenberg_experimental_global_styles_get_mappings(), | ||
| // TODO: remove from here and access directly from lib/blocks.php |
There was a problem hiding this comment.
Note that the block-align property was added 0d08cda I think we can fix this in a subsequent PR: it should be removed from here and lib/blocks.php refactored to use the block registry instead of global-styles.php
|
Size Change: +50 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
| "linkColor": true | ||
| "__experimentalStyles": { | ||
| "color": { | ||
| "background": true, |
There was a problem hiding this comment.
In the client, the background and text keys are not used, only support for __experimentalStyles.color is checked. Although we could remove them from here and make the server verification do the same, this will make the support keys to have different shape than the block attributes and theme.json. I lean towards making the client be more strict about these checks in a subsequent PR and actually verify that background and text are true.
Thoughts?
There was a problem hiding this comment.
See #24298 (comment) as an example where conflating background and text is forcing us to do things that we may not want to do.
| "__experimentalColor": { | ||
| "gradients": true, | ||
| "linkColor": true | ||
| "__experimentalStyles": { |
There was a problem hiding this comment.
In this config, we'd want to:
- Define whether something is enabled or not
- Potentially define whether it uses presets
- Potentially enable/disable custom values
- Potentially choose whether to show selectors in inspector/toolbar
- Potentially other configs I'm not thinking of right now.
Do you think this new config scales properly to all these kind of configs?
I do wonder whether this new config is more tailored toward applying styles and not configuring available controls for a block. WDYT?
There was a problem hiding this comment.
Right. I'm thinking about separate keys per each thing. This is matching the styles subtree of the theme.json, the same way __experimentalFeatures in block.json matches the features subtree of the theme.json (which, atm, is only implemented by the paragraph block, though). If we wanted to add presets or UI configurations, we can do as separate keys within supports as well.
A couple of questions that I have are:
-
Is this enough granularity for blocks that define many contexts (ex: heading block => core/heading/h1, core/heading/h2)? Both the current mechanism and this reorganization make all headings share the same features they support.
-
What's the relation between
block.jsonand thelib/experimental-theme.jsonthat comes with core? Isblock.jsonfor enabling/disabling things and theexperimental-theme.jsonfor setting the values? Does it make sense to conflate the two? If we conflate them, the first question takes more importance as, for values, we do need different things per context.
All in all, given that this is a mere reorganization and that it doesn't introduce any block deprecations, etc, I think it's fine to go with this. It brings some needed clarity and alignment between block.json and theme.json.
There was a problem hiding this comment.
Even if theme.json and block.json work in coordination, I'm not sure we should map one over the other tbh.
If I'm a block author's, I want to enable colors and configure how the controls show, I don't really care about applying styles aside maybe a default value, and it's weird tbh if these are in separate keys.
If I'm a theme author, I want to define a color palette for the editor, potentially define separate palettes for specific blocks and I want to enable/disable custom colors or palettes. I also want to give default values for colors. In this case, separate keys may be good.
It seems like two different ways of thinking about these configs.
I do wonder if it's too early to be aligning configs and thinking about what keys to use... Maybe we should focus now to adding more features and configs to all blocks and themes and once we have all of these under experimental flags or keys, we can decide how to align with a clear view.
There was a problem hiding this comment.
I think I may have introduced some confusion. What we currently have is:
-
If core blocks want to
- Declare support for a style property => use the block.json
- Set a default value for a style property => use core's lib/experimental-theme.json
-
Theme authors want to set palettes, styles, etc => use the theme's theme.json
-
Plugin authors want to
- Declare support for a style property => use the block.json
- Set a default value for a style property => we don't have an API for this.
I was referring to conflating 1.1 and 1.2. I think I may agree with you that probably is a good idea to keep them separated. However, we'd then need to find a solution for 3.2. I don't have an answer for this yet, just sharing early thoughts to get the conversation started.
As for the tree organization, I'm not too opinionated, as long as we use the same shape everywhere (block attributes, theme.json spec, and block supports). At the moment, block supports is the only place that uses a different shape. I guess we can fix what we have now for clarity and iterate later when we gain a better knowledge about this?
There was a problem hiding this comment.
At the moment, block supports is the only place that uses a different shape. I guess we can fix what we have now for clarity and iterate later when we gain a better knowledge about this?
I was personally arguing that given the fact that block authors and theme authors think differently about these features, it make sense to have a different shape.
A block author think about the color support for his block wholistically and not separately (enabling, configuring presets, and potentially a default value)
which means: (block.json)
color: {
enabled: true,
gradients: true,
ui: ['toolbar']
palette: {} // Can define a palette that overrides the default editor palette
default: "somevalue" // may or may not be needed,
allowCustomColor: true,
allowCustomGradients: true,
}
but for a theme author (theme.json), he thinks differently, he thinks first about applying config globally and then go specific per block if needed. We also don't want to control the UI for blocks here. so for me:
presets: {
global: {
color: {}
},
core/some-block: {
color: {}
},
},
features: {
global: {
allowCustomColors: true,
},
core/some-block: {
allowCustomColors: false,
},
},
styles: {
global: {
color: 'black',
},
core/some-block: {
color: 'red',
},
}
also from a theme's author perspective, presets and features might be the same thing (just controlling the editor) so I wonder if they should be separate but I guess that's a separate discussion.
There was a problem hiding this comment.
OK, let's put this PR in the backburner.
After some PRs I'm working on are merged (example: #24416) I was thinking of starting to port some of the theme_supports features to this new system. That may be a good opportunity to re-evaluate.
There was a problem hiding this comment.
Riad, in case you missed it, more food for thought that's related to what we discussed here: #22887
Follow-up to this conversation #24298 (comment)
The way we declared the style properties within block.json's supports key was inconsistent with how the properties were stored in the theme.json and the block attributes, which followed the theme.json specification.