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

wp_get_global_styles: resolver the reference to the value #49715

Open
oandregal opened this issue Apr 11, 2023 · 11 comments
Open

wp_get_global_styles: resolver the reference to the value #49715

oandregal opened this issue Apr 11, 2023 · 11 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.

Comments

@oandregal
Copy link
Member

Related #45171

Description

When using references, the wp_get_global_styles functions returns the internal reference link instead of the CSS value.

Step-by-step reproduction instructions

  • Use a theme that has a theme.json.
  • Paste the following under styles.blocks:
"core/post-terms": {
    "color":{ "background": {"ref": "styles.color.background"}}
}
  • Use the wp_get_global_styles functions to retrieve those styles. For example, paste the following in functions.php of the theme:
add_action( 'init', function(){
        error_log( print_r( wp_get_global_styles( array(), array('block_name'=>'core/post-terms') ), true ) );
} );

The result will be:

(
    [color] => Array( [background] => Array( [ref] => styles.color.background )
)

Instead, the expected value is that the reference should be resolved to the actual CSS value. Let's say the root background color is black, so the result should be:

(
    [color] => Array( [background] => black )
)

Otherwise, the consumers of this function need to wrangle this themselves, as in #49693

@oandregal oandregal added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Apr 11, 2023
@oandregal
Copy link
Member Author

@oandregal oandregal changed the title wp_get_global_styles: whether to return the reference or the value wp_get_global_styles: resolver the reference to the value Apr 11, 2023
@oandregal
Copy link
Member Author

Note that by not resolving the references we are pushing complexity to the consumers:

  • they need to get the links values in a second pass (or get them all and then use mangle that data)
  • they need to take care of "broken links": when the reference cannot be resolved to a value, it should not be returned by this function, for example.

@ramonjd
Copy link
Member

ramonjd commented Apr 12, 2023

Would something like this work? #49737
It's pretty rough, but most of my time was spent tracking down where to put the loops 😅

@oandregal
Copy link
Member Author

I've done some digging to find out the places where references are exposed to the consumers. There are a few, and I'm afraid that consumers need to deal with this complexity already:

  • The references need to be exposed via the global styles REST endpoints to be available in the client (site editor). Note they don't fully work at the moment, but they should.
  • The theme.json filters expose the references for consumers as well. This is necessary, as we need them in later stages. For example, we cannot resolve the references in the wp_theme_json_data_theme as they won't be available when the user data is merged.

I wonder if wp_get_global_styles returning them also makes sense.

Ideas about next steps:

  1. Create a public utility that resolves them. For example, wp_theme_json_resolve_styles. This could be useful for consumers of the theme.json filters and consumers of the wp_get_global_styles function.
  2. Either create a wp_get_global_styles_resolved or make wp_get_global_styles (via a new parameter) return the resolved styles.

Is that all the options we have?

If so, I'm inclined to go with 1 because 2 doesn't solve the use case of the theme.json filters.

Thoughts?

@ramonjd
Copy link
Member

ramonjd commented Apr 13, 2023

Thanks for doing the heavy thinking around this @oandregal

Create a public utility that resolves them. For example, wp_theme_json_resolve_styles. This could be useful for consumers of the theme.json filters and consumers of the wp_get_global_styles function.

I think this sounds like a good approach - more testable and easier to drop in to places where folks need it.

Just curious: do you see wp_theme_json_resolve_styles accepting a $theme_json arg (similar to the filters)? I can imagine it would resolve the style ref and spit back the entire updated theme_json.

I'm not 100% over the issue, but I'd also assume we'd use it within wp_get_global_styles, e.g.,

$theme_json = wp_theme_json_resolve_styles( WP_Theme_JSON_Resolver::get_merged_data( $origin )->get_raw_data() );

$styles = $theme_json['styles'];

What do you think?

@oandregal
Copy link
Member Author

It sounds good to create wp_theme_json_resolve_style_references so it's useful to everyone.

I'm not sure what to do about wp_get_global_styles:

  1. Change the default behavior to resolve references.
  2. Don't change the default behavior, as it may be useful for extenders to have access to the "links between nodes". Instead, create a new option/parameter allowing consumers to resolve the references.
  3. Don't change anything. Consumers would have now a new utility they can use if they need to resolve them. Example: wp_theme_json_resolve_style_references( wp_get_global_styles ).

The reason I am undecided is that the WordPress code itself doesn't use the wp_get_global_styles function, only the mobile app and potentially other extenders. I've seen some using it via wpdirectory and also copy/pasting parts of the WP_Theme_JSON class (get_property_value, etc.). Tentatively pinging people from mobile and a few plugins for thoughts @geriux @samnajian @rodrigoprimo @Aljullu @nerrad @mmtr @jeherve

@jeherve
Copy link
Contributor

jeherve commented Apr 14, 2023

I'll cc @BogdanUngureanu on this, who is more familiar with this than me (related).

@geriux
Copy link
Member

geriux commented Apr 17, 2023

The reason I am undecided is that the WordPress code itself doesn't use the wp_get_global_styles function, only the mobile app and potentially other extenders.

If we change for example, what it is returned from wp_get_global_styles it will be an issue for the mobile editor due to how app updates work and how to keep compatibility and deprecating old features.

If it's an enhancement to wp_get_global_styles where it would just add functionality without changing the format then that would work perfectly, since we currently have to parse all CSS variables manually when loading the mobile editor.

@Aljullu
Copy link
Contributor

Aljullu commented Apr 20, 2023

Thanks for the ping, @oandregal. I'm not sure if that's what you are looking for, but in case it's helpful:

  • The only place where we use wp_get_global_styles() in WooCommerce (and WC Blocks) is here. Basically we check if the theme provides styles for specific elements and based on that we provide opinionated styles for them or not. I think our usage wouldn't be affected by whether it returns the resolved value or the reference. Sidenote: at the time of writing, we only use it for button elements.
  • I might be mixing things here, but in WC Blocks we have quite a lot of logic to convert from block attributes to resolved styles/class names. I'm not sure if there is a better way of doing so or whether that's something that could be simplified with your proposal, but that's definitely a pain point we have as the logic to convert from attributes to styles/classes is not straight forward.

@samnajian
Copy link
Contributor

I'm personally more inclined towards providing a utility function, as explained in the 3rd option , the reasoning behind it:

  1. The resolved values are generally not needed but only for some use cases.
  2. The alternatives, i.e changing the current/default output would mean breaking changes for something that's already working and used rather widely, or the other option of providing new parameters to the same function will make both the parameters and the function body more complex and less concise.

@oandregal what do you think?

@oandregal
Copy link
Member Author

Relevant thread for this conversation, so we can see everything together https://github.com/WordPress/gutenberg/pull/50484/files#r1192502874

@annezazu annezazu moved this to Needs Dev / Todo in WordPress 6.4 Editor Tasks Aug 30, 2023
@bph bph moved this from Needs Dev / Todo to Done in WordPress 6.4 Editor Tasks Sep 18, 2023
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] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Done
Development

No branches or pull requests

7 participants