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

Improve Navigation block color controls #44712

Open
getdave opened this issue Oct 5, 2022 · 5 comments
Open

Improve Navigation block color controls #44712

getdave opened this issue Oct 5, 2022 · 5 comments
Labels
[Block] Navigation Affects the Navigation Block [Feature] Navigation Menus Any issue relating to Navigation Menus [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@getdave
Copy link
Contributor

getdave commented Oct 5, 2022

What problem does this address?

Currently the Navigation block implements custom controls for all aspects of color including:

  • Text
  • Background
  • Submenu and overlay text
  • Submenu and overlay background

However these are used in a non-standard way which causes confusion and holds the potential for bugs.

The key problems are:

  • The controls are custom to the block and non-standard. Therefore the block will not inherit any future enhancements applied to the standardised color controls and the styling mechanics are likely to be inconsistent with other blocks.
  • Most items in a Navigation are links but the control is designed for Text color. Currently the block uses the CSS cascade to implicitly set the link color on the item (via color: inherit). However this is vulnerable to being overriden by Link styles coming from ancestor blocks using global styles.
  • Submenu and overlay styles are conflated meaning it is impossible to style these two aspects separately.

What is your proposed solution?

As the block has been around for some time, these problems are not trivial to solve. Some suggestions include:

@getdave
Copy link
Contributor Author

getdave commented Oct 5, 2022

@aaronrobertshaw As you've been working on Block Supports a lot I wonder if you'd have any insight here particularly around whether we can pass color values set via block supports down to child blocks via block context?

You'll note that the Nav block currently passes the color values down to the various child blocks which allows them to use the colors set on the parent. Would you recommend continuing with this approach or instead should we rely on the CSS cascade to propagate the colors?

@aaronrobertshaw
Copy link
Contributor

Thanks for the ping @getdave 👍

whether we can pass color values set via block supports down to child blocks via block context?

If I understand the question correctly, we should be able to pass block support set colors down via block context.

The values set via the color block supports get saved into normal block attributes. So I'd imagine that it would be a matter of updating the block context configuration to match those and handle any block deprecation requirements.

Named colors are generally saved in top-level block attributes e.g. textColor, backgroundColor, gradient etc. You can find where these attributes are added via a filter in the color block support hook. Custom color values are saved within the block's style attribute e.g. style.color.text.

You'll note that the Nav block currently passes the color values down to the various child blocks which allows them to use the colors set on the parent.

The Social Links block and its inner Social Link children, use block context as well to allow uniform application of colors from the parent block.

Would you recommend continuing with this approach or instead should we rely on the CSS cascade to propagate the colors?

As you note, relying on the CSS cascade might get us into a little trouble. If we can avoid that, I would.

Going back to the Social Links block example, in an earlier iteration, it employed CSS custom properties to both avoid using block context and have more explicit control over the application of colors. This was changed to the current block context approach due to some concerns at the time around setting CSS vars within inline styles. From memory, there might have been some issues with KSES in certain environments (e.g. core) stripping those styles out. I believe that has changed since though. If you find it has, leveraging CSS vars here might be a nice clean option.

The controls are custom to the block and non-standard. Therefore the block will not inherit any future enhancements applied to the standardised color controls and the styling mechanics are likely to be inconsistent with other blocks.

The Navigation block uses the PanelColorSettings component which is pretty much a convenience wrapper around PanelColorGradientSettings. The latter uses ColorGradientSettingsDropdown to generate the color control. This is actually what the color block supports use. Also, PanelColorGradientSettings wraps its color controls within a ToolsPanel just like the color block supports.

In this regard, the main differences I see here in the Navigation block's controls are;

  • They render in their own ToolsPanel when the color controls could be rendered into the color block supports panel (more on this in a moment).
  • The color values are saved in bespoke block attributes.

My point here is I might be missing how these controls will miss any future enhancements.

#42092. This will involve separating the Submenu and Overlay menu color controls from the other controls.

I've commented on that draft PR as all these color-related controls can in fact be rendered within the same panel. The color block support controls are rendered via SlotFill into their "Color" panel. This can be achieved for the Nav block's controls by wrapping them in an <InspectorControls __experimentalGroup="color">.

🤞 That helps and I haven't gone off the rails too far in my reply.

Let me know if I can clarify or help out any further.

@carolinan
Copy link
Contributor

I'd like to try to work on this, but I am on vacation from november 28 til december 5 so if anyone is able to pick it up faster that would be even better.

@aaronrobertshaw
Copy link
Contributor

I'm actually currently working on moving the navigation block's color controls under the color block support panel. This will act as a small first step to addressing this issue.

It has been driven by the experiment introducing tabs to the block inspector sidebar. Without moving these controls they'll appear under the settings tab instead of the styles tab.

Once I have a draft PR up for this, I'll drop a link to it here.

@aaronrobertshaw
Copy link
Contributor

PR moving the color controls into the block support panel can be found here: #46049

@jordesign jordesign added [Type] Task Issues or PRs that have been broken down into an individual action to take [Feature] Navigation Menus Any issue relating to Navigation Menus labels Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Feature] Navigation Menus Any issue relating to Navigation Menus [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

4 participants