-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add settings
hook
#40547
Add settings
hook
#40547
Conversation
Size Change: +232 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
@@ -18,6 +18,7 @@ | |||
} | |||
}, | |||
"supports": { | |||
"settings": 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.
The style
attribute we already enable by the other block suports (color, typography, etc). Although unlikely, I wonder if there's a block that doesn't want the styles but does want the settings. This is why I used settings
for the hook name instead of section
.
If we want to go with section
instead, I'd argue that we'd also want to make it register styles
if they are not defined yet.
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.
No real formed opinion on my part yet, but simply settings
could make sense.
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 "section" shouldn't be a thing in our code base, it should be a consequence of several enabled features/context. So I think "settings" is the right call. The only thing is whether we start stable or give it some time as "experimental".
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 would lean to making it experimental for some time.
@@ -18,6 +18,7 @@ | |||
} | |||
}, | |||
"supports": { | |||
"settings": 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.
No real formed opinion on my part yet, but simply settings
could make sense.
|
||
// 2. Fall back to the settings from the block editor store (__experimentalFeatures). | ||
const settings = select( blockEditorStore ).getSettings(); | ||
if ( result === undefined ) { |
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.
The only change in this section is to rename __experimentalFeaturesResult
to result
.
|
||
if ( experimentalFeaturesResult !== undefined ) { | ||
// 1. Take settings from the block instance or its ancestors. |
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.
The algorith for useSetting
is:
- lookup for settings defined in the block instance or any of its ancestors => this is the only behavioral change introduced by this PR.
- lookup for settings defined in the block editor store
- fallback for deprecated settings
- fallback for dropcap
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 global styles, I added a custom palette to the "paragraph" block and then selected a paragraph block from your example above. I wasn't sure what should show up:
- The palette of the parent "group"
- The palette of the "paragraph" block from Global styles.
I think that's the remaining point we should figure out in the algorithm, we should probably just pick one approach and stick with it.
My first thought is that picking the "paragraph" settings from the global styles might make more sense here. (Right now it picks the parent's palette)
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 shared some longer thoughts for this at #40318 (comment)
I've looked at use cases for sections, aka why do we want to do this? I understand we want to enable doing things such as "set colors for this whole section" or "disable custom font sizes for the whole section". These are easier to do with a "by source" algorithm (first considers block instance and its ancestors, then theme.json data). Specially when/if we expose this to the users via an UI.
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 it's not that simple. The current algorithm is different that "style" and it's not clear why we'd want "settings" to work differently.
If you apply a text color to a paragraph block, it won't be using the color from the parent "style", it will continue to use the "global styles" one.
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.
Yeah, but styles and settings behave different in theme.json
as well: styles aren't inherited and settings are .
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.
The CSS model allows us to choose between still keeping the table palette or overwriting anyway. On the alternative model, I don't really have a choice.
You do! You just set the settings for the specific block types you're interested in (and the other ones -the table- will fall back to the store settings).
I think the crux of the conversation is: what are the common uses case we should optimize for? If this one is the typical thing patterns want to do and the table example you shared is the uncommon one, the model this PR follows works best.
what is the solution we are going to have to on the example I gave
I've created a pen that demonstrates the challenge you shared and how we can address it https://codepen.io/oandregal/pen/eYyqxZO?editors=1100
Let me unwrap it here as well. Given a section like this one (note the paragraph has a red color applied by the user):
<div class="wp-section-UUID">
<p class="has-red-color"></p>
</div>
If the theme has added some paragraph-specific settings, as in:
{
"settings": {
"color": {
"palette": [
{ "slug": "red", "color": "red-theme-global" }
]
},
"blocks": {
"core/paragraph": {
"color": {
"palette": [
{ "slug": "red", "color": "red-theme-block" }
]
}
}
}
}
}
And the section has this:
{
"settings": {
"color": {
"palette": [
{ "slug": "red", "color": "red-section-global" }
]
}
}
}
How do we make it so the color rendered for the paragraph is red-section-global
?
/*
* THIS IS THE CSS GENERATED FOR THE `theme.json`
*/
body {
--wp--preset--color--red: "red-theme-global";
}
p {
--wp--preset--color--red: "red-theme-block";
}
.has-red-color {
color: var(--wp--preset--color--red) !important;
}
/*
* THIS IS THE CSS GENERATED FOR THE SECTION.
*/
.wp-section-UUID,
.wp-section-UUID [class*="wp-block"],
/*
* We also need fallbacks for any block that don't use the .wp-block-* single selector
* but instead use __experimentalSelector to define their own. These include
* paragraph, list (li, ul), heading (h1-h6), and table so far.
*/
.wp-section-UUID p,
.wp-section-UUID li,
.wp-section-UUID ul,
.wp-section-UUID h1,
.wp-section-UUID h2,
.wp-section-UUID h3,
.wp-section-UUID h4,
.wp-section-UUID h5,
.wp-section-UUID h6,
.wp-section-UUID .wp-block-table > table {
--wp--preset--color--red: "red-section-global";
}
Does this address your concerns?
Note that this PR doesn't generate the styles for the presets declared in a section. That's something that has a complexity of its own and I thought that to be a follow-up (things like generating the preset classes or not depending on whether the preset is already defined in theme.json
, etc).
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.
Ongoing draft PR for rendering the settings at #40683
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.
You do! You just set the settings for the specific block types you're interested in (and the other ones -the table- will fall back to the store settings).
Your section may not be aware of the blocks it is interested in, it may have a group where the user can nest anything inside.
I think the crux of the conversation is: what are the common uses case we should optimize for? If #40547 (comment) is the typical thing patterns want to do and the table example you shared is the uncommon one, the model this PR follows works best.
I agree we should optimize for the most common cause. The thing is we don't know the most common case yet. I think a theme setting block specific settings is something rare and very specific that should be respected even if that block is used inside a section setting something global for that setting. Your expectation is that something global at the section should take priority above a theme-specific setting. I guess we need to choose and iterated and if we receive feedback or new information later change the direction if that's a possibility.
How do we make it so the color rendered for the paragraph is red-section-global?
.wp-section-UUID,
.wp-section-UUID [class*="wp-block"], /* Any child block */
/*
* We also need to cover for child blocks that don't use the .wp-block-* class:
* paragraph, list (ul, or), and heading (h1-h6).
*/
.wp-section-UUID p,
.wp-section-UUID li,
.wp-section-UUID ul,
.wp-section-UUID h1,
.wp-section-UUID h2,
.wp-section-UUID h3,
.wp-section-UUID h4,
.wp-section-UUID h5,
.wp-section-UUID h6,
The solution proposed makes things work but requires us to ship much more CSS code, for every color or font size, etc instead of just need to use just .wp-section-UUID, we need to use a much bigger selector per preset we have that means more bytes sent over the wire for every section that has some preset.
I'm not sure of the best approach either so I'm fine with giving a try to the solution we have here and seeing how it works in practice.
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.
Actually, there's a simpler CSS for the presets generated by the section if we embrace how theme.json
works:
/*
* CSS Custom Properties defined in the section
*/
.wp-section-UUID .has-red-color {
--wp--preset--color--red: "red-section-global";
}
/*
* CSS classes for presets defined in the section.
* Ideally, we should not enqueue the classes that are already enqueued by theme.json.
* Though we still need to check at runtime because a given theme may have them
* but a different theme may not.
*/
.wp-section-UUID .has-red-color { color: var(--wp--preset--color--red) !important; }
That's it (codepen).
I think we can decide which of the alternatives we want to pursue in the PR that implements 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.
Yes but if have a block specific color, on theme.json we generate:
p.has-red-color {
color: var(--wp--preset--color--paragraph-red)
}
So in each section we can not do just:
.wp-section-UUID .has-red-color {
--wp--preset--color--red: "red-section-global";
}
We also need to overwrite the block specific colors:
.wp-section-UUID p.has-red-color {
--wp--preset--color--red: "red-section-global";
}
0be0863
to
5b81fc8
Compare
@@ -102,7 +107,7 @@ const removeCustomPrefixes = ( path ) => { | |||
* ``` | |||
*/ | |||
export default function useSetting( path ) { |
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 wonder if we can unit test this hook to avoid regressions. Wrap it in a test component and mock the global styles input and see what you get in different situations.
settings: { | ||
type: 'object', | ||
}, | ||
}; |
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 works :) I wonder if this kind of built-in attributes we're adding (style, lock, settings) should at some point move to the registration function instead of using hooks. They're becoming so central to the block editor experience. (A discussion for later)
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's already a thing in WordPress core for the lock
attribute:
I fully agree we should follow with style
and className
which are essential for styling all blocks.
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's worth noting that a couple of blocks can't use style
and className
, as they lack the markup to apply them to, e.g. the Page Break and Shortcode blocks. Not sure if that matters here, but I figured I'd bring it up.
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.
Looking good overall 👏
Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>
5b81fc8
to
f52967d
Compare
I think we need to clarify another question: what should a section be able to do with the presets? Be able to set presets from any origin or only presets for the custom origin? This is, should the format for presets include the origin key ( {
"settings": {
"color": {
"palette": {
"theme": [],
"theme": [],
"custom": []
}
}
}
} or only the custom ones (no key and we match the presets defined in a section as {
"settings": {
"color": {
"palette": []
}
}
} I lean towards the first option (what this PR does) because it allows for greater control. |
I think we should go with any origin as this PR does. |
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 works well in my tests and I think it is ready to merge 👍 I think we can make the settings support experimental for some time but I don't have string feelings here as settings are something we really need.
@@ -18,6 +18,7 @@ | |||
} | |||
}, | |||
"supports": { | |||
"settings": 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.
I would lean to making it experimental for some time.
What?
This PR adds a new hook called
settings
which blocks can opt-in into and that allows them to have asettings
object like thetheme.json
has.Why?
We want to create the concept of a "section" that is able hold settings & styles (like
theme.json
does) but in block instances. See #40318 and #39281How?
This is only one part of getting settings to work in block instances. The other part is building an UI for them (get something like a global styles sidebar to show in the blocks that opt-into this hook).
settings
hook.useSetting
hook to lookup for settings in the block instance or its ancestors before looking into the block editor store.Testing Instructions
Questions
settings
hook #40547 (comment)settings
hook #40547 (comment)Follow-ups
See #40318