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

Changes from 2 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