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

Update conditional logic for editor_styles #3916

Closed
wants to merge 6 commits into from

Conversation

mikachan
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/57561


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@hellofromtonya
Copy link
Contributor

Tests for failing for

Tests_Dependencies_Styles::test_block_styles_for_editing_without_theme_support
Failed asserting that false is true.

for the last assertion in this test:

	public function test_block_styles_for_editing_without_theme_support() {
		// Confirm we are without theme support by default.
		$this->assertFalse( current_theme_supports( 'wp-block-styles' ) );

		wp_default_styles( $GLOBALS['wp_styles'] );

		$this->assertFalse( wp_style_is( 'wp-block-library-theme' ) );
		wp_enqueue_style( 'wp-edit-blocks' );
		$this->assertTrue( wp_style_is( 'wp-block-library-theme' ) );
	}

Looking at the code in this PR, the 'wp-block-library-theme' style is no longer enqueued if the theme doesn't support 'wp-block-styles'. This means the last assertion in this test should expect the state the state not to change, i.e. assertFalse().

In reading the intent of the original Gutenberg PR:

This PR is an attempt to never allow the wp-block-library-theme CSS to be loaded under the conditions we introduced in WordPress/gutenberg#44640.

this does appear to be the intent, i.e. if the theme doesn't support 'wp-block-styles', then wp_style_is( 'wp-block-library-theme' ) should be false.

I'll update the unit test accordingly.

@mikachan
Copy link
Member Author

Thank you for updating that test, @hellofromtonya 🙇‍♂️

if the theme doesn't support 'wp-block-styles', then wp_style_is( 'wp-block-library-theme' ) should be false.

This is correct 👍

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

Tested for both TT2 and TT3 https://core.trac.wordpress.org/ticket/57561#comment:13

  • Fixes the issue for TT3, a theme that does not add 'wp-block-styles' theme support ✅
  • Does not impact TT2, a theme that does add 'wp-block-styles' theme support ✅

This PR is ready for commit 👍

@hellofromtonya
Copy link
Contributor

@mikachan mikachan deleted the update/editor-styles-logic branch February 20, 2023 21:37
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