-
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
Migrate existing theme supports to configure the editor to theme.json configs #24761
Conversation
5d19198
to
442b296
Compare
lib/experimental-default-theme.json
Outdated
@@ -127,6 +127,9 @@ | |||
"features": { | |||
"typography": { | |||
"dropCap": false | |||
}, | |||
"colors": { | |||
"allowCustom": 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'm assuming this file is where we define if the feature is opt-in or opt-out. (Default value provided by Core).
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.
Also by reading this, I wonder if it makes more sense to avoid the separation between the top-level "presets" and "features" and just combine them because these are used in conjunction in general.
Components will do things like:
const colorsPreset = useEditorFeature( 'colors.preset' );
const allowCustomColors = useEditorFeature( 'colors.allowCustom' );
// Render palette with these options.
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.
Alternatively we can provide a separate hook useEditorPreset( 'colors' )
but it's a bit weird.
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 a bit weird to me to see the prefix "allow" given cases like "dropCap" when it is implied. I think just custom
would be fine.
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.
Removed the prefix.
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.
Updated #20588 by adding a task to look into either creating a usePreset('colors')
hook or integrate into the existing useEditorFeature('color.preset')
.
// Deprecated theme supports | ||
if ( null !== get_theme_support( 'disable-custom-colors' ) ) { | ||
$config['global']['features']['colors']['allowCustom'] = ! get_theme_support( 'disable-custom-colors' ); | ||
} |
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 is where we added the backward compatibility support for the deprecated theme support flags.
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.
If the global features config is empty the previous condition will return:
if (
empty( $config['global']['features'] ) ||
! is_array( $config['global']['features'] )
) {
return array();
}
And in that case, we will not pass the flag value. Should we handle this case?
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.
Should be fixed.
settings.disableCustomColors === undefined | ||
? undefined | ||
: ! settings.disableCustomColors, | ||
}; |
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 is where we say which existing block setting maps to which feature path (backward compatibility for block editor settings)
const blockSupportValue = getBlockSupport( blockName, path ); | ||
if ( blockSupportValue !== undefined ) { | ||
return blockSupportValue; | ||
} |
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 don't think this hook should concern itself with getBlockSupport
. getBlockSupport
is only used to say that a block support a given feature and should be used directly in the "hooks" and not in useEditorFeature
which responsibility is more related to block-editor settings 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.
This has introduced a regression by which the dropCap control is no longer shown for paragraph #24930
This is also related to this conversation. I think we should merge the fix above so it makes it to this week's release, and then we can figure out how to settle the info coming from block.json, core's theme.json, and theme's theme.json.
Size Change: +72 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
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.
Hi @youknowriad, I'm not being able to test this PR using theme.json.
In used the global styles test theme. I changed to the following theme.json:
{
"global": {
"styles": {
"color": {
"background": "var(--wp--preset--color--tertiary)",
"link": "hotpink"
}
},
"features": {
"typography": {
"dropCap": false
},
"colors": {
"allowCustom": false
}
}
},
"core/paragraph": {
"styles": {
"color": {
"text": "var(--wp--preset--color--quaternary)"
},
"typography": {
"fontSize": "calc(1px * var(--wp--preset--font-size--normal))",
"lineHeight": 1.4
}
}
}
}
And the colors continue to be enabled. I'm doing something wrong?
@jorgefilipecosta Good testing. It was due to two subtle issues that should be fixed with the last commit. |
ce8f026
to
cb501c1
Compare
If no blockers, I'm thinking of merging this in order to do the same for the remaining flags. |
Related #20588
The idea of this PR is that there's a lot of existing theme supports like
disable-custom-colors
... that can be used to control Gutenberg and we want to consolidate all of these in a unique "features" key that can be provided bytheme.json
orblock-editor-settings
filter.This PR explores how we can do so while still retaining backward compatibility for existing theme supports the block editor settings. It explores doing so for the
disableCustomColors
setting. It is now transformed to a feature with the following path:colors.allowCustom
.Also to do so, it adds the global styles settings to the new screens (navigation, widgets, edit-site).
Testing instructions
disable-custom-colors
colors.allowCustom
path.