-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use same mappings for the style properties everywhere #24320
Changes from all commits
ae4e783
7c1e46a
bd3cce8
9ad9900
9df7f24
96e4c33
258ebcd
71a5978
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,9 +14,13 @@ | |
], | ||
"html": false, | ||
"lightBlockWrapper": true, | ||
"__experimentalColor": { | ||
"gradients": true, | ||
"linkColor": true | ||
"__experimentalStyles": { | ||
"color": { | ||
"background": true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the client, the Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #24298 (comment) as an example where conflating |
||
"gradient": true, | ||
"link": true, | ||
"text": 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.
In this config, we'd want to:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thefeatures
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 withinsupports
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.json
and thelib/experimental-theme.json
that comes with core? Isblock.json
for enabling/disabling things and theexperimental-theme.json
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I may have introduced some confusion. What we currently have is:
If core blocks want to
Theme authors want to set palettes, styles, etc => use the theme's theme.json
Plugin authors want to
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Riad, in case you missed it, more food for thought that's related to what we discussed here: #22887