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

Consolidate overall Global Styles mechanics in the server #20047

Merged
merged 40 commits into from
Feb 14, 2020

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Feb 5, 2020

This PR seeks to consolidate the overall server-side mechanism for Global Styles. Continues work from #19883

  1. It adds the user's Global Styles to the resolver, so it now works using core, theme, and user data. It stores the user's data using a Custom Post Type, so we can leverage existing editor mechanisms (undo/redo, draft/publish status for Global Styles, etc). This also presumes we want Global Styles to be tied to the theme so when theme changes we want to start with a new blank slate for Global Styles, which is implemented by using the theme's textdomain as part of the CPT's post name.

  2. It exposes to the editor a couple of variables necessary for live-editing the styles: the ID of the entity that holds the user's styles + the base styles (core+theme). How this works will be demonstrated in subsequent PR. Note this is made to work only with the Beta Site Editor.

  3. As per past conversations, it flattens the Global Styles tree stripping out the block variables. It looks like this:

{
	'color': {
		'primary': '<primary color>',
		'background': '<background color>',
		'text': '<text color>',
	},
        'line-height': '<line height>'
}

which will be converted to this CSS rule (see discussion about syntax):

:root {
	--wp--color--primary: '<primary color>';
	--wp--color--background: '<background color>';
	--wp--color--text: '<text color>';
        --wp--line-height: '<line height>';
}

Note that the names and variables we expose are still in flux, so take the current ones as only valid for demo purposes.

How to test

  • Install and enable one of the block-based themes (find one at theme-experiments). Instructions here assume Twenty Twenty Blocks.
  • Check out this PR and npm install && npm run build.
  • Create a void file named experimental-theme.json in your current theme's folder. For example, wp-content/themes/twentytwenty-blocks/experimental-theme.json.
  • Got to Gutenberg > Experiments and enable "Full Site Editing".
  • Go to Gutenberg > Site Editor.

1) Check that core's Global Styles work:

  • Add a button and verify that its background color is #52accc.

2) Check that theme's Global Styles override core's if present:

  • Add the following into the experimental-theme.json file created before.
{
	"color": {
		"primary": "hotpink"
	}
}
  • Reload the Site Editor.
  • Add a button and verify that the button's color is now hotpink.

3) Check that user's Global Styles override theme's if present:

  • Insert in the database a new post with the following data [1]:
    • post_type='wp_global_styles'
    • post_name='wp-global-styles-THEME-TEXTDOMAIN' (ex: 'wp-global-styles-twentytwenty-blocks')
    • post_content='{"color":{"primary":"blue"}}'
    • post_status='publish'
  • Reload the Site Editor.
  • Add a button and verify that the button's color is now blue.

[1] Make sure there isn't another post with the same post_name; if there is one, delete it first. Find below some ways you can add the required data for testing purposes. This is for the experimental Twenty Twenty Blocks theme, change to use your active theme's text-domain as required.

wp-cli -- you may need to provide other connection parameters depending on your setup:

wp post delete $(wp post list --post_type='wp_global_styles' --format=ids) --force
wp post create
    --post_type='wp_global_styles'
    --post_name='wp-global-styles-twentytwenty-blocks'
    --post_content='{"color":{"primary":"blue"}}'
    --post_status='publish'

PHP -- paste the following code within the gutenberg_experimental_global_styles_enqueue_assets when it comes to test user's Global Styles:

	// Delete existing CPT, if any.
	$gs_cpt_list = wp_get_recent_posts( [
		'post_type' => 'wp_global_styles',
	] );
	foreach ( $gs_cpt_list as $gs_cpt ) {
		wp_delete_post( $gs_cpt['ID'] );
	}

	// Insert new one with required data.
	wp_insert_post( [
	    'ID'           => 0,
	    'post_content' => json_encode( [
	        'color'=> [
	            'primary' => 'blue',
	        ],
	    ] ),
	    'post_type'   => 'wp_global_styles',
	    'post_name'   => 'wp-global-styles-twentytwenty-blocks',
	    'post_status' => 'publish',
	] );

@oandregal oandregal self-assigned this Feb 5, 2020
@oandregal oandregal added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Feb 5, 2020
@oandregal oandregal requested a review from ItsJonQ February 5, 2020 11:02
@oandregal oandregal changed the title Consolidate overrall Global Styles mechanics in the server Consolidate overall Global Styles mechanics in the server Feb 5, 2020
@oandregal
Copy link
Member Author

oandregal commented Feb 5, 2020

I just realized we didn't hide the GlobalStyles code behind a experimental flag. I'll add that in this PR.

Edit: added in 76e744e and e33a7a5.

@ItsJonQ
Copy link

ItsJonQ commented Feb 5, 2020

Heya @nosolosw !! Thank you for working on this 😍

I walked through the steps you outlined:

  • Check out this PR and npm install && npm run build.
  • Create a void file named experimental-theme.json in your current theme's folder.
  • Enable the "Global Styles" in the experiments page.
  • Create a new post with a block button, publish, and go to the front end.

However, I wasn't able to see the Global styles stuff render. No wp-gs className being added, no hotpink button.

Is there something I need to do in addition to this?

Note: Testing locally on fresh Docker install and TwentyTwenty theme.

Thanks!


Update, I also tried running the wp command, but I see this:

❯ wp post create --post_type='wp_global_styles' --post_name='wp-global-styles-twenty-twenty' --post_content='{"color":{"primary":"blue"}}' --post_status='publish'
Warning: mysqli_real_connect(): php_network_getaddresses: getaddrinfo failed: nodename nor servname provided, or not known in /Users/Q/Sites/@wordpress/gutenberg/wordpress/src/wp-includes/wp-db.php on line 1638
Warning: mysqli_real_connect(): (HY000/2002): php_network_getaddresses: getaddrinfo failed: nodename nor servname provided, or not known in /Users/Q/Sites/@wordpress/gutenberg/wordpress/src/wp-includes/wp-db.php on line 1638
Error: `php_network_getaddresses: getaddrinfo failed: nodename nor servname provided, or not known`
Error establishing a database connection
This either means that the username and password information in your `wp-config.php` file is incorrect or we can’t contact the database server at `mysql`. This could mean your host’s database server is down.

Are you sure you have the correct username and password?
Are you sure you have typed the correct hostname?
Are you sure the database server is running?

If you’re unsure what these terms mean you should probably contact your host. If you still need help you can always visit the WordPress Support Forums. `php_network_getaddresses: getaddrinfo failed: nodename nor servname provided, or not known`

@oandregal
Copy link
Member Author

Nope, that's all you need! Just tested with a brand new site, fresh clone PR, and the steps above: it worked fine for me.

If there is no wp-gs or #global-styles-inline-css stylesheet it means GlobalStyles isn't active: either the experimental-theme.json couldn't be found in the top-level dir of the theme or the experiment flag isn't enabled.

As per the CLI error. It looks like you need to provide some extra info (user/pass)? If you use the env bundled with gutenberg it may be more convenient to do npm run env cli instead. An alternative to CLI if that gets cumbersome would be pasting the following code within the gutenberg_experimental_global_styles_enqueue_assets function for testing user's Global Styles:

	// Delete existing CPT, if any.
	$gs_cpt_list = wp_get_recent_posts( [
		'post_type' => 'wp_global_styles',
	] );
	foreach ( $gs_cpt_list as $gs_cpt ) {
		wp_delete_post( $gs_cpt['ID'] );
	}

	// Insert new one with required data.
	wp_insert_post( [
	    'ID'           => 0,
	    'post_content' => json_encode( [
	        'color'=> [
	            'primary' => 'blue',
	        ],
	    ] ),
	    'post_type'   => 'wp_global_styles',
	    'post_name'   => 'wp-global-styles-twenty-twenty', // Note this is for TwentyTwenty!
	    'post_status' => 'publish',
	] );

@oandregal
Copy link
Member Author

I saw that we were using the Full Site Editing flag in #20061 so I thought it'd be best to use it here as well. Changed in f9d90e0 and updated the instructions accordingly.

@ItsJonQ
Copy link

ItsJonQ commented Feb 6, 2020

@nosolosw Hallooo!! Thank you for your support 🙏
Unfortunately, I'm still not able to get it working on my end

