Navigation Submenu: Refactor to use the block supports function #48936

scruffian · 2023-03-08T17:34:06Z

What?

This takes the custom implementation of color supports from the navigation submenu block and changes it to use gutenberg_apply_colors_support from the color supports.

Why?

This is the first step towards using standard color supports for the navigation block, rather than having a custom implementation. This will help the block to behave in a more predictable way, eliminate bugs and take advantage of tools in Global Styles.

How?

We have to modify the block_type and block_attributes, so that the block supports work as expected. I'm not sure that is an acceptable workaround. Long term we should actually enable this block support, but we need to migrate the colors from the navigation block attribute to the submenu block which is complex. This begins the process of unifying the approach in a simple way.

Testing Instructions

Run the tests:

npm run test:unit:php -- --filter Render_Block_Navigation_Submenu_Test

Set text and background preset colors on the overlay for the Navigation block
Check that the colors on the frontend match those set in the editor.
Do the same for custom colors.

Screenshots or screencast

scruffian · 2023-03-08T17:40:56Z

packages/block-library/src/navigation-submenu/index.php

+		}
+
+		// This allows us to be able to get a response from gutenberg_apply_colors_support.
+		$block->block_type->supports['color'] = true;


Is it ok to modify this?

draganescu · 2023-03-14T14:12:43Z

I have tested this and the colors do not apply on the front end.

Backend

Frontend

getdave

Test both Preset and Custom colors and this worked as described in both the Site and the Post Editor.

Screen.Capture.on.2023-03-21.at.15-28-32.mp4

Screen.Capture.on.2023-03-21.at.15-29-56.mp4

getdave

Left some notes and questions. For me the biggest thing missing here is addition of tests. We should/could write some.

getdave · 2023-03-21T15:32:51Z

packages/block-library/src/navigation-submenu/index.php

@@ -14,66 +14,6 @@
 * @param  bool  $is_sub_menu Whether the block is a sub-menu.
 * @return array Colors CSS classes and inline styles.
 */
-function block_core_navigation_submenu_build_css_colors( $context, $attributes, $is_sub_menu = false ) {


I love it that we can ditch this custom code 🥳

getdave · 2023-03-21T15:34:01Z

packages/block-library/src/navigation-submenu/index.php

-			$colors['css_classes']
-		);
-		$css_classes = trim( implode( ' ', $classes ) );
+		// Copy some attributes from the parent block to this one.


This is unusual code so should we be more specific about what's happening here?

getdave · 2023-03-21T15:34:09Z

packages/block-library/src/navigation-submenu/index.php

-		);
-		$css_classes = trim( implode( ' ', $classes ) );
+		// Copy some attributes from the parent block to this one.
+		// Ideally this would happen in the client when the block is created.


Why is it ideal?

I was thinking that then the sub-menu block would be the one to control it, rather than being orchestrated by the parent.

I'm not sure. The reason it's orchestrated by the parent is so you can set the colors for all children in a single place rather than have to visit every single child block and configure the colors.

We could make it so changing one child block's colors changes all of them but I think that's very confusing.

I would consider leaving the controls in place on the Nav block but allowing the values set their to determine the defaults for controls we expose on the child blocks.

That means we still have a single place to se the colors.

Let me know if I've misunderstood here.

I think what I envisage is that the controls remain on the parent block, but when they are set, the block updates the child blocks in the client, so that the block itself handles all of the logic about the display. That way nothing about the submenu blocks is stored in the parent block, its just a UI layer which allows the user to update multiple blocks at once.

Follow up then?

Absolutely.

getdave · 2023-03-21T15:34:44Z

packages/block-library/src/navigation-submenu/index.php


-		$style_attribute = $colors['inline_styles'];
+		// This allows us to be able to get a response from gutenberg_apply_colors_support.
+		$block->block_type->supports['color'] = true;


Why not rely on the actual block supports. If someone decides to disable block supports for this block then we should respect that.

If we do that then users will be able to modify the settings for the submenu block in the UI, independently from the parent block. I think what we need to do is move the color settings from the navigation block to the submenu block, which will require a UI update and a block transform, and a lot of thinking about backwards compatibility. I do think we should do all of that, but if I had done it in this PR you'd be telling me it was too big, so this is just a first step toward that goal.

Ok sure. I agree with small PRs.

I was thinking that as $block->block_type->supports['color'] = true; appears to indicate the supports is a Boolean we could just rely on that Boolean being set in the block.json for Submenu. But now I think I understand that if you did that the UI for colors would appear in the Submenu block - which isn't what we want.

So yes I think this workaround is an ok tradeoff for being able to utilise the standardised color code generation.

What's the long term plan? Expose UI in the Submenu Block to allow changing colors on individual blocks but have the initial settings still coming from the parent Nav block?

What's the long term plan? Expose UI in the Submenu Block to allow changing colors on individual blocks but have the initial settings still coming from the parent Nav block?

