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

Check wp_theme_has_theme_json in addition to wp_is_block_theme in gutenberg_enqueue_global_styles_custom_css() #52655

Closed
wants to merge 3 commits into from

Conversation

mrleemon
Copy link
Contributor

@mrleemon mrleemon commented Jul 14, 2023

What?

Replace wp_is_block_theme with wp_theme_has_theme_json in gutenberg_enqueue_global_styles_custom_css() to make custom css from theme.json work on frontend IN CLASSIC THEMES WITH A THEME.JSON FILE, not only pure block themes.

Fixes #52644

I hope this can be fixed for WP6.3

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@spacedmonkey
Copy link
Member

This is a big change. Please do not merge until testing and performance review has been done.

@spacedmonkey
Copy link
Member

Trunk This PR
Response Time (median) 54.57 54.74
wp-load-alloptions-query (median) 0.88 0.88
wp-before-template (median) 26.11 26.24
wp-before-template-db-queries (median) 2.8 2.77

This change has a performance overhead, but not a large one.

@carolinan carolinan added the [Type] Bug An existing feature does not function as intended label Jul 17, 2023
@Mamaduka Mamaduka added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Jul 17, 2023
@Mamaduka
Copy link
Member

The custom CSS feature is meant to be used by the block themes, and the css property provides a way to set default values. If I remember correctly, this is the reason for using wp_is_block_theme().

Cc-ing @glendaviesnz; he did more work on this feature than me.

@mrleemon
Copy link
Contributor Author

mrleemon commented Jul 17, 2023

@Mamaduka If that is the case, why is the css theme.json prop working in the backend but not in the frontend in classic themes shipping a theme.json file? The most logical thing would be to make it work in both or in none. But the latter option makes no sense, because the rest of the theme.json props work in classic themes with no problems.

@mrleemon mrleemon changed the title Replace wp_is_block_theme with wp_theme_has_theme_json Jul 17, 2023
@glendaviesnz
Copy link
Contributor

Cc-ing @glendaviesnz; he did more work on this feature than me.

From memory, the discussions around the custom CSS use in classic versus block themes was around avoiding confusion between the existing customizer custom CSS input and the global styles custom CSS. I don't remember thinking about needing to block this specific use case and can't think of any problems with allowing it - if the performance difference isn't an issue.

@carolinan
Copy link
Contributor

Block themes do not require a theme.json file, I think we need to check for one or the other.

@mrleemon
Copy link
Contributor Author

mrleemon commented Jul 19, 2023

Block themes do not require a theme.json file, I think we need to check for one or the other.

I didn't know that. So, to allow the use of the "css" prop in classic themes with a theme.json file, should the check be?

if ( ! wp_is_block_theme() && ! wp_theme_has_theme_json() )

@mrleemon mrleemon changed the title Replace wp_is_block_theme with wp_theme_has_theme_json in gutenberg_enqueue_global_styles_custom_css() Sep 14, 2023
@youknowriad
Copy link
Contributor

Can we make a decision here: either ship this or close the issue as won't fix. I personally see no harm in allowing classic themes with theme.json to provide a custom CSS property via theme.json

@mrleemon
Copy link
Contributor Author

mrleemon commented Jan 20, 2024

I hope this is not wontfixed 🙏. As I explain in #52644, right now, the theme.json css prop is working on the backend in classic/hybrid themes. Why cripple it on the frontend? It makes no sense. Also, why make this specific theme.json prop not work in classic/hybrid themes, while the rest of props does? Fixing this would help increase the adoption of the block editor among some developers.

@mrleemon
Copy link
Contributor Author

This PR is no longer valid because the gutenberg_enqueue_global_styles_custom_css() function has been deprecated in Gutenberg 17.8.

@mrleemon mrleemon closed this Jul 17, 2024
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 [Type] Bug An existing feature does not function as intended
6 participants