Skip to content
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: Global styles css variables generation mechanism #19883

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions experimental-default-global-styles.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't included in the plugin:

https://plugins.trac.wordpress.org/browser/gutenberg/tags/7.7.1

Because we whitelist files to include in the ZIP, and this isn't one of them:

build_files=$(ls build/*/*.{js,css,asset.php} build/block-library/blocks/*.php build/block-library/blocks/*/block.json)
# Generate the plugin zip file.
status "Creating archive... 🎁"
zip -r gutenberg.zip \
gutenberg.php \
lib \
packages/block-serialization-default-parser/*.php \
post-content.php \
$vendor_scripts \
$build_files \
readme.txt \
changelog.txt \
README.md

We could simply add the pattern there, but I also think we should try to align some convention of where we want these sort of static files to live.

If we move this into lib or a subfolder, it would be included.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR moves it within lib/global-styles.

"global": {
"color": {
"primary": "#52accc",
"background": "white",
"text": "black"
}
}
}
131 changes: 131 additions & 0 deletions lib/global-styles.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
<?php
/**
* Global styles functions, filters and actions, etc.
*
* @package gutenberg
*/

/**
* Function that given a branch of the global styles object recursively generates
* an array defining all the css vars that the global styles object represents.
*
* @param array $global_styles_branch Array representing a brach of the global styles object.
* @param string $prefix Prefix to append to each variable.
* @param bool $is_start Indicates if we are on the first call to gutenberg_get_css_vars (outside the recursion).
* @return array An array whose keys are css variable names and whose values are the css variables value.
*/
function gutenberg_get_css_vars( $global_styles_branch, $prefix = '', $is_start = true ) {
$result = array();
foreach ( $global_styles_branch as $key => $value ) {
$processed_key = str_replace( '/', '-', $key );
$separator = $is_start ? '' : '--';
$new_key = ( $prefix ? $prefix . $separator : '' ) . $processed_key;

if ( is_array( $value ) ) {
$result = array_merge(
$result,
gutenberg_get_css_vars( $value, $new_key, false )
);
} else {
$result[ $new_key ] = $value;
}
}
return $result;
}

/**
* Function responsible for enqueuing the style that define the global styles css variables.
*/
function gutenberg_enqueue_global_styles_assets() {
if ( ! locate_template( 'experimental-theme.json' ) ) {
return;
}
$default_global_styles_path = dirname( dirname( __FILE__ ) ) . '/experimental-default-global-styles.json';
$default_global_styles = null;

if ( file_exists( $default_global_styles_path ) ) {
$default_global_styles = json_decode(
file_get_contents( $default_global_styles_path ),
true
);
}

$theme_json_path = locate_template( 'experimental-theme.json' );
$theme_json_global_styles = null;
if ( $theme_json_path ) {
$theme_json_global_styles = json_decode(
file_get_contents( $theme_json_path ),
true
);
}

// To-do: Load user customizations from a CPT.
$css_vars = array();
foreach (
array(
$default_global_styles,
$theme_json_global_styles,
) as $global_styles_definition
) {
if ( ! $global_styles_definition ) {
continue;
}
if ( isset( $global_styles_definition['global'] ) ) {
$css_vars = array_merge(
$css_vars,
gutenberg_get_css_vars( $global_styles_definition['global'], '--wp-' )
);
}
if ( isset( $global_styles_definition['blocks'] ) ) {
$css_vars = array_merge(
$css_vars,
gutenberg_get_css_vars( $global_styles_definition['blocks'], '--wp-block-' )
);
}
}

if ( empty( $css_vars ) ) {
return;
}

$inline_style = ":root {\n";
foreach ( $css_vars as $var => $value ) {
$inline_style .= "\t" . $var . ': ' . $value . ";\n";
}
$inline_style .= '}';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A filter here would come in really handy...
I'd like to work on this issue: #19611 (comment) but without a filter it's just going to get messy 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are still in early stages, but I agree filters here make sense. I will propose an experimental filter 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we hold on adding filters until we're sure about the APIs...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to start making a list of which functions (in the non-code sense) make sense to extend.

wp_register_style( 'global-styles', false, array(), true, true );
wp_add_inline_style( 'global-styles', $inline_style );
wp_enqueue_style( 'global-styles' );
}
add_action( 'enqueue_block_assets', 'gutenberg_enqueue_global_styles_assets' );

/**
* Adds class wp-gs to the frontend body class if the theme defines a experimental-theme.json.
*
* @param array $classes Existing body classes.
* @return array The filtered array of body classes.
*/
function gutenberg_add_wp_gs_class_front_end( $classes ) {
if ( locate_template( 'experimental-theme.json' ) ) {
return array_merge( $classes, array( 'wp-gs' ) );
}
return $classes;
}
add_filter( 'body_class', 'gutenberg_add_wp_gs_class_front_end' );


/**
* Adds class wp-gs to the block-editor body class if the theme defines a experimental-theme.json.
*
* @param string $classes Existing body classes separated by space.
* @return string The filtered string of body classes.
*/
function gutenberg_add_wp_gs_class_editor( $classes ) {
global $current_screen;
if ( $current_screen->is_block_editor() && locate_template( 'experimental-theme.json' ) ) {
return $classes . ' wp-gs';
}
return $classes;
}
add_filter( 'admin_body_class', 'gutenberg_add_wp_gs_class_editor' );
1 change: 1 addition & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,4 @@ function gutenberg_is_experiment_enabled( $name ) {
require dirname( __FILE__ ) . '/experiments-page.php';
require dirname( __FILE__ ) . '/customizer.php';
require dirname( __FILE__ ) . '/edit-site-page.php';
require dirname( __FILE__ ) . '/global-styles.php';
4 changes: 4 additions & 0 deletions packages/block-library/src/button/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ $blocks-button__height: 56px;
}
}

.wp-gs .wp-block-button__link:not(.has-background) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain to me why the .wp-gs is necessary? Why can't we just drop it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I added the rule .wp-block-button__link:not(.has-background) to the block most existing themes would break. That happens because that selector has a specificity of two classes, so a theme setting the background color using .wp-block-button__link{ background-color: ...} would stop working.

Another case where this problem is even more noticeable is the implementation of global styles in a paragraph. We can not simply set p { background-color: var(--wp-gs-paragraph-color-text, var( --wp-gs-color-text ) ) otherwise paragrapgs would probably change colors in most existing themes.

The wp-gs class offers a block the possibility of keeping existing styles so existing themes styling them still work while at the same time they could provide styles that are used in the presence of a theme compatible with global styles.

The CSS variables with the defaults or user-configured options will be always be loaded even if the theme does not support global styles. So a new block build now may not need to use wp-gs and can just use the global styles all the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these problems be solved if we avoid an explicit value at the last argument. Concretely, if it's something like that

background-color: var(--wp-core-button-color-background, var(--wp-color-primary, initial));

(initial or inherit depending on the property we're setting)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this code would not work because --wp-color-primary would always be defined as it is a default that WordPress needs to provide even if themes don't use global styles. If defaults were not provided blocks could never safely rely on a global variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--wp-color-primary would always be defined as it is a default that WordPress needs

Making sure I'm understanding properly here. Why WordPress need to define defaults for these variables?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to provide a set of defaults to make things consistent even if the theme is not setting global styles.
So, for example, block A and block B created by different authors both use the --wp-gs-color-text, if a default for the variable is not provided both blocks may end up using different text colors while ideally, the text color would be the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just consider the browser defaults as the defaults? There's something I'm missing here.

If these blocks do support the variables they will have something like that on their styles right?

color: var(--wp-block-something-color-primary, var(--wp-color-primary, initial));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set something like p:not(.has-background) { color: var(--wp-block-something-color-primary, var(--wp-color-primary, initial)); } without targetting current themes supporting global styles (having wp-gs class) we make break the styles for example if the current theme has p { color: red } the style will stop working as color will take the default browser value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we just remove "initial" from that code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we just remove "initial" from that code?

In my tests, I think the browser uses the default value for the property when the var is used (it seems the same as including initial).

So I think we can not avoid wp-gs otherwise we may break styles.

background-color: var(--wp-block-core-button--color--background, var(--wp-color--primary, $dark-gray-700));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgefilipecosta Halloo!!! Just wanted to verify! It looks like there are two hyphens after the prefix, example:

--wp-block-core-button--color--background

Note: --color--background

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is intentional to remove ambiguity. If we don't add two hyphens, we don't know if the variable represents this object { ... { color: { background: ... } } } or this one { ...{ 'color-background': ... } }. Having two different object shapes generate the same variables is something that would, for sure, cause problems as an object may overwrite the styles of another object when it is not supposed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I'm down with this approach 👍
Thank you!

}

.is-style-squared .wp-block-button__link {
border-radius: 0;
}
Expand Down