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.

Sign up for GitHub

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

Disable settings.shadow.defaultPresets for classic themes #6309

Closed
wants to merge 20 commits into from
Closed

Disable settings.shadow.defaultPresets for classic themes #6309

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e1dd97f
opt-in to shadow presets only in classic themes
ajlende Mar 20, 2024
f838ba3
Fix WP_Test_REST_Themes_Controller schema test
ajlende Mar 20, 2024
4cc07b0
Fix PHP lint
ajlende Mar 20, 2024
43bddac
Fix PHP lint
ajlende Mar 20, 2024
9006ad2
Fix editor-shadow-presets
ajlende Mar 21, 2024
ce570a5
Rename shadow to shadow-presets
ajlende Mar 21, 2024
3845b40
Add inline docs for editor-shadow-presets
ajlende Mar 21, 2024
9e13987
Make classic themes opt-in to default shadow presets
ajlende Mar 22, 2024
f4fa539
Allow classic themes to opt-in to default shadow presets with appeara…
ajlende Mar 22, 2024
b4932d2
Remove option for configuring shadows in classic themes
ajlende Mar 22, 2024
99e8d0e
Remove unused code
ajlende Mar 22, 2024
cb9c2f7
Disable shadow presets for classic themes
ajlende Mar 25, 2024
c35f2dd
Remove doc comment for 6.5.0
ajlende Mar 26, 2024
522ff61
Add tests for ticket 57545
oandregal Mar 27, 2024
1aa93a7
Fix lint issues
oandregal Mar 27, 2024
feec02d
Update trac ticket
oandregal Mar 27, 2024
9888800
Make linter happy
oandregal Mar 27, 2024
7955b41
Update test_translations_are_applied test set
oandregal Mar 27, 2024
c32ceb3
Make linter happy
oandregal Mar 27, 2024
064b549
Update test set for test_merges_child_theme_json_into_parent_theme_json
oandregal Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/wp-includes/class-wp-theme-json-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,17 @@ public static function get_theme_data( $deprecated = array(), $options = array()
}
$theme_support_data['settings']['color']['defaultGradients'] = $default_gradients;

if ( ! isset( $theme_support_data['settings']['shadow'] ) ) {
$theme_support_data['settings']['shadow'] = array();
}
/*
* Shadow presets are explicitly disabled for classic themes until a
* decision is made for whether the default presets should match the
* other presets or if they should be disabled by default in classic
* themes. See https://github.com/WordPress/gutenberg/issues/59989.
*/
$theme_support_data['settings']['shadow']['defaultPresets'] = false;

