Navigation Block: Fix link colors and remove unnecessary classes

[aaronrobertshaw]

Fixes #25889

Description

This improves the display of color selections for the navigation block between the editor and frontend. It also removes unneeded css classes and styles from the inner links allowing them to inherit the colors from the main navigation block itself.

Changes Included

• Removes unnecessary block context passing of colors
  • packages/block-library/src/navigation/block.json
  • packages/block-library/src/navigation-link/block.json
• CSS now allows colors to be inherited by navigation links from the main navigation block and fixes color consistency between editor and frontend for most themes.
  • packages/block-library/src/navigation/editor.scss
  • packages/block-library/src/navigation/style.scss
• Removes manual application of block support classes and styles for colors in navigation link within the editor.
  • packages/block-library/src/navigation-link/edit.js
• Removes manual generation and application of colors and font size classes and styles from server side rendering. These are block supports and provided automatically via get_block_wrapper_attributes()
  • packages/block-library/src/navigation/index.php
  • packages/block-library/src/navigation-link/index.php

How has this been tested?

Manually tested.

Tried multiple themes and multiple color combinations between named and custom colors. Used a mix of full featured and basic themes. In particular the themes tested with were:

• TwentyTwenty
• Maywood
• Varia

Testing Instructions

1. Before applying this PR branch
2. Create a post and add a navigation block to it
3. Change around color settings noticing odd behaviours
4. Save different color combinations and compare with frontend for unexpected results
5. Checkout this PR branch
6. Change around color settings and confirm it behaves visually as you would expect
7. Save different combinations and confirm these choices are reflected on the frontend

Screenshots

Before

TwentyTwenty	Maywood	Varia

After

TwentyTwenty	Maywood	Varia

Types of changes

Bug fix.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[apeatling]

Tested this and confirmed that it is now working reliably on the front end, whereas before custom background colors were not showing up for me. Nice work removing all that code!

[talldan]

@aaronrobertshaw Thanks for working on this. I wasn't aware there was a bug, but checking master text colors do seem to behave inconsistently (in TwentyTwenty some work and others don't).

Would be really helpful if you could share a bit more detail on what the changes are and why they were needed. The diff is fairly large and hard to follow without spending a lot of time on it.

An issue I noticed in testing is that submenu backgrounds are now transparent when no background is applied:

This is something that has regressed a few times previously, so would be great to get that fixed. 😄

[aaronrobertshaw]

@talldan Appreciate you taking the time to look at this one. Apologies for not providing enough info in the PR description.

Overview of changes

A lot of the diff is clean up of the unnecessary application of css classes and inline styles on the links when they can inherit them from the parent block. This allows a fair bit of code removal and avoids the need to pass those color attributes via the block context.

The inconsistency of colors is a little trickier for me to articulate, so please bear with me 🙂 .

When the color palettes and related named color css classes do not match between the editor and the frontend, what the user selects and sees in the editor turns out to be different than what is displayed on the frontend. I believe the extent of this issue is dependent on the theme and what it supports or provides. Given it was a problem across a range of themes, we should prevent a little frustration for our users and honour their selections where we can.

As the useColors hook already provided actual color values we could just leverage those to set the inline styles forcing consistency between the editor and the frontend.

Summary of changes per file

navigation-link/index.php

• Links don't need the css classes and inline styles. They can inherit from the main block
• Removed code applying color classes and styles

navigation-link/edit.js

• Links don't need the css classes and inline styles. They can inherit from the main block
• Removed code determining color values and injecting classes and styles

navigation-link/block.json

• Removed the color attributes from block context

navigation/block.json

• Removed the color attributes from block context

navigation/edit.js

• Added hook to get the useColor created attribute values, and assign them to the custom color attributes
• This is to force the consistency to frontend, not ideal but better than user's not getting what they asked for

navigation/editor.scss

• Add style to allow the navigation links to get the current color from main block

navigation/index.php

• Tweaked so that if we have a custom color set it is used
• This pairs with the change to navigation/edit.js

navigation/style.scss

• Tweaks the setting of default font and background colors when no selections made
• This is where the transparent background bug was caused ( fixed now though )

[talldan]

Thanks for providing details @aaronrobertshaw and fixing that issue, that really helps for review and also for anyone looking back at the history.

The main part I'm uneasy about at the moment is the React hook that always sets custom colors. I feel as much as possible the nav block should stay consistent with the color systems used by other blocks. At the moment this PR changes that by setting inline styles even for named colors. Is this mainly a specificity issue or something else?

[aaronrobertshaw]

@talldan I understand your unease on that hook always setting the custom colors. I feel a little the same way about it.

The issue isn't with specificity really but consistency and ensuring the user gets what they asked for. As it is now, across many themes, the editor gets one color value for a named color class e.g. has-primary-color and the frontend gets a different color value for that same named color class.

The poor user is left thinking they've selected one color and puzzled as to why a different color is shown when they view their site. As much as I don't like forcing inline styles and therefore those custom color attributes, I don't like making the excuse that it's the theme's fault when we could meet the user's expectations without too much trouble. The basis for this approach happened to be an earlier incarnation of the navigation link block where it was forcing inline styles in the editor.

This is a bit of a problem in general with the current color system. If we had a means to force the consistency in named color css classes between the editor and frontend, I think we could avoid this. I might be missing something so if you have any suggestions I'm happy to alter the approach.

[talldan]

@aaronrobertshaw I think rather than the nav block having ad-hoc code for handling colors (and more importantly markup in the frontend), consistency for blocks is best as a baseline.

Once there's consistency, it makes it easier to look into improving the color system across all blocks.

[aaronrobertshaw]

@talldan I’ve created an issue regarding achieving consistency of color selections between the editor and frontend #25889.

The commit (8b84c42ef3955624cc2efad65a78bfb2afe717ab) that added the hook to force the inline styles has been reverted as well. What is left allows the color styles to cascade from the nav block to its links and removes a lot of otherwise unnecessary code e.g. passing of colors via block context. It also allows the editor to display the selected text color properly.

[aaronrobertshaw]

I've rebased this and recent updates have solved some of the color consistency issues noted previously. Currently, it is just the navigation block suffering some inconsistency with link colors.

During the rebase I noticed that duplicate CSS classes and styles were introduced (7c2fb7c) when the block was updated to use get_block_wrapper_attributes(). This PR now cleans that up as well given it was already cleaning up application of classes on the navigation links.

The PR description has been updated to included a more detailed breakdown of the included changes.

Would you mind taking another look at this @talldan?

[aaronrobertshaw]

Closing this as the underlying issues have been resolved elsewhere.