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

Output theme palette presets when appearance tools are enabled #5816

Closed
wants to merge 8 commits into from
Closed

Output theme palette presets when appearance tools are enabled #5816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
039a650
Output theme palette presets when appearance tools are enabled
tellthemachines Dec 22, 2023
6f1be6a
Add test
tellthemachines Jan 3, 2024
fa22e67
Improve tests
tellthemachines Jan 3, 2024
3e179d1
Fix unwanted spaces
tellthemachines Jan 3, 2024
ec81eec
Remove another space
tellthemachines Jan 3, 2024
11f0858
Make sure added theme supports are removed at teardown.
tellthemachines Jan 4, 2024
dfd3bb9
Remove theme support locally in each test.
tellthemachines Jan 9, 2024
b9c1d89
Revert "Remove theme support locally in each test."
tellthemachines Jan 9, 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
8 changes: 7 additions & 1 deletion src/wp-includes/global-styles-and-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,13 @@ function wp_get_global_stylesheet( $types = array() ) {
* @see wp_add_global_styles_for_blocks
*/
$origins = array( 'default', 'theme', 'custom' );
if ( ! $supports_theme_json ) {
/*
* If the theme doesn't have theme.json but supports both appearance tools and color palette,
* the 'theme' origin should be included so color palette presets are also output.
*/
Comment on lines +225 to +228
Copy link
Member

Choose a reason for hiding this comment

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

@tellthemachines This needs to fixed.

/*
 * text
 */

if ( ! $supports_theme_json && ( current_theme_supports( 'appearance-tools' ) || current_theme_supports( 'border' ) ) && current_theme_supports( 'editor-color-palette' ) ) {
$origins = array( 'default', 'theme' );
} elseif ( ! $supports_theme_json ) {
$origins = array( 'default' );
}
$styles_rest = $tree->get_stylesheet( $types, $origins );
Expand Down
3 changes: 3 additions & 0 deletions tests/phpunit/tests/rest-api/rest-themes-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ public static function wpTearDownAfterClass() {
self::delete_user( self::$subscriber_id );
self::delete_user( self::$contributor_id );
self::delete_user( self::$admin_id );

remove_theme_support( 'editor-gradient-presets' );
remove_theme_support( 'editor-color-palette' );
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 was unaware of this until now, but added theme support for a feature in a test will persist in other tests even across theme switches, unless explicitly removed.

The remove_theme_support( 'editor-color-palette' ); in the teardown function of WpGetGlobalStylesheet below was causing a failure in one of the wpThemeJsonResolver tests, because the theme support for 'editor-gradient-presets' added in this test was changing the order of the color settings output from theme.json, which gets merged with theme supports here. I decided to remove them both explicitly in this test to avoid future issues.

Copy link
Member

Choose a reason for hiding this comment

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

Does it work if we call remove_theme_support at the end of the test?

So like this:

	public function test_theme_supports_editor_gradient_presets_array() {
		$gradient = array(
			'name'     => __( 'Vivid cyan blue to vivid purple', 'themeLangDomain' ),
			'gradient' => 'linear-gradient(135deg,rgba(6,147,227,1) 0%,rgb(155,81,224) 100%)',
			'slug'     => 'vivid-cyan-blue-to-vivid-purple',
		);
		add_theme_support( 'editor-gradient-presets', array( $gradient ) );
		$response = self::perform_active_theme_request();
		$result   = $response->get_data();
		$this->assertArrayHasKey( 'theme_supports', $result[0] );
		$this->assertSame( array( $gradient ), $result[0]['theme_supports']['editor-gradient-presets'] );
		remove_theme_support( 'editor-gradient-presets' );
	}

No big deal, happy with whatever works, it's just that usually wpTearDownAfterClass only contains the "mirror image" of what's in set_up or wpSetUpBeforeClass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should work! Lemme try that

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question, what happens in PHPUnit if you call a function after a failing assertion? Would remove_theme_support still get called? If not, then we might want to have remove_theme_support be called just before the first assertion so that the absence of calling remove_theme_support doesn't affect other tests 🤔

My main thinking there is so that if this test "really" fails, and someone's trying to investigate, they don't get distracted by another failing test that's only failing if this test didn't manage to do all the cleanup needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question! I had to go and try it out 😄 and can confirm a failing assertion means the rest of the block won't get executed.

I guess that's a good reason to not remove_theme_support inline then. I'd better revert the latest commit.

}

/**
Expand Down
71 changes: 71 additions & 0 deletions tests/phpunit/tests/theme/wpGetGlobalStylesheet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ class Tests_Theme_WpGetGlobalStylesheet extends WP_Theme_UnitTestCase {
*/
private $remove_theme_support_at_teardown = false;

/**
* Flag to indicate whether to remove 'border' theme support at tear_down().
*
* @var bool
*/
private $remove_border_support_at_teardown = false;

/**
* Flag to indicate whether to switch back to the default theme at tear down.
*
Expand All @@ -40,6 +47,12 @@ public function tear_down() {
switch_theme( WP_DEFAULT_THEME );
}

if ( $this->remove_border_support_at_teardown ) {
$this->remove_border_support_at_teardown = false;
remove_theme_support( 'border' );
remove_theme_support( 'editor-color-palette' );
}

parent::tear_down();
}

Expand Down Expand Up @@ -246,6 +259,64 @@ public function test_caching_is_used_when_developing_theme() {
$this->assertNotSame( $css, wp_get_global_stylesheet(), 'Caching was used despite theme development mode' );
}

/**
* Tests that theme color palette presets are output when appearance tools are enabled via theme support.
*
* @ticket 60134
*/
public function test_theme_color_palette_presets_output_when_border_support_enabled() {

$args = array(
array(
'name' => 'Black',
'slug' => 'nice-black',
'color' => '#000000',
),
array(
'name' => 'Dark Gray',
'slug' => 'dark-gray',
'color' => '#28303D',
),
array(
'name' => 'Green',
'slug' => 'haunted-green',
'color' => '#D1E4DD',
),
array(
'name' => 'Blue',
'slug' => 'soft-blue',
'color' => '#D1DFE4',
),
array(
'name' => 'Purple',
'slug' => 'cool-purple',
'color' => '#D1D1E4',
),
);

// Add theme support for appearance tools.
add_theme_support( 'border' );
add_theme_support( 'editor-color-palette', $args );
$this->remove_border_support_at_teardown = true;
Copy link
Member

@noisysocks noisysocks Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to my other comment, it would be a lot simpler (no need for the member variable) if we can call remove_theme_support() at the end of the test.

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 was following the pattern of $remove_theme_support_at_teardown in this file. I agree simpler is better if there are no negative consequences to removing supports added per test.

Copy link
Member

@noisysocks noisysocks Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would prefer to remove that as well if it's not necessary. Try it and see if tests pass ✅


// Check for both the variable declaration and its use as a value.
$variables = wp_get_global_stylesheet( array( 'variables' ) );

$this->assertStringContainsString( '--wp--preset--color--nice-black: #000000', $variables );
$this->assertStringContainsString( '--wp--preset--color--dark-gray: #28303D', $variables );
$this->assertStringContainsString( '--wp--preset--color--haunted-green: #D1E4DD', $variables );
$this->assertStringContainsString( '--wp--preset--color--soft-blue: #D1DFE4', $variables );
$this->assertStringContainsString( '--wp--preset--color--cool-purple: #D1D1E4', $variables );

$presets = wp_get_global_stylesheet( array( 'presets' ) );

$this->assertStringContainsString( 'var(--wp--preset--color--nice-black)', $presets );
$this->assertStringContainsString( 'var(--wp--preset--color--dark-gray)', $presets );
$this->assertStringContainsString( 'var(--wp--preset--color--haunted-green)', $presets );
$this->assertStringContainsString( 'var(--wp--preset--color--soft-blue)', $presets );
$this->assertStringContainsString( 'var(--wp--preset--color--cool-purple)', $presets );
}

/**
* Adds the 'editor-font-sizes' theme support with custom font sizes.
*
Expand Down
Loading