Duotone: Style Engine: Add unit test and associated refactoring #49033
Conversation
lib/block-supports/duotone.php
Outdated
| @@ -290,6 +290,59 @@ function gutenberg_tinycolor_string_to_rgb( $color_str ) { | |||
| } | |||
| } | |||
|
|
|||
| class WP_Duotone { | |||
There was a problem hiding this comment.
Not sure if there are some WordPress PHP style guidelines somewhere, but I always see classes in their own file named class-wp-something.php. Should we do the same for this class?
I also often see the Gutenberg version with a _Gutenberg suffix. In some places I think this is automatically added during the build process, but I don't think anything is done for lib. Do we need to include the suffix here so the name is WP_Duotone_Gutenberg?
| @@ -676,4 +676,35 @@ public function test_should_dedupe_and_merge_css_rules() { | |||
|
|
|||
| $this->assertSame( '.gandalf{color:white;height:190px;border-style:dotted;padding:10px;margin-bottom:100px;}.dumbledore,.rincewind{color:grey;height:90px;border-style:dotted;}', $compiled_stylesheet ); | |||
| } | |||
|
|
|||
| /** | |||
| * Tests returning a generated stylesheet from a set of duotone rules. | |||
There was a problem hiding this comment.
This is more of an integration test than a unit test for the style engine since it's testing both get_id_selector_property_and_maybe_svg and gutenberg_style_engine_get_stylesheet_from_css_rules. Is there a better place to put integration tests than mixed in with the unit tests for the style engine package?
There was a problem hiding this comment.
I updated the approach to just use strings.
lib/block-supports/duotone.php
Outdated
| @@ -290,6 +290,59 @@ function gutenberg_tinycolor_string_to_rgb( $color_str ) { | |||
| } | |||
| } | |||
|
|
|||
| class WP_Duotone { | |||
| static function get_id_selector_property_and_maybe_svg( $duotone_attr, $duotone_support ) { | |||
There was a problem hiding this comment.
When there are words like and and maybe in a function name, it means the function is probably doing too much. And in this case, it feels like that even more so because it generates four outputs, one of which may be null.
|
Flaky tests detected in b819d17. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4405744658
|
What?
This adds a unit test to the style engine to test that duotone properties are correctly output.
Why?
This catches a bug that was fixed in #49004.
How?
Call the style engine with static strings
Testing Instructions
npm run test:unit:php -- --filter 'WP_Block_Supports_Duotone_Test'
npm run test:unit:php -- --filter 'WP_Style_Engine_Test'
Question
Since this is really testing a fix in wp_kses, is this still a useful test to have?