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

Global Styles: Alternative method for enqueueing custom CSS #47554

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

Mamaduka
Copy link
Member

What?

My alternative to the enqueueing method is proposed at #47396.

Instead of using two separate callbacks for loading custom CSS from Customizer and Site Editor, the new method removes Customzer callback, combines custom CSS values, and enqueues styles after global-styles.

I've also made the following changes to the get_global_styles_custom_css method:

  • Added prefix to avoid conflicts when the function is backported.
  • Removed checks for the $types variable - the function doesn't accept arguments.
  • Update the cache key name and added it to the clean function.
  • Avoid calling the ::get_merged_data if a theme does not have a theme.json file.

Testing Instructions

Please take a look at the instructions in the original PR.

@Mamaduka Mamaduka self-assigned this Jan 30, 2023
@Mamaduka Mamaduka added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Jan 30, 2023
@github-actions
Copy link

github-actions bot commented Jan 30, 2023

Flaky tests detected in d552d79.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4053536069
📝 Reported issues:

@@ -77,7 +77,7 @@ function gutenberg_get_global_styles_custom_css() {
}

if ( ! wp_theme_has_theme_json() ) {
return;
return '';
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to return empty string here.

@glendaviesnz
Copy link
Contributor

This tested well for me. The only thing I noticed is that with using wp_add_inline_style within wp_enqueue_scripts instead of echoing the style in wp_head it places the styles higher up in the header than the default customizer output which is towards the very end of the header. I think there would only be a very minimal risk of this causing any specificity issues with any existing hybrid themes with customizer custom CSS set so don't think it is too critical, what do you think?

A comment was also made here about trying to keep the custom CSS loading in the same location as the customizer CSS - I don't have a strong opinion one way or the other on how critical this is - use of the wp_head and manually echoing out the <style> element seems like the only way to do this, and this seems to be a little hacky.

@Mamaduka
Copy link
Member Author

Using wp_head with high priority doesn't guarantee that Global Styles's custom CSS will be loaded last or even after global-styles.

Let's keep using wp_add_inline_style for now. Then, we can change the callback hook and printing method based on feedback. What do you think?

cc @aristath

@aristath
Copy link
Member

Let's keep using wp_add_inline_style for now. Then, we can change the callback hook and printing method based on feedback. What do you think?

Sounds good to me 👍

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@Mamaduka Mamaduka merged commit 3b44904 into update/custom-css-load-order Jan 31, 2023
@Mamaduka Mamaduka deleted the alt/custom-css-load-order branch January 31, 2023 12:00
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