I've recorded a video here walking through the instructions:

https://www.loom.com/share/0635e0c69b1042129e09a12f555920e3

Hopefully this helps with some debugging?

@ItsJonQ
Copy link

ItsJonQ commented Feb 7, 2020

@nosolosw Hallo!!!

Just tested this again, and it's working as expected! I have no idea why it wasn't working for me previously. Thank you so much for your efforts in helping me debug! 🤗

It was most likely my local Docker environment. My machine has been having issues lately.
I reinstalled all Docker things 2 days ago, and it's been pretty stable since.

I left a comment for something to fix. After that, I would say this is 👍 from me!

@oandregal
Copy link
Member Author

I left a comment for something to fix.

@ItsJonQ I don't seem to find any comment in the PR?

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

@nosolosw Oops! I forgot to hit Request changes! 🙇


if ( is_array( $value ) ) {
$result = array_merge(
$result,
gutenberg_get_css_vars( $value, $new_key, false )
gutenberg_experimental_global_styles_get_css_vars( $value, $new_key . '-' )
Copy link

Choose a reason for hiding this comment

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

@nosolosw Haii!!!

Would we be able to change this from - to -- (double hyphen)?

@jorgefilipecosta made the suggestion of using double hyphen to better indicate depth levels from the global styles object within the flattened CSS var string.

https://github.com/WordPress/gutenberg/pull/19883/files#r373700289

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! But this was when we had different levels (globals vs blocks) so it was useful to differentiate the element part (global/block name) vs the CSS custom property name. Now that we don't have the element part (everything is prefixed by --wp-), is the -- still useful?

Copy link
Member Author

@oandregal oandregal Feb 10, 2020

Choose a reason for hiding this comment

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

I mean, now we have --wp-color-primary, would we want to have --wp--color-primary instead?

Copy link
Member Author

@oandregal oandregal Feb 11, 2020

Choose a reason for hiding this comment

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

Updated at 7efa53e and 798a0d3


@jorgefilipecosta and I talked this over and I want to write down the trade-offs of the current approach so we have them documented somewhere until we find a better place.

Jorge's rationale for suggesting the double dash is this: in scoped styles (we store the CSS custom properties as block attributes) we need to deserialize the string into an object for editor things. In that case, we won't be able to distinguish between a valid CSS property name (ex: line-height) and a name that results of flattening the object keys (background-color from {background: { color: 'red' }}). Let's work through an example. We've got this string we want to deserialize back into an object:

--wp-background-color: 'red';
--wp-line-height: 1.2;

How do we know which object to deserialize into without information about the object keys/CSS property names?

This is what we want:

{
    background: {
        color: 'red',
    },
    line-height: 1.2
}

A simple parser that creates one level per dash would return (which is not what we want, so we have to make the parser a bit more complex):

{
    background: {
        color: 'red',
    },
    line {
        height: 1.2,
    }
}

By using double dashes the deserialization parser is simpler (each object level is separated by --):

--wp--background--color: 'red';
--wp--line-height: 1.2;

Personally, I'm not a fan of this implementation detail leaking into the API design/naming schema. However, I also think this is a fair approach to simplify the parser and get us going. The underlying assumption is that we store this information only once in the block attribute as a string, not as an object: when we cross that river, we'd have the opportunity to reevaluate this and change if necessary.

@oandregal oandregal force-pushed the try/global-styles-server branch from f9d90e0 to d950b5f Compare February 11, 2020 10:00
@oandregal oandregal requested a review from ItsJonQ February 11, 2020 10:30
@oandregal
Copy link
Member Author

This PR is ready to land, although it only works in the block-editor. We're going to integrate Global Styles in the edit-site editor, for which we need to add support for some things in edit-site:

  • hook admin_body_class: we use this to add the wp-gs class. Although the hook is fired, we need a way to identify the edit-site context server-side (something akin to $current_screen->is_block_editor).
  • hook block settings: doesn't fire for edit-site context.
  • hook enqueue_block_assets: doesn't fire for edit-site context.

I'm going to work on the edit-site things separately, so we can land this PR without being blocked.

@oandregal
Copy link
Member Author

I was able to make this work for edit-site in a decoupled and simple way, so I went ahead and added the code in this branch. This works now only with edit-site and frontend.

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

🚀 @nosolosw Awesome work!!! Thank you for creating and iterating on this! Not only that, but you've also added it to FSE "Edit Site".

Testing locally, and it's working as outlined in your instructions.

From a code perspective, the mechanics of the PHP updates make sense of me.

@ItsJonQ
Copy link

ItsJonQ commented Feb 11, 2020

@jorgefilipecosta If you're around, it would be great to get your thoughts on the PHP implementation 🙏 . Thank you!!

lib/experiments-page.php Outdated Show resolved Hide resolved
@oandregal oandregal force-pushed the try/global-styles-server branch from a218643 to 6f1e87b Compare February 11, 2020 19:25
@oandregal oandregal merged commit d870797 into master Feb 14, 2020
@oandregal oandregal deleted the try/global-styles-server branch February 14, 2020 09:30
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 14, 2020
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @nosolosw nice work here 👍 generally the changes seem good and I think they are going in the right direction.

function gutenberg_experimental_global_styles_get_user_cpt( $post_status_filter = array( 'publish' ), $should_create_draft = false ) {
$user_cpt = array();
$post_type_filter = 'wp_global_styles';
$post_name_filter = 'wp-global-styles-' . strtolower( wp_get_theme()->get( 'TextDomain' ) );
Copy link
Member

Choose a reason for hiding this comment

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

I think as an id of the theme we should use get_stylesheet() and not the 'TextDomain'. I think the database setting the WordPress core stores in the database to identify the current theme is the stylesheet.

Copy link
Member Author

Choose a reason for hiding this comment

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

The template parts use the TextDomain for this, so I thought we may want to use the same mechanism for API consistency. I'm happy to change it to something else, but I guess it makes sense to update template parts as well?

$cpt_post_id = wp_insert_post(
array(
'post_content' => '{}',
'post_status' => 'draft',
Copy link
Member

Choose a reason for hiding this comment

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

I think we may have a bug, in gutenberg_experimental_global_styles_get_user we are querying published posts, but we insert a draft post. On the next execution, a published post will still not exist and we will probably create another one.
I think we may be able to create the post with the published status ( the empty object will not do anything);

Copy link
Member Author

@oandregal oandregal Feb 17, 2020

Choose a reason for hiding this comment

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

Note the optional parameter $should_create_draft, which is false by default. We only insert a draft if there is no CPT during the editor call gutenberg_experimental_global_styles_get_user_cpt_id. That's the only place we need a pre-existing CPT: the entities API only works with pre-existing objects, can't create new ones from the client-side.


/**
* Adds class wp-gs to the frontend body class if the theme defines a experimental-theme.json.
Copy link
Member

Choose a reason for hiding this comment

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

If we unconditionally add the wp-gs class we will break the styles of some blocks.

.wp-gs .wp-block-button__link:not(.has-background) {
	background-color: var(--wp--color--primary);
}

As the class is added even if the theme does not support global styles a theme that was changing the button color using the following code will stop working:

 .wp-block-button__link {
	background-color: red;
}

I think the whole idea of adding wp-gs was to mark themes that support global styles if we unconditionally add the class we would be able to not add it at all as well.
I know that for now we only add if the experiment is enabled but one day we remove the experiment and in that case, we would unconditionally add the class.
I think we should add the class when the experiment is enabled and when the theme supports global styles.

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 already do that! The class is only added when the experiment is enabled, the theme has support, and the editor is site editor (for back-end, doesn't apply to front-end).

@@ -37,7 +37,7 @@ $blocks-button__height: 56px;
}

.wp-gs .wp-block-button__link:not(.has-background) {
background-color: var(--wp-block-core-button--color--background, var(--wp-color--primary, $dark-gray-700));
background-color: var(--wp--color--primary);
Copy link
Member

Choose a reason for hiding this comment

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

Should we pass $dark-gray-700 as a fallback?

Copy link
Member Author

@oandregal oandregal Feb 17, 2020

Choose a reason for hiding this comment

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

Note that --wp--color--primary is going to have always a value (user-defined, the theme default, or core default), so the fallback won't be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants