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

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

Merged
merged 10 commits into from
Mar 23, 2023
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
17f38a2
Navigation Submenu: Remove needed code
scruffian Mar 8, 2023
996abf1
php lint
scruffian Mar 9, 2023
68526bd
Create test scaffold from Nav Link block tests
getdave Mar 21, 2023
44fd29c
Scaffold out first colors test
getdave Mar 21, 2023
02b199b
Assert color applied from custom colors
getdave Mar 21, 2023
99bc885
Validate classes not applied if not present
getdave Mar 21, 2023
bb098ed
Test for mixed custom and presets
getdave Mar 21, 2023
4807578
Experiment with utilising WP_HTML_Tag_Processor to improve tests
getdave Mar 21, 2023
168a906
Remove unused code
getdave Mar 21, 2023
67f9d9c
rename variables
scruffian Mar 22, 2023
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
103 changes: 28 additions & 75 deletions packages/block-library/src/navigation-submenu/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love it that we can ditch this custom code 🥳

$colors = array(
'css_classes' => array(),
'inline_styles' => '',
);

// Text color.
$named_text_color = null;
$custom_text_color = null;

if ( $is_sub_menu && array_key_exists( 'customOverlayTextColor', $context ) ) {
$custom_text_color = $context['customOverlayTextColor'];
} elseif ( $is_sub_menu && array_key_exists( 'overlayTextColor', $context ) ) {
$named_text_color = $context['overlayTextColor'];
} elseif ( array_key_exists( 'customTextColor', $context ) ) {
$custom_text_color = $context['customTextColor'];
} elseif ( array_key_exists( 'textColor', $context ) ) {
$named_text_color = $context['textColor'];
} elseif ( isset( $context['style']['color']['text'] ) ) {
$custom_text_color = $context['style']['color']['text'];
}

// If has text color.
if ( ! is_null( $named_text_color ) ) {
// Add the color class.
array_push( $colors['css_classes'], 'has-text-color', sprintf( 'has-%s-color', $named_text_color ) );
} elseif ( ! is_null( $custom_text_color ) ) {
// Add the custom color inline style.
$colors['css_classes'][] = 'has-text-color';
$colors['inline_styles'] .= sprintf( 'color: %s;', $custom_text_color );
}

// Background color.
$named_background_color = null;
$custom_background_color = null;

if ( $is_sub_menu && array_key_exists( 'customOverlayBackgroundColor', $context ) ) {
$custom_background_color = $context['customOverlayBackgroundColor'];
} elseif ( $is_sub_menu && array_key_exists( 'overlayBackgroundColor', $context ) ) {
$named_background_color = $context['overlayBackgroundColor'];
} elseif ( array_key_exists( 'customBackgroundColor', $context ) ) {
$custom_background_color = $context['customBackgroundColor'];
} elseif ( array_key_exists( 'backgroundColor', $context ) ) {
$named_background_color = $context['backgroundColor'];
} elseif ( isset( $context['style']['color']['background'] ) ) {
$custom_background_color = $context['style']['color']['background'];
}

// If has background color.
if ( ! is_null( $named_background_color ) ) {
// Add the background-color class.
array_push( $colors['css_classes'], 'has-background', sprintf( 'has-%s-background-color', $named_background_color ) );
} elseif ( ! is_null( $custom_background_color ) ) {
// Add the custom background-color inline style.
$colors['css_classes'][] = 'has-background';
$colors['inline_styles'] .= sprintf( 'background-color: %s;', $custom_background_color );
}

return $colors;
}

/**
* Build an array with CSS classes and inline styles defining the font sizes
Expand Down Expand Up @@ -129,7 +69,6 @@ function block_core_navigation_submenu_render_submenu_icon() {
* @return string Returns the post content with the legacy widget added.
*/
function render_block_core_navigation_submenu( $attributes, $content, $block ) {

$navigation_link_has_id = isset( $attributes['id'] ) && is_numeric( $attributes['id'] );
$is_post_type = isset( $attributes['kind'] ) && 'post-type' === $attributes['kind'];
$is_post_type = $is_post_type || isset( $attributes['type'] ) && ( 'post' === $attributes['type'] || 'page' === $attributes['type'] );
Expand All @@ -144,15 +83,10 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {
return '';
}

$colors = block_core_navigation_submenu_build_css_colors( $block->context, $attributes );
$font_sizes = block_core_navigation_submenu_build_css_font_sizes( $block->context );
$classes = array_merge(
$colors['css_classes'],
$font_sizes['css_classes']
);
$style_attribute = ( $colors['inline_styles'] . $font_sizes['inline_styles'] );
$style_attribute = $font_sizes['inline_styles'];

$css_classes = trim( implode( ' ', $classes ) );
$css_classes = trim( implode( ' ', $font_sizes['css_classes'] ) );
$has_submenu = count( $block->inner_blocks ) > 0;
$is_active = ! empty( $attributes['id'] ) && ( get_queried_object_id() === (int) $attributes['id'] );

Expand Down Expand Up @@ -249,14 +183,33 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {
}

if ( $has_submenu ) {
$colors = block_core_navigation_submenu_build_css_colors( $block->context, $attributes, $has_submenu );
$classes = array_merge(
array( 'wp-block-navigation__submenu-container' ),
$colors['css_classes']
);
$css_classes = trim( implode( ' ', $classes ) );
// Copy some attributes from the parent block to this one.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// Ideally this would happen in the client when the block is created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it ideal?

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 thinking that then the sub-menu block would be the one to control it, rather than being orchestrated by the parent.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow up then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#49313

if ( array_key_exists( 'overlayTextColor', $block->context ) ) {
$attributes['textColor'] = $block->context['overlayTextColor'];
}
if ( array_key_exists( 'overlayBackgroundColor', $block->context ) ) {
$attributes['backgroundColor'] = $block->context['overlayBackgroundColor'];
}
if ( array_key_exists( 'customOverlayTextColor', $block->context ) ) {
$attributes['style']['color']['text'] = $block->context['customOverlayTextColor'];
}
if ( array_key_exists( 'customOverlayBackgroundColor', $block->context ) ) {
$attributes['style']['color']['background'] = $block->context['customOverlayBackgroundColor'];
}

$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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to modify this?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

$colors_supports = gutenberg_apply_colors_support( $block->block_type, $attributes );
$css_classes = 'wp-block-navigation__submenu-container';
if ( array_key_exists( 'class', $colors_supports ) ) {
$css_classes .= ' ' . $colors_supports['class'];
}

$style_attribute = '';
if ( array_key_exists( 'style', $colors_supports ) ) {
$style_attribute = $colors_supports['style'];
}

$inner_blocks_html = '';
foreach ( $block->inner_blocks as $inner_block ) {
Expand Down
213 changes: 213 additions & 0 deletions phpunit/blocks/render-block-navigation-submenu-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
<?php
/**
* Tests server side rendering of core/navigation-submenu
*
* @package Gutenberg
* @subpackage block-library
*/

/**
* Tests for various cases in Navigation Submenu rendering
*/
class Render_Block_Navigation_Submenu_Test extends WP_UnitTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

@scruffian What do you think of the test coverage?

Copy link
Contributor Author

@scruffian scruffian Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

private static $category;
private static $page;
private static $draft;
private static $custom_draft;
private static $custom_post;


/**
* @var array|null
*/
private $original_block_supports;

public static function set_up_before_class() {
self::$page = self::factory()->post->create_and_get(
array(
'post_type' => 'page',
'post_status' => 'publish',
'post_name' => 'tabby',
'post_title' => 'Tabby cats',
'post_content' => 'Tabby cat content',
'post_excerpt' => 'Tabby cat',
)
);
}

public function set_up() {
parent::set_up();

$this->original_block_supports = WP_Block_Supports::$block_to_render;
WP_Block_Supports::$block_to_render = array(
'attrs' => array(),
'blockName' => '',
);
}

public function tear_down() {
WP_Block_Supports::$block_to_render = $this->original_block_supports;
parent::tear_down();
}

/**
* @group submenu-color-inheritance
* @covers ::gutenberg_render_block_core_navigation_submenu
*/
public function test_should_apply_preset_colors_inherited_from_parent_block_via_context() {
$page_id = self::$page->ID;

$parsed_blocks = parse_blocks(
'<!-- wp:navigation-submenu {"label":"Submenu Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} -->
<!-- wp:navigation-link {"label":"Submenu Item Link Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} /-->
<!-- /wp:navigation-submenu -->'
);

$this->assertEquals( 1, count( $parsed_blocks ), 'Submenu block not parsable.' );

$block = $parsed_blocks[0];

// Colors inherited from parent Navigation block.
$context = array(
'overlayTextColor' => 'purple',
'overlayBackgroundColor' => 'yellow',
);

$navigation_link_block = new WP_Block( $block, $context );
draganescu marked this conversation as resolved.
Show resolved Hide resolved

$rendered_html = gutenberg_render_block_core_navigation_submenu(
$navigation_link_block->attributes,
array(),
$navigation_link_block
);

$p = new WP_HTML_Tag_Processor( $rendered_html );
draganescu marked this conversation as resolved.
Show resolved Hide resolved
$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'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/**
* @group submenu-color-inheritance
* @covers ::gutenberg_render_block_core_navigation_submenu
*/
public function test_should_apply_custom_colors_inherited_from_parent_block_via_context() {
$page_id = self::$page->ID;

$parsed_blocks = parse_blocks(
'<!-- wp:navigation-submenu {"label":"Submenu Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} -->
<!-- wp:navigation-link {"label":"Submenu Item Link Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} /-->
<!-- /wp:navigation-submenu -->'
);

$this->assertEquals( 1, count( $parsed_blocks ), 'Submenu block not parsable.' );

$block = $parsed_blocks[0];

// Colors inherited from parent Navigation block.
$context = array(
'customOverlayTextColor' => '#BCC60A',
'customOverlayBackgroundColor' => '#E10E0E',
);

$navigation_link_block = new WP_Block( $block, $context );

$this->assertStringContainsString(
'<ul style="color:' . $context['customOverlayTextColor'] . ';background-color:' . $context['customOverlayBackgroundColor'] . ';" class="wp-block-navigation__submenu-container has-text-color has-background">',
gutenberg_render_block_core_navigation_submenu(
$navigation_link_block->attributes,
array(),
$navigation_link_block
),
'Submenu block colors inherited from context not applied correctly'
);
}

/**
* @group submenu-color-inheritance
* @covers ::gutenberg_render_block_core_navigation_submenu
*/
public function test_should_apply_mix_of_preset_and_custom_colors_inherited_from_parent_block_via_context() {
$page_id = self::$page->ID;

$parsed_blocks = parse_blocks(
'<!-- wp:navigation-submenu {"label":"Submenu Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} -->
<!-- wp:navigation-link {"label":"Submenu Item Link Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} /-->
<!-- /wp:navigation-submenu -->'
);

$this->assertEquals( 1, count( $parsed_blocks ), 'Submenu block not parsable.' );

$block = $parsed_blocks[0];

// Colors inherited from parent Navigation block.
$context = array(
'overlayTextColor' => 'purple',
'customOverlayBackgroundColor' => '#E10E0E',
);

$navigation_link_block = new WP_Block( $block, $context );

$this->assertStringContainsString(
'<ul style="background-color:' . $context['customOverlayBackgroundColor'] . ';" class="wp-block-navigation__submenu-container has-text-color has-' . $context['overlayTextColor'] . '-color has-background">',
gutenberg_render_block_core_navigation_submenu(
$navigation_link_block->attributes,
array(),
$navigation_link_block
),
'Submenu block colors inherited from context not applied correctly'
);
}

/**
* @group submenu-color-inheritance
* @covers ::gutenberg_render_block_core_navigation_submenu
*/
public function test_should_not_apply_custom_colors_if_missing_from_context() {
$page_id = self::$page->ID;

$parsed_blocks = parse_blocks(
'<!-- wp:navigation-submenu {"label":"Submenu Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} -->
<!-- wp:navigation-link {"label":"Submenu Item Link Label","type":"page","id":' . $page_id . ',"url":"http://localhost:8888/?page_id=' . $page_id . '","kind":"post-type"} /-->
<!-- /wp:navigation-submenu -->'
);

$this->assertEquals( 1, count( $parsed_blocks ), 'Submenu block not parsable.' );

$block = $parsed_blocks[0];

// Intentionally empty - no colors.
$context = array();

$navigation_link_block = new WP_Block( $block, $context );

$actual = gutenberg_render_block_core_navigation_submenu(
$navigation_link_block->attributes,
array(),
$navigation_link_block
);

$this->assertStringContainsString(
'<ul class="wp-block-navigation__submenu-container">',
$actual,
'Submenu block should not apply colors if missing from context'
);

$this->assertStringNotContainsString(
$actual,
'has-text-color has-background',
'Submenu block should not apply "has-*" color classes if missing from context'
);

}
}