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: Fix color handling and inheritance #68351

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

Navigation Submenu: Fix color handling and inheritance #68351

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8cdd919
Navigation Submenu: Fix color handling and inheritance
yogeshbhutkar Dec 27, 2024
7d483df
Navigation Submenu: Improve color handling and inheritance logic
yogeshbhutkar Dec 31, 2024
0f0bb5e
Navigation Submenu: Enhance color attribute handling and fallback logic
yogeshbhutkar Jan 1, 2025
c2e99d1
Refactor: minor bug fixes
yogeshbhutkar Jan 1, 2025
7ff2e55
Tests: update order of wrapper attributes to fix failing tests
yogeshbhutkar Jan 1, 2025
faf394e
PHP Tests: Update conditional rendering of wrapper attributes to fix …
yogeshbhutkar Jan 1, 2025
57fb66e
PHP Tests: Fix inconsistent spacing between attributes
yogeshbhutkar Jan 1, 2025
2b4e452
Refactor: streamline color support handling in navigation submenu
yogeshbhutkar Jan 6, 2025
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
21 changes: 14 additions & 7 deletions packages/block-library/src/navigation-submenu/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,23 +283,30 @@ export default function NavigationSubmenuEdit( {
}
}

const _textColor = attributes.textColor ?? textColor;
const _customTextColor = attributes.style?.color?.text ?? customTextColor;
const _backgroundColor = attributes.backgroundColor ?? backgroundColor;
const _customBackgroundColor =
attributes.style?.color?.background ?? customBackgroundColor;

const blockProps = useBlockProps( {
ref: useMergeRefs( [ setPopoverAnchor, listItemRef ] ),
className: clsx( 'wp-block-navigation-item', {
'is-editing': isSelected || isParentOfSelectedBlock,
'is-dragging-within': isDraggingWithin,
'has-link': !! url,
'has-child': hasChildren,
'has-text-color': !! textColor || !! customTextColor,
[ getColorClassName( 'color', textColor ) ]: !! textColor,
'has-background': !! backgroundColor || customBackgroundColor,
[ getColorClassName( 'background-color', backgroundColor ) ]:
!! backgroundColor,
'has-text-color': !! _textColor || !! _customTextColor,
Copy link
Contributor

Choose a reason for hiding this comment

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

"textColor": {
	"type": "string"
},

Since its a string, I feel this is a little less readable on first glance.

Maybe adding a explicit check might be better?

'has-text-color': Boolean(_textColor || _customTextColor),

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 the Double Bang operator conveys the boolean nature and should provide sufficient context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, but the original included two comparisons, whereas this approach reduces it to just one.

[ getColorClassName( 'color', _textColor ) ]:
!! _textColor && ! _customTextColor,
'has-background': !! _backgroundColor || !! _customBackgroundColor,
[ getColorClassName( 'background-color', _backgroundColor ) ]:
!! _backgroundColor && ! _customBackgroundColor,
'open-on-click': openSubmenusOnClick,
} ),
style: {
color: ! textColor && customTextColor,
backgroundColor: ! backgroundColor && customBackgroundColor,
color: _customTextColor,
backgroundColor: _customBackgroundColor,
},
onKeyDown,
} );
Expand Down
50 changes: 44 additions & 6 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 @@ -99,6 +99,16 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {
$open_on_hover_and_click = isset( $block->context['openSubmenusOnClick'] ) && ! $block->context['openSubmenusOnClick'] &&
$show_submenu_indicators;

$block->block_type->supports['color'] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeated again on line 258, is there a reason we need to set twice?

Copy link
Contributor Author

@yogeshbhutkar yogeshbhutkar Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The repetition on line 258 could be omitted safely. Will be done in the next commit. Thanks for the suggestion.

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

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

$wrapper_attributes = get_block_wrapper_attributes(
array(
'class' => $css_classes . ' wp-block-navigation-item' . ( $has_submenu ? ' has-child' : '' ) .
Expand Down Expand Up @@ -203,6 +213,16 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {
}

if ( $has_submenu ) {
// These properties for submenus should only be applied from context.
// Values directly stored inside attributes should be applied to the parent block only.
unset(
$attributes['textColor'],
$attributes['backgroundColor'],
$attributes['customTextColor'],
$attributes['customBackgroundColor'],
$attributes['style']
);

// Copy some attributes from the parent block to this one.
// Ideally this would happen in the client when the block is created.
if ( array_key_exists( 'overlayTextColor', $block->context ) ) {
Expand All @@ -218,6 +238,22 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {
$attributes['style']['color']['background'] = $block->context['customOverlayBackgroundColor'];
}

// If there are no overlay colors provided, then inherit the colors from the parent block.
if ( ! isset( $attributes['textColor'] ) && ! isset( $attributes['style']['color']['text'] ) ) {
if ( array_key_exists( 'textColor', $block->context ) ) {
$attributes['textColor'] = $block->context['textColor'];
} elseif ( array_key_exists( 'customTextColor', $block->context ) ) {
$attributes['style']['color']['text'] = $block->context['customTextColor'];
}
}
if ( ! isset( $attributes['backgroundColor'] ) && ! isset( $attributes['style']['color']['background'] ) ) {
if ( array_key_exists( 'backgroundColor', $block->context ) ) {
$attributes['backgroundColor'] = $block->context['backgroundColor'];
} elseif ( array_key_exists( 'customBackgroundColor', $block->context ) ) {
$attributes['style']['color']['background'] = $block->context['customBackgroundColor'];
}
}

// This allows us to be able to get a response from wp_apply_colors_support.
$block->block_type->supports['color'] = true;
$colors_supports = wp_apply_colors_support( $block->block_type, $attributes );
Expand All @@ -244,12 +280,14 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {
$html = $tag_processor->get_updated_html();
}

$wrapper_attributes = get_block_wrapper_attributes(
array(
'class' => $css_classes,
'style' => $style_attribute,
)
);
$wrapper_attributes = array();
if ( ! empty( $style_attribute ) ) {
$wrapper_attributes[] = sprintf( 'style="%s"', $style_attribute );
}
if ( ! empty( $css_classes ) ) {
$wrapper_attributes[] = sprintf( 'class="%s"', $css_classes );
}
$wrapper_attributes = implode( ' ', $wrapper_attributes );

$html .= sprintf(
'<ul %s>%s</ul>',
Expand Down
Loading