// Allow themes to enable link color setting via theme_support.
if ( current_theme_supports( 'link-color' ) ) {
$theme_support_data['settings']['color']['link'] = true;
Expand Down
3 changes: 1 addition & 2 deletions src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ public static function get_element_class_name( $element ) {
* @since 6.0.0
* @since 6.2.0 Added `dimensions.minHeight` and `position.sticky`.
* @since 6.4.0 Added `background.backgroundImage`.
* @since 6.5.0 Added `background.backgroundSize`, `dimensions.aspectRatio`, and `shadow.defaultPresets`.
* @since 6.5.0 Added `background.backgroundSize` and `dimensions.aspectRatio`.
* @var array
*/
const APPEARANCE_TOOLS_OPT_INS = array(
Expand All @@ -679,7 +679,6 @@ public static function get_element_class_name( $element ) {
array( 'spacing', 'margin' ),
array( 'spacing', 'padding' ),
array( 'typography', 'lineHeight' ),
array( 'shadow', 'defaultPresets' ),
ajlende marked this conversation as resolved.
Show resolved Hide resolved
);

/**
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@
"text": true
},
"shadow": {
"defaultPresets": false,
"defaultPresets": true,
"presets": [
{
"name": "Natural",
Expand Down
14 changes: 14 additions & 0 deletions tests/phpunit/data/themedir1/block-theme/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@
"padding": true,
"blockGap": true
},
"shadow": {
"presets": [
{
"name": "Natural",
"slug": "natural",
"shadow": "2px 2px 3px #000"
},
{
"name": "Test",
"slug": "test",
"shadow": "2px 2px 3px #000"
}
]
},
"blocks": {
"core/paragraph": {
"color": {
Expand Down
6 changes: 0 additions & 6 deletions tests/phpunit/tests/theme/wpThemeJson.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,6 @@ public function test_get_settings_appearance_true_opts_in() {
'typography' => array(
'lineHeight' => true,
),
'shadow' => array(
'defaultPresets' => true,
),
'blocks' => array(
'core/paragraph' => array(
'typography' => array(
Expand Down Expand Up @@ -334,9 +331,6 @@ public function test_get_settings_appearance_true_opts_in() {
'typography' => array(
'lineHeight' => false,
),
'shadow' => array(
'defaultPresets' => true,
),
),
),
);
Expand Down
75 changes: 75 additions & 0 deletions tests/phpunit/tests/theme/wpThemeJsonResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,4 +1070,79 @@
$actual_settings
);
}

/**
* @ticket 57545
oandregal marked this conversation as resolved.
Show resolved Hide resolved
*/
public function test_theme_shadow_presets_do_not_override_default_shadow_presets() {
switch_theme( 'block-theme' );

$theme_json_resolver = new WP_Theme_JSON_Resolver();
$theme_json = $theme_json_resolver->get_merged_data();
$actual_settings = $theme_json->get_settings()['shadow']['presets'];

$expected_settings = array(
'default'=> array(

Check failure on line 1085 in tests/phpunit/tests/theme/wpThemeJsonResolver.php

View workflow job for this annotation

GitHub Actions / PHP coding standards 

Expected 1 space before "=>"; 0 found
array(
'name' => 'Natural',
'shadow' => '6px 6px 9px rgba(0, 0, 0, 0.2)',
'slug' => 'natural',
),
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to assert specifically that the natural present has not been overriden by the Theme version from block-theme's theme.json.

The original bug related to the theme presets with the same slug overriding the core presets so it might be good to specifically assert to focus the test on this aspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also include a reference to the location of the related core preset?

wordpress-develop/src/wp-includes/theme.json

Lines 196 to 200 in f25ca58

{
"name": "Natural",
"slug": "natural",
"shadow": "6px 6px 9px rgba(0, 0, 0, 0.2)"
},

Copy link
Member

Choose a reason for hiding this comment

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

This value would never be overridden. What would happen instead is that theme array would have the natural preset. You may want to check by setting shadow.defaultPresets:false in core's theme.json and then execute npm run test:php -- --filter test_theme_shadow_presets_do_not_override_default_shadow_presets.

Note that I've also added a theme preset that is actually added (test) to make sure this is also working.

Copy link
Member

Choose a reason for hiding this comment

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

Essentially, presets work this way:

  1. Once the different theme.json sources (default, theme, user) are merged, all the values are available under the source key. For example:
array(
  'default' => array( /* come from core's theme.json */
    array( 'slug' => 'natural', /* ...*/ ),
    array( 'slug' => 'deep', /* ...*/ ),
  ),
  'theme'  => array( /* come from the theme's theme.json */
    array( 'slug' => 'natural', /* ...*/ ),
    array( 'slug' => 'test', /* ...*/ ),
  )
)
  1. However, in certain situations, we don't want the theme presets to "override" default presets, so we remove any whose slug is the same as one of the default presets. This is what happens for shadows, resulting in the following:
array(
  'default' => array( /* come from core's theme.json */
    array( 'slug' => 'natural', /* ...*/ ),
    array( 'slug' => 'deep', /* ...*/ ),
  ),
  'theme'  => array( /* come from theme's theme.json, after removing those whose slug is the same as any of the default presets */
    array( 'slug' => 'test', /* ...*/ ),
),
)

This is the base behavior for all presets. From there, this data is used differently by the components:

  • UI: some design tools may want list the presets from source (this is what colors do) but others display them all together (font families).
  • CSS: in general, every preset item available in the default and theme array will be converted to --wp--preset--category--slug (e.g.: --wp--preset--shadow-natural). This was the issue Alex raised and that is fixed by this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the additional clarification. I was understanding correctly, but perhaps not communicating that understanding very well.

When I say "overridden" I mean that the bug was that the Theme preset would take precedence over the Core preset.

Now with this PR the Core preset takes precedence over the Theme preset.

The condition is that they must have a matching slug. This is why you've added a "Test" preset that only exists in the Theme to verify that this functionality still works.

array(
'name' => 'Deep',
'shadow' => '12px 12px 50px rgba(0, 0, 0, 0.4)',
'slug' => 'deep',
),
array(
'name' => 'Sharp',
'shadow' => '6px 6px 0px rgba(0, 0, 0, 0.2)',
'slug' => 'sharp',
),
array(
'name' => 'Outlined',
'shadow' => '6px 6px 0px -3px rgba(255, 255, 255, 1), 6px 6px rgba(0, 0, 0, 1)',
'slug' => 'outlined',
),
array(
'name' => 'Crisp',
'shadow' => '6px 6px 0px rgba(0, 0, 0, 1)',
'slug' => 'crisp',
),
),
'theme' => array(
array(
'name' => 'Test',
'shadow' => '2px 2px 3px #000',
'slug' => 'test',
),
),
);

wp_recursive_ksort( $actual_settings );
wp_recursive_ksort( $expected_settings );

$this->assertSame(
$expected_settings,
$actual_settings
);
}

/**
* @ticket 57545
*/
oandregal marked this conversation as resolved.
Show resolved Hide resolved
public function test_shadow_default_presets_value_for_block_and_classic_themes() {
$theme_json_resolver = new WP_Theme_JSON_Resolver();
$theme_json = $theme_json_resolver->get_merged_data();

$defaultPresetsForClassicTheme = $theme_json->get_settings()['shadow']['defaultPresets'];
$this->assertFalse( $defaultPresetsForClassicTheme );

switch_theme( 'block-theme' );
$theme_json_resolver = new WP_Theme_JSON_Resolver();
$theme_json = $theme_json_resolver->get_merged_data();

$defaultPresetsForBlockTheme = $theme_json->get_settings()['shadow']['defaultPresets'];
$this->assertTrue( $defaultPresetsForBlockTheme );
}

}

Check failure on line 1148 in tests/phpunit/tests/theme/wpThemeJsonResolver.php

View workflow job for this annotation

GitHub Actions / PHP coding standards 

The closing brace for the class must go on the next line after the body
Loading