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

Make child themes inherit parent's style variations. #46554

Merged
merged 15 commits into from
Jan 18, 2023

Conversation

jffng
Copy link
Contributor

@jffng jffng commented Dec 14, 2022

What?

Makes parent theme style variations inherited by the child theme.

cc @WordPress/block-themers

Why?

Closes #45965

How?

Changes the get_style_variations method of the theme json resolver. Checks if it's a child theme and returns the parent theme variations in addition to the child's. If the file basename of the child variation is the same as its parent, it will only include the child's ("overwriting" the parent variation).

Testing Instructions

  1. Activate a child of a theme that has style variations. You can use this TT3 child for testing: TT3: add a test child theme that overwrites the Canary variation theme-experiments#311
  2. Verify the child theme's variations appear in addition to the parent themes:
Before After
Screenshot 2022-12-16 at 12 41 06 PM Screenshot 2022-12-16 at 12 38 26 PM

Testing Instructions for Keyboard

Screenshots or screencast

@jffng
Copy link
Contributor Author

jffng commented Dec 16, 2022

I saw there is an effort to reorganize the theme json code instead of inheriting per WP version: #46579 (comment)

I may wait on that PR to land this one, to simplify backporting later, but any general feedback is welcome.

@scruffian
Copy link
Contributor

This is looking good. It might be good to add a unit test, so that we don't introduce regressions in the future.

@ockham
Copy link
Contributor

ockham commented Dec 21, 2022

Thanks a lot for for working on this, @jffng!

I agree with @scruffian that this is the kind of code we'd want covered by some unit test(s) to avoid regressions.
I think we have some prior art in the repo that we can use as inspiration: In the phpunit/data/themedir1/ directory, we have a few test themes that we might be able to use here.

You could e.g. add some variations to block-theme's theme.json and write a test that sets block-theme-child as the active theme and verifies that its parent's style variations are returned by the relevant function.

I think that this existing test looks like we could copy and modify it to cover the behavior for child themes:

public function test_get_theme_items() {
wp_set_current_user( self::$admin_id );
$request = new WP_REST_Request( 'GET', '/wp/v2/global-styles/themes/emptytheme/variations' );
$response = rest_get_server()->dispatch( $request );
$data = $response->get_data();
$expected = array(
array(
'version' => 2,
'settings' => array(
'color' => array(
'palette' => array(
'theme' => array(
array(
'slug' => 'foreground',
'color' => '#3F67C6',
'name' => 'Foreground',
),
),
),
),
),
'styles' => array(
'blocks' => array(
'core/post-title' => array(
'typography' => array(
'fontWeight' => '700',
),
),
),
),
'title' => 'variation',
),
);
$this->assertSameSetsWithIndex( $data, $expected );
}

@jffng jffng force-pushed the try/child-theme-inherit-parent-style-variations branch from cce171a to a5b4967 Compare January 11, 2023 05:38
@jffng
Copy link
Contributor Author

jffng commented Jan 11, 2023

I rebased this since the resolver class was bundled into a single file.

Will work on updating the tests next.

@github-actions
Copy link

github-actions bot commented Jan 11, 2023

Flaky tests detected in fcfbc12.
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/3936732768
📝 Reported issues:

ksort( $nested_html_files );
foreach ( $nested_html_files as $path => $file ) {
$variation_files = static::recursively_iterate_JSON( $base_directory );
if ( $template_directory !== $base_directory && is_dir( $template_directory ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

It fails to show style variations from parent when child theme doesn't have a styles directory. I guess, it should still have them displayed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, thanks for testing.

I addressed this in 1fdb35d

@@ -1,5 +1,5 @@
{
"version": 1,
"version": 2,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the version number here, which seems like it was incorrect to begin with since it was already using the customTemplates and templateParts properties which were added in v2.

@jffng
Copy link
Contributor Author

jffng commented Jan 12, 2023

I added a unit test in 1a892f3, let me know what you think @ockham @scruffian when you have a moment 🙏

@ockham
Copy link
Contributor

ockham commented Jan 16, 2023

I added a unit test in 1a892f3, let me know what you think @ockham @scruffian when you have a moment 🙏

Thank you for that! The PR is looking pretty good! One final suggestion, would you mind adding another style variation to also cover the "overwriting" behavior in the test:

If the file basename of the child variation is the same as its parent, it will only include the child's ("overwriting" the parent variation).


Aside, some unrelated tests are currently failing for me when running them locally. I'll rebase this PR to make sure it's inconsequential.

@ockham ockham force-pushed the try/child-theme-inherit-parent-style-variations branch from 337a1a8 to 7877289 Compare January 16, 2023 13:15
@jffng
Copy link
Contributor Author

jffng commented Jan 17, 2023

@ockham thanks for the review!

would you mind adding another style variation to also cover the "overwriting" behavior in the test:

Done in fcfbc12. I added an additional variation to the parent, so the test covers both the following behaviors of the get_style_variations function:

  • a child theme's variation overwrites a parent theme's variation of the same name (variation-a)
  • the parent theme's variation is inherited (variation-b)

@ockham
Copy link
Contributor

ockham commented Jan 17, 2023

Thank you, that's exactly the kind of coverage I had in mind 👍

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Tested manually with the child theme from WordPress/theme-experiments#311. Code looks good, and unit tests cover the expected behavior nicely 😊

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Child themes should inherit style variations
6 participants