WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.57k stars 4.22k forks source link

Improve Navigation block color controls #44712

Open getdave opened 2 years ago

getdave commented 2 years ago

What problem does this address?

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

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

The key problems are:

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 commented 2 years ago

@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 commented 2 years ago

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;

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

https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

PR moving the color controls into the block support panel can be found here: https://github.com/WordPress/gutenberg/pull/46049