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

Try generating styles for classic themes by filtering theme.json data #44268

Closed
wants to merge 3 commits into from

Conversation

oandregal
Copy link
Member

What?

Potential alternative to #43981

This PR experiments with extracting the logic we have for classic themes to a different place, making use of the new theme_json_(origin) filters.

Why?

It's increasingly difficult to reason about and follow which styles would classic themes get.

How?

By making use of the theme_json_(origin) filters we can modify the theme.json if, and only if, the current theme in use is a classic theme.

Testing Instructions

Not ready for testing. It's just a skeleton so far.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Very cool idea, thanks for opening this up @oandregal! I can imagine it being really useful for if and when we need a different output for classic versus blocks-based themes.

I think the problem this abstraction solves might be slightly tangential to the problem of outputting the styles in #43981 at the desired level of specificity for Classic themes, though. Unless I'm missing something, I think the idea is for the base Buttons styling rules to be effectively the same in Classic and blocks themes?

$new_data = array(
'version' => 2,
'settings' => array( /* we should maintain the presets by core */ ),
'styles' => array( /* clear this? add only the base-layout-styles? */ ),
Copy link
Contributor

Choose a reason for hiding this comment

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

As a bit of background, the subtlety behind base-layout-styles is that it only outputs layout styles and skips calling get_block_classes. The logic is around here: https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php#L679-L681

So it's slightly different to filtering the styles object.

That said, the original reason for base-layout-styles was to ensure that we can output base / root layout styles for things like the gap between Buttons, without pulling in / outputting styles from the styles object. Given that #43981 is proposing outputting styles for classic themes, too, then the concept of base-layout-styles might become redundant anyway 🙂

@oandregal oandregal force-pushed the try/theme-json-for-classic-themes branch from a607975 to 5c019f0 Compare September 21, 2022 15:31
@@ -47,6 +47,10 @@ public function update_with( $new_data ) {
return $this;
}

public function create_with( $new_data ) {
Copy link
Member Author

@oandregal oandregal Sep 21, 2022

Choose a reason for hiding this comment

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

Turns out we don't have a great way to clear existing data through the merge process if it's not overwrite it. This creates a new object from scratch. May need to revisit the ability to clear data but this works for now.

* We should be able to remove this.
* Without it, it breaks.
*/
"spacing" => array(
Copy link
Member Author

@oandregal oandregal Sep 21, 2022

Choose a reason for hiding this comment

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

Something breaks if I create a WP_Theme_JSON object without settings. It seems that it's this bit that fails. I haven't been able to look at why, but it should be possible to create an object with empty settings, correct? cc @glendaviesnz

@oandregal
Copy link
Member Author

I'm gardening my PRs and this one doesn't seem to have gotten any momentum. Closing for now, can reopen later if necessary.

@oandregal oandregal closed this Apr 18, 2023
@oandregal oandregal deleted the try/theme-json-for-classic-themes branch April 18, 2023 14:01
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