Make child themes inherit parent's style variations.#46554
Conversation
|
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. |
|
This is looking good. It might be good to add a unit test, so that we don't introduce regressions in the future. |
|
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. You could e.g. add some variations to I think that this existing test looks like we could copy and modify it to cover the behavior for child themes: gutenberg/phpunit/class-gutenberg-rest-global-styles-controller-test.php Lines 83 to 117 in 8a71fd8 |
cce171a to
a5b4967
Compare
|
I rebased this since the resolver class was bundled into a single file. Will work on updating the tests next. |
|
Flaky tests detected in fcfbc12. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3936732768
|
| 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 ) ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nice catch, thanks for testing.
I addressed this in 1fdb35d
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "version": 1, | |||
| "version": 2, | |||
There was a problem hiding this comment.
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.
|
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:
Aside, some unrelated tests are currently failing for me when running them locally. I'll rebase this PR to make sure it's inconsequential. |
337a1a8 to
7877289
Compare
|
@ockham thanks for the review!
Done in fcfbc12. I added an additional variation to the parent, so the test covers both the following behaviors of the
|
|
Thank you, that's exactly the kind of coverage I had in mind 👍 |
There was a problem hiding this comment.
Tested manually with the child theme from WordPress/theme-experiments#311. Code looks good, and unit tests cover the expected behavior nicely 😊
![]()
What?
Makes parent theme style variations inherited by the child theme.
cc @WordPress/block-themers
Why?
Closes #45965
How?
Changes the
get_style_variationsmethod 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
Testing Instructions for Keyboard
Screenshots or screencast