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.

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

Conversation

yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented Dec 27, 2024

What, Why & How?

Testing Instructions for Keyboard

  1. Navigate to a Post/Page edit screen.
  2. Create a Navigation Block.
  3. Try setting up different permutations for text and background properties along with custom colors.
  4. Notice the bugs mentioned in the issue are now fixed.

Screencasts

The following are the expected outcomes that the submenu block should abide by.

If the navigation block has either a background or a text color assigned, and there's no color provided for the submenu & overlay text or submenu & overlay background, then navigation blocks color should be inherited. ( ✅ )

one

If the navigation block has specified a color, but, the submenu parent has also specified a color (background and text ), the submenu container should still inherit the color from the navigation block. ( ✅ )

two

If submenu & overlay text or submenu & overlay background values are provided, these should overwrite the values of navigation block. ( ✅ )

three

if only submenu's color and background value are passed, then it shouldn't override the value of the submenu container. Container's values must be either inherited from navigation or from the dedicated submenu control options on navigation. ( ✅ )

four

Closes: #68350

@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review January 1, 2025 08:12
Copy link

github-actions bot commented Jan 1, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: Mayank-Tripathi32 <[email protected]>
Co-authored-by: carolinan <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@Mayank-Tripathi32 Mayank-Tripathi32 left a comment

Choose a reason for hiding this comment

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

@yogeshbhutkar Background color for sub menu block is not getting applied I think, Can you recheck?
https://github.com/user-attachments/assets/c561b8c3-cf34-41db-9fa9-effd6511aaa2

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

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.

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

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Block] Submenu Affects the Submenu Block - for submenus in navigation labels Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Submenu Affects the Submenu Block - for submenus in navigation [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Sub Menu: Bugs in color handling and block context
3 participants