I agree that we still need a way to set it on the Nav block so that you don't need to change it for every child, but I've not given much thought to how that would work in practice yet.

I had the same gut reaction initially and even tried to code a variant that just relies on block supports, but we do have some blockers there. However:

users will be able to modify the settings for the submenu block in the UI, independently from the parent block

@jasmussen is that a problem really?

getdave · 2023-03-21T17:28:57Z

phpunit/blocks/render-block-navigation-submenu-test.php

+		$p = new WP_HTML_Tag_Processor( $rendered_html );
+		$p->next_tag(
+			array(
+				'tag_name'   => 'ul',
+				'class_name' => 'wp-block-navigation__submenu-container',
+			)
+		);
+		$p->get_attribute( 'class' );
+
+		$this->assertEquals(
+			'wp-block-navigation__submenu-container has-text-color has-purple-color has-background has-yellow-background-color',
+			$p->get_attribute( 'class' ),
+			'Submenu block colors inherited from context not applied correctly'
+		);


I tried using WP_HTML_Tag_Processor to improve the readability of the tests. It seems better to use this to assert on the HTML rather than assertStringContainsString which is very simplistic.

Open to opinions on whether this is a good idea.

What's the worry about the more simplistic approach?

Is it important that we have the test care about equaling all of those classnames in that order? Or should the classname include all 5 of the classes but in any order? If it's not important that it only equals that, it could be nice to check that it contains the classnames so it doesn't fail unnecessarily.

I'm not versed in the block nav though, so I'm not sure what the main considerations are.

What's the worry about the more simplistic approach?

If the test fails it's quite difficult to see the difference between the two strings. Where the output for equals seemed clearer.

Also the Tag Processor makes it clearer we are asserting directly on the className rather than on any other portion of the <ul> HTML which could change and cause unrelated failures.

getdave · 2023-03-21T17:30:15Z

@scruffian I added some tests cases specifically to test for color supports.

Can be run with

npm run test:unit:php -- --filter Render_Block_Navigation_Submenu_Test

I feel like they could be extended to use a @dataProvider to test multiple variants but it would require abstracting the code of the tests themselves and at that point it's becoming quite complex to reason about the tests which I dislike as an anti-pattern.

Let me know what you think?

getdave · 2023-03-22T15:18:07Z

phpunit/blocks/render-block-navigation-submenu-test.php

+/**
+ * Tests for various cases in Navigation Submenu rendering
+ */
+class Render_Block_Navigation_Submenu_Test extends WP_UnitTestCase {


@scruffian What do you think of the test coverage?

Should we use the navigation block itself in the tests rather than the navigation submenu block? I think I prefer using the submenu block as you have done, as it feels more inline with the future direction of the block.

~~The coverage looks good, but I have two test failures~~
Edit: This was an environment issue.

phpunit/blocks/render-block-navigation-submenu-test.php

scruffian · 2023-03-22T20:53:09Z

I updated some of the variables. I'm happy to merge this if someone else can give me a ✅

github-actions · 2023-03-22T21:20:09Z

Flaky tests detected in 67f9d9c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4494361988
📝 Reported issues:

[Flaky Test] allows tabbing to the inspector from the last block #48504 in /test/e2e/specs/site-editor/writing-flow.spec.js
[Flaky Test] shows changes in a template when a template part it contains is modified #42139 in /test/e2e/specs/site-editor/template-part.spec.js

adamziel · 2023-03-23T09:39:48Z

phpunit/blocks/render-block-navigation-submenu-test.php

+			$navigation_submenu_block
+		);
+
+		$tags = new WP_HTML_Tag_Processor( $rendered_html );


getdave

LGTM. We have tested this manually but I'd feel a lot more comfortable if we'd had tests to assert nothing changed.

At least we now have some tests for this block going forward.

Thanks for working on this.

If there are next steps (e.g. expose child controls on parent) then let's create Issues now to track them.

…Press/gutenberg#48936 in favour of wp_apply_colors_support this commit adds a deprecation to core

The doc block wasn't

…52152) The doc block wasn't

Marc-pi · 2023-06-30T07:11:55Z

@ramonjd the milestone is wrong ;-)

ramonjd · 2023-06-30T07:19:01Z

the milestone is wrong ;-)

Wait, where? 😄

Marc-pi · 2023-06-30T07:20:01Z

ramonjd · 2023-06-30T07:21:55Z

I think that's right? This is PR was merged Mar 24, 2023. All I've done is remove the doc block that isn't being used (today).

Marc-pi · 2023-06-30T07:23:52Z

ok, my mistake, i saw this in the commit feed, i was suprised

ramonjd · 2023-06-30T07:36:50Z

All good, thank you anyway! 🙇🏻

Navigation Submenu: Remove needed code

17f38a2

scruffian added the [Block] Navigation Affects the Navigation Block label Mar 8, 2023

scruffian requested a review from tellthemachines as a code owner March 8, 2023 17:34

scruffian self-assigned this Mar 8, 2023

scruffian mentioned this pull request Mar 8, 2023

Navigation: Remove duplicate css color building functions #48700

Closed

scruffian commented Mar 8, 2023

View reviewed changes

php lint

996abf1

getdave reviewed Mar 21, 2023

View reviewed changes

getdave added 7 commits March 21, 2023 15:59

Create test scaffold from Nav Link block tests

68526bd

Scaffold out first colors test

44fd29c

Assert color applied from custom colors

02b199b

Validate classes not applied if not present

99bc885

Test for mixed custom and presets

bb098ed

Experiment with utilising WP_HTML_Tag_Processor to improve tests

4807578

Remove unused code

168a906

getdave reviewed Mar 21, 2023

View reviewed changes

getdave reviewed Mar 22, 2023

View reviewed changes

scruffian commented Mar 22, 2023

View reviewed changes

phpunit/blocks/render-block-navigation-submenu-test.php Outdated Show resolved Hide resolved

scruffian commented Mar 22, 2023

View reviewed changes

phpunit/blocks/render-block-navigation-submenu-test.php Outdated Show resolved Hide resolved

rename variables

67f9d9c

adamziel reviewed Mar 23, 2023

View reviewed changes

getdave approved these changes Mar 23, 2023

View reviewed changes

scruffian mentioned this pull request Mar 23, 2023

Navigation Submenu: Add color supports #49313

Open

scruffian merged commit 88e97c4 into trunk Mar 23, 2023

scruffian deleted the remove/unused-code-from-navigation-submenu branch March 23, 2023 15:58

github-actions bot added this to the Gutenberg 15.5 milestone Mar 23, 2023

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jun 30, 2023

block_core_navigation_submenu_build_css_colors was deprecated in Word…

4c596f0

…Press/gutenberg#48936 in favour of wp_apply_colors_support this commit adds a deprecation to core

ramonjd added a commit that referenced this pull request Jun 30, 2023

block_core_navigation_submenu_build_css_colors was removed in #48936

948ac38

The doc block wasn't

This was referenced Jun 30, 2023

Navigation submenu: remove unused doc block #52152

Merged

Navigation submenu block: deprecate deleted block_core_navigation_submenu_build_css_colors function WordPress/wordpress-develop#4762

Closed

ramonjd added a commit that referenced this pull request Jun 30, 2023

block_core_navigation_submenu_build_css_colors was removed in #48936 (#…

5d2fd62

…52152) The doc block wasn't

manooweb mentioned this pull request Jul 3, 2023

Don't remove 'block_core_navigation_link_build_css_colors()' without a deprecation #52244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation Submenu: Refactor to use the block supports function #48936

Navigation Submenu: Refactor to use the block supports function #48936

scruffian commented Mar 8, 2023 •

edited by getdave

Loading

scruffian Mar 8, 2023

draganescu commented Mar 14, 2023

getdave left a comment

getdave left a comment

getdave Mar 21, 2023

getdave Mar 21, 2023

getdave Mar 21, 2023

scruffian Mar 22, 2023

getdave Mar 22, 2023

scruffian Mar 22, 2023

getdave Mar 23, 2023

scruffian Mar 23, 2023

scruffian Mar 23, 2023

getdave Mar 21, 2023

scruffian Mar 22, 2023

getdave Mar 22, 2023

scruffian Mar 22, 2023

draganescu Mar 23, 2023

getdave Mar 21, 2023

jeryj Mar 21, 2023

getdave Mar 22, 2023

getdave commented Mar 21, 2023 •

edited

Loading

getdave Mar 22, 2023

scruffian Mar 22, 2023 •

edited

Loading

scruffian commented Mar 22, 2023

github-actions bot commented Mar 22, 2023

adamziel Mar 23, 2023

getdave left a comment

Marc-pi commented Jun 30, 2023

ramonjd commented Jun 30, 2023

Marc-pi commented Jun 30, 2023

ramonjd commented Jun 30, 2023

Marc-pi commented Jun 30, 2023

ramonjd commented Jun 30, 2023

Navigation Submenu: Refactor to use the block supports function #48936

Navigation Submenu: Refactor to use the block supports function #48936

Conversation

scruffian commented Mar 8, 2023 • edited by getdave Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Choose a reason for hiding this comment

draganescu commented Mar 14, 2023

Backend

Frontend

getdave left a comment

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

scruffian Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

scruffian commented Mar 22, 2023

github-actions bot commented Mar 22, 2023

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

Marc-pi commented Jun 30, 2023

ramonjd commented Jun 30, 2023

Marc-pi commented Jun 30, 2023

ramonjd commented Jun 30, 2023

Marc-pi commented Jun 30, 2023

ramonjd commented Jun 30, 2023

scruffian commented Mar 8, 2023 •

edited by getdave

Loading

getdave commented Mar 21, 2023 •

edited

Loading

scruffian Mar 22, 2023 •

edited

Loading