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 #1287

Closed
wants to merge 1 commit into from
Closed

Add global styles #1287

wants to merge 1 commit into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented May 24, 2021

Part of https://core.trac.wordpress.org/ticket/53175
Follow-up to #1262

To do

  • prepare corresponding Gutenberg PR

How to test

A theme without theme.json (use the TwentyTwentyOne theme):

  • Load the front. Look for a global-styles-inline-css embedded stylesheet. The expected result is that none should be found.
  • Load the post editor. Search for --wp--preset--color. The expected result is that none should be found.

A theme with theme.json support:

  • Load the front. Look for a global-styles-inline-css embedded stylesheet. The expected result is that there's a stylesheet that contains the CSS generated by the engine. Depending on the theme's theme.json, it should be something along these lines:
body{
  --wp--preset--color--black: #000000;
  /* more CSS Custom Properties for the presets */
}
.has-black-color{ color: #000000 !important; }
.has-black-background-color{ background-color: #000000 !important; }
/* other classes for every preset value  */
  • Load the post editor. Search for --wp--preset--color. The expected result is that there's an embedded stylesheet that contains the CSS Custom Properties.
  • Load the post editor. Search for .has-black-color{ ... }. The expected result is that there's an embedded stylesheet that contains the classes for the presets as well as any other block styles defined by the theme via theme.json.

@oandregal oandregal changed the title Add globwp_enqueue_global_styles function Add global styles May 24, 2021
@oandregal
Copy link
Member Author

Need to update the unit tests and prepare the corresponding Gutenberg PR, but the code is ready for review.

* @return void
*/
function wp_enqueue_global_styles() {
if ( ! WP_Theme_JSON_Resolver::theme_has_support() ) {
Copy link
Member

Choose a reason for hiding this comment

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

In cases where there is link color enabled add_theme_support('experimental-link-color'); we need to output the preset classes, should we have a condition for that here?

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'm not familiar with prior context on how to deal with this so I'd welcome advice. My thinking is that we should do the following:

  • We shouldn't port experimental-link-color to WordPress core. Themes that want that should use enable it via theme.json.
  • In the plugin, I'd expect we remove support for experimental-link-color at some point. Probably when the minimum WordPress version for the plugin is 5.8.

Thoughts?

Copy link
Member

@jorgefilipecosta jorgefilipecosta May 24, 2021

Choose a reason for hiding this comment

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

Yes this seems like a plan 👍

$stylesheet = $theme_json->get_stylesheet();

if ( $can_use_cache ) {
set_transient( 'global_styles', $stylesheet, MINUTE_IN_SECONDS );
Copy link
Member

Choose a reason for hiding this comment

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

Now that this going to be shipped in production would it make sense to have a bigger cache time? cc: @aristath
On another question would it make sense to use add_action( 'switch_theme', ... ); to clean the cache during a theme switch as we do in #1262?

@oandregal oandregal marked this pull request as ready for review May 24, 2021 15:41
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.

This worked well on my tests and should be ready to merge 👍
I found some issues setting an empty color palette does not work (it defaults to core) but that's a regression setting an empty color palette is a way to disable the color picker. I guess we should look into these issues separately.

@jorgefilipecosta
Copy link
Member

Committed in 3d3aa3c this PR can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants