Add: API to allow blocks to access global styles. #34178
Conversation
jorgefilipecosta
added
[Feature] Block API
jorgefilipecosta
requested review from
ellatrix,
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: -35 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/api-to-expose-global-styles-to-blocks
branch
from
08e96ee to
5cba6cb
Compare
lib/global-styles.php
Outdated
| if ( 'mobile' === $context ) { | ||
| $settings['__experimentalStyles'] = $consolidated->get_raw_data()['styles']; | ||
| } | ||
| $settings['__experimentalStyles'] = $consolidated->get_raw_data()['styles']; |
There was a problem hiding this comment.
Asking for my own benefit here :)
I was wondering, is there a use case for where we'd want to access __experimentalStyles in the site editor, or in other words, do any templates in the site editor need to know about user styles?
Or would it be okay to limit adding this object to editor/widget contexts?
There was a problem hiding this comment.
Can the __experimentalStyles data be limited to the mobile and post-editor contexts? Perhaps also in the widgets editor if it uses the existing block-editor hooks (such as line-height), but I'm not sure how that works. The site-editor already has access to this data, so it wouldn't need it.
There was a problem hiding this comment.
I was wondering, is there a use case for where we'd want to access __experimentalStyles in the site editor
Yes on the site editor blocks still need to access the __experimentalStyles flag, as the hooks like line-height also run there. But as @oandregal said on the site editor we already have this information so we don't need to pass it again, I'm going to update the code. The setting will be available anyway on the block editor it will just be dynamically computed.
| @@ -70,7 +74,7 @@ export default function LineHeightControl( { value: lineHeight, onChange } ) { | |||
| onKeyDown={ handleOnKeyDown } | |||
| onChange={ handleOnChange } | |||
| label={ __( 'Line height' ) } | |||
| placeholder={ BASE_DEFAULT_VALUE } | |||
| placeholder={ placeholder } | |||
There was a problem hiding this comment.
I like this way of doing things. ❤️ Having the default value as the placeholder works I think as I can still set numerical values to 0 for example, and the native appearance (greyed out) indicates that it's a default value.
Just noting a couple of things down, all of which are probably best for follow ups if this gets off the ground.
- We'll have to parse unit values (
px,vhetc) from the default values and reset them in the controls where units change - I tested a few controls, and I think there will be some work to ensure we can pass default values as placeholders to the rest of our controls hooks. For example, box-control will require a tweak to handle placeholders for top/right/bottom/left inputs. This is for spacing controls such as margin and padding. But anything importing
<UnitControl />directly such as border-width should just work 👍
There was a problem hiding this comment.
Good point regarding the units @ramonjd. I guess we can deal with units a follow-up to avoid growing this PR.
|
Thank you for working on this feature, and I learned a lot by reading the code. It came right as we were discussing whether it would be good idea too (which I think it is!). I tested according to test instructions, plus added defaults for various block controls hooks and all seem to work as expected, with css vars being parsed correctly. I'm happy to help look at ensuring placeholders work for our controls as well if required. Looking forward to the updates on this one! 🙇 |
|
While testing this I've missed the ability to reset the user value to the theme value in the line-height control so filed at issue for it at #34260 |
| const setting = useSelect( | ||
| ( select ) => { | ||
| const settings = select( blockEditorStore ).getSettings(); | ||
| const settingsForBlock = get( settings, [ |
There was a problem hiding this comment.
mmm, was looking at what sort of data this was using. Some braindump as to how / why inheritance would increase the complexity to take into account:
- Some CSS properties are inherited, some other don't.
- A block can be top-level but also can be nested within other blocks: to determine the value we'd need to traverse up the tree and consolidate all values of the chain.
- CSS custom properties: if the theme uses them, this breaks the CSS inheritance model for the property.
I agree this PR should be limited to only take data from the specific block.
| if ( ! variable || ! isString( variable ) ) { | ||
| return variable; | ||
| } | ||
| const INTERNAL_REFERENCE_PREFIX = 'var:'; |
There was a problem hiding this comment.
What do you think of using explicit names to document where this data comes from? I'm thinking of something along these lines:
const USER_VALUE_PREFIX = 'var:';
const THEME_VALUE_PREFIX = 'var(--wp--';
const THEME_VALUE_SUFFIX = ')';There was a problem hiding this comment.
What do you think of substituting every use of context by blockName? We no longer use context anywhere, so I'd think it's more familiar for readers to follow along with blockName (our future selves included 😅 ).
| return getResolvedStyleVariable( features, blockName, result ); | ||
| } | ||
|
|
||
| export function getResolvedStyleVariable( features, context, variable ) { |
There was a problem hiding this comment.
It wasn't clear to me what this function was for based on its name. What alternatives do we have to improve this? What about something along the lines of getValueFromStyle? It sits well with the other helper functions getValueFromPresetVariable, getValueFromCustomVariable
There was a problem hiding this comment.
I've noticed that before moving it here it was called getValueFromVariable, that name also works for me.
| features, | ||
| context, | ||
| presetPath, | ||
| presetProperty, |
There was a problem hiding this comment.
How about naming these two presetPropertyName, presetPropertyValue to communicate what they are?
There was a problem hiding this comment.
It seems the role of findInPresetsBy is to find the preset object whose presetPropertyName matches the presetPropertyValue among all the available.
The complexity here is that it needs to take into account that 1) there may be a block and a global preset and 2) any of those may have core, theme, and user presets. This is the full list of candidate presets available, sorted by priority:
- block preset from user
- block preset from theme
- block preset from core
- global preset from user
- global preset from theme
- global preset from core
My understanding is that we should return the first candidate whose presetPropertyName matches presetPropertyValue. Wouldn't that be enough? I've looked at the PR that introduced this function #33149 and I don't understand why we need the recursion when the property to look up is not the slug.
There was a problem hiding this comment.
I've updated in trunk the implementation of this function to this algorithm:
- Create a list of the available presets, sorted by priority.
- Return the first preset.
And it works as described at #33149
Code, for reference:
function findInPresetsBy(
styles,
blockName,
presetPath,
presetPropertyName,
presetPropertyValue
) {
const candidates = [];
const origins = [ 'user', 'theme', 'user' ];
const blockPresets = get( styles, [
'settings',
'blocks',
blockName,
...presetPath,
] );
origins.forEach( ( origin ) => {
if ( blockPresets?.[ origin ] ) {
blockPresets[ origin ].forEach( ( preset ) => {
if ( preset[ presetPropertyName ] === presetPropertyValue ) {
candidates.push( preset );
}
} );
}
} );
const globalPresets = get( styles, [ 'settings', ...presetPath ] );
origins.forEach( ( origin ) => {
if ( globalPresets?.[ origin ] ) {
globalPresets[ origin ].forEach( ( preset ) => {
if ( preset[ presetPropertyName ] === presetPropertyValue ) {
candidates.push( preset );
}
} );
}
} );
return candidates.length >= 1 ? candidates[ 0 ] : undefined;
}There was a problem hiding this comment.
Hi @oandregal,
My understanding is that we should return the first candidate whose presetPropertyName matches presetPropertyValue. Wouldn't that be enough?
I don't think it is enough.
Imagining user theme.json defines presets:
{
color: "#000"
slug: "black",
name: "Black"
}
And theme defines:
{
color: "#001"
slug: "black",
name: "Black"
}
If we search the color property value of "#1" without the recursion we would return the theme preset, but we should not return it because the variable was overwritten and now there is no variable with color "#1".
The recursion makes sure that when we find a preset, there is no other higher priority preset with the same slug and different property because if there is the preset should be ignored.
There was a problem hiding this comment.
Nice one 👍 I've pushed some tests in c78a322 to make sure this behavior is documented and retained.
packages/edit-site/src/components/editor/global-styles-provider.js
Outdated
Show resolved
Hide resolved
| return variable; | ||
| } | ||
|
|
||
| export function getPresetVariableRepresentingAValue( |
There was a problem hiding this comment.
More naming nitpicks 😅 Before moving it here, it was getPresetVariable. If we want to give it more context perhaps it can be getPresetVariableFromValue, which is also symmetrical to the other exported function in this file.
Co-authored-by: André <583546+oandregal@users.noreply.github.com>
…r.js Co-authored-by: André <583546+oandregal@users.noreply.github.com>
jorgefilipecosta
force-pushed
the
add/api-to-expose-global-styles-to-blocks
branch
from
7cb9f0c to
6773622
Compare
|
Hi @ramonjd, @oandregal, thank you for the reviews! Your feedback was applied/answred. Let me know if there are other possible improvements or if this is ready to ship. |
|
What do you think of this PR which is attempting to do something similar? #41696 |
@jorgefilipecosta, is this PR something that is still necessary? |
We still need this PR or something like it in order to access global style values for specific blocks in the post editor. An important use case for this is reflecting current state in block controls. If a block type has padding set globally, and also has a padding control that allows setting custom values for specific blocks, it would make sense for that padding control to be set initially to whatever the global value is. This has been discussed a fair bit in #42173. #41696 doesn't solve this problem; it only allows us to set global values to other global values in theme.json. |
Probably some issue during a rebase. If we want to review this PR, I can fix this.
That PR makes sense it is something that affects just the engine. In this one we try to make the block inspector reflect what styles are applied to a block which right now is impossible. A block may have a very huge font size applied from global styles the user wants to decrease it a little on a specific case, but does not even knows what is the actual font size to begin with.
Exactly I still think reflecting the styles on the block inspector is something we want to do. |
Thanks @jorgefilipecosta, I'm happy to help review if you'd like to resume work on this! |
|
This branch is full of conflicts, so I'll get started copying over the changes to a new branch as that'll be easier 😅 |
|
Closing this PR as a new branch will be started. |
This PR implements an experimental API that allows a block to know if a global style is being applied to it:
E.g:
This mechanism takes into account CSS variables. If a theme uses a custom CSS variable or a preset CSS variable the value of the variable is returned.
This mechanism is useful for the mobile APP as we can more easily apply the styles to a block that comes from theme.json cc: @hypest.
It is also useful for cases where when applying a local style we need context if there are global styles or not. E.g: when applying a border width we need to know if a global border style exists if not we apply a solid border style so the border width makes sense if a style already exists we leave what the theme defined. cc: @aaronrobertshaw, @mtias.
It will also allow the UI to properly reflect the styles of a block. Right now, I already did an improvement for testing purposes. The Line height placeholder reflects the true line-height value applied using theme.json. So if theme.json applies a 2 line-height we show 2 instead of the default 1.5.
This PR is still very simple and does not implement inheritance. Inheritance is complex and I guess we should see how far we go without it.
How has this been tested?
I verified on the site editor that I can still apply preset styles e.g: colors. And when I change the preset the styles relying on that preset also change. (existing edit site variable resolution still works).
I added the following data as theme.json styles:
I added a paragraph on the block editor and verified the paragraph line-height appeared with two as a placeholder.
I added the following data as theme.json styles:
I added a paragraph on the block editor and verified the paragraph line-height appeared with two as a placeholder.
I added the following custom variable to the settings of theme.json:
I added the following data as theme.json styles:
I added a paragraph on the block editor and verified the paragraph line height appeared with 3 as a placeholder.