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 Block: Add separate overlay color option #29963

Closed
jasmussen opened this issue Mar 18, 2021 · 14 comments · Fixed by #31149
Closed

Navigation Block: Add separate overlay color option #29963

jasmussen opened this issue Mar 18, 2021 · 14 comments · Fixed by #31149
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Feature] Inspector Controls The interface showing block settings and the controls available for each block [Status] In Progress Tracking issues with work in progress

Comments

@jasmussen
Copy link
Contributor

jasmussen commented Mar 18, 2021

What problem does this address?

The Navigation block, by default, is transparent, and submenus are white with black text:

Screenshot 2021-03-18 at 10 03 46

When you define colors on the navigation block, submenus are able to inherit those, and look the same:

Screenshot 2021-03-18 at 10 03 34

What I'm unable to do, is colorize just the submenu, and have the main menu stay transparent, like this:

cant-do

What is your proposed solution?

Allow setting the colors of main and submenus individually:

Frame 482

Note: Fixing this ticket also fixes #30165.

@jasmussen jasmussen added [Feature] Inspector Controls The interface showing block settings and the controls available for each block [Block] Navigation Affects the Navigation Block labels Mar 18, 2021
@carolinan
Copy link
Contributor

Maybe this has been discussed but what if each menu item had its own colors?

@jasmussen
Copy link
Contributor Author

Because submenus are children of the menu item, and because they inherit colors, if we were to allow text and background color to be chosen on a per-menu item basis (which could be interesting), submenus would inherit the colors of their parent menu item.

That wouldn't preclude us from still creating an "Overlay background color" that would override it, such as in the first of these two mockups.

@jasmussen jasmussen changed the title Navigation Block: Allow colorizing only submenus Navigation Block: Add separate overlay color option Mar 29, 2021
@jasmussen
Copy link
Contributor Author

I updated this ticket to clarify the best path forward.

@georgeh
Copy link
Contributor

georgeh commented Apr 7, 2021

Looking into this

@georgeh
Copy link
Contributor

georgeh commented Apr 15, 2021

I've been struggling to find a good way to add this to the existing color menu, and come up with 3 approaches:

  1. Stop using supports: color and manually manage the colors in this block. Basically fork all the color UI for this block.
  2. Add a second Inspector panel for "Submenu Color." I don't like this because it clutters everything up.
  3. Add a filter to the color hook so that we can append the submenu color controls from the block.

I like number 3 the best of those. I'll put up separate PRs, one for the filter in the color hook and a second one that implements the controls in this block

@jasmussen
Copy link
Contributor Author

Thanks for surfacing those problems! The design showing multiple ways to add color controls beyond just background and text is based on designs in #27473 (comment), so this is not going to be a one-off "new color support" case and we might want to find which path is the path proposed in global styles and go that route.

For that reason, @nosolosw and perhaps @gziolo, I know you're both always so busy, but I'd really appreciate some insights into how a new "overlay color" panel could best be added for the navigation block to follow best practices.

@gziolo
Copy link
Member

gziolo commented Apr 16, 2021

I need to think about it, but UI wise it could benefit from using a subgroup:

  • Color
    • Background
    • Text
    • Overlay
      • Background
      • Text

Now, with that in mind, I'm trying to better understand what we get as a final result. In practice, the overlay is rendered as part of the Navigation Link block (it's a bit confusing when you find out). As a first thought, I would expect to have the color support to be applied to the Navigation Link block, but it only makes sense when it acts as a container. In addition to that, you want to have the same color applied to all overlays/link containers, so it would be great to bubble it up to the Navigation block. I must admit it's very complex and it feels like with the current architecture of navigation blocks a robust supports: color for the overlay is close to impossible.

Add a filter to the color hook so that we can append the submenu color controls from the block.

Can you expand on this idea? How this filter would work?

@jasmussen
Copy link
Contributor Author

UI-wise, I don't think a sub-group is necessary. I'm thinking a list of this visual treatment might work well:

So just a list of color properties.

Now, with that in mind, I'm trying to better understand what we get as a final result. In practice, the overlay is rendered as part of the Navigation Link block (it's a bit confusing when you find out). As a first thought, I would expect to have the color support to be applied to the Navigation Link block, but it only makes sense when it acts as a container. In addition to that, you want to have the same color applied to all overlays/link containers, so it would be great to bubble it up to the Navigation block. I must admit it's very complex and it feels like with the current architecture of navigation blocks a robust supports: color for the overlay is close to impossible.

I'm missing bits and pieces here, but for me it doesn't seem very complex at all.

There's a navigation block, and I can set a background color on it, a foreground color on it, and I can set an overlay background and foreground color. I would expect both of the overlay properties to be attached to the Navigation Block itself, and I don't think there's huge value in allowing these overlay colors to be customized on a per-menu-item basis.

@jasmussen
Copy link
Contributor Author

Perhaps a better way to think of it is as the background color for the burger menu from #30047. So just one color for the background, one for the foreground, and it's a property of the block itself, just like the burger menu is a toggle on the block itself.

@gziolo
Copy link
Member

gziolo commented Apr 16, 2021

It makes me wonder if we should introduce an official API that registers commonly used inspector/toolbar controls so we could cover some custom cases for colors that you can see in the Navigation, Buttons, or the Social Links blocks. I'm sure there is more, but the idea would be that a plugin can register a custom color for overlay, then a block enables it with some supports flag.

Well, it isn’t that you could create any control for start, but maybe we can explore having a variations for existing controls like:

wp.blockEditor.registerBlockControl( {
    name: 'navigation-overlay',
    type: 'background-color',
    label: __( 'Overlay' ),
} );

The important part would be to have some utils that handle all style and className props for the React element that renders the element that needs those colors.

I guess supports should be very minimalistic, but it would be great to have a way to build a higher-level API that allows you to customize those controls for blocks. To get there, we probably need to formalize those controls in the first place

@oandregal
Copy link
Member

oandregal commented Apr 16, 2021

Braindump about the structure of color properties:

  • background, color, gradient, or link props map directly to a specific CSS property: background-color, color, background, and --wp--style--link-color, specifically.
  • We also have the upcoming duotone block attribute, which is not serialized to the block's HTML markup but instead we attach some styles in the server.

If we were to add two new overlay properties for color (background and foreground) I'd follow what we did links: they'll be serialized to custom properties so can be used by inner elements of the block (--wp--style--overlay-background, --wp--style--overlay-foreground).


Braindump about "lifting up" properties from children to parent:

It seems this issue falls into the category of how to "lift up" some child block properties to the parent. We could solve this use case by having background / foreground colors in the navigation-link block, but it's not a great user experience.

One approach we're using to "lift up" some properties is by using the "skip serialization" mechanism. This works fine when the property that's lifted up has the same semantics than the parent's. Example: social links. How to set up background color for all children at once from the parent. Use "skip serialization" and use the parent background color attribute to set children's. It works fine. We're not lifting up anything, though, but solved that problem by converting it into a different design space wich cover more use cases (it also serves complex blocks that don't necessarily are built up from children blocks or want to use the background color in a specific way, etc).

Coming back to lifting up things. This use case is different than the above. Here, the parent still wants to retain its own "background" and "foreground" properties, it just needs more "background & foreground colors" for its inner children. One way to fix this would be adding the ability for parents to absorb the UI controls of the children or register more things. They'll use then the context to pass down the block attributes and the children will serialize them. It's not automatic, but it can work fine.


Another thought is how this fits with the elements API. Overlays share some characteristics with links: both are inner components of the block + add extra characteristics that can't be absorbed by existing properties + we may want to set overlays across blocks in the global styles sidebar.


Actionable paths forward:

In addition to the paths forward George shared, I'll add two more:

  1. Add overlay background and overlay foreground as two new color properties in supports serialized as CSS vars, as with links.
  2. Explore an API to cover the use case of having "more of the same properties".

It's Friday, so I'm going to let my brain sim all this wall of text until Monday 😅 but would welcome thoughts from @youknowriad and @matias as well

@youknowriad
Copy link
Contributor

youknowriad commented Apr 16, 2021

I'm still wrapping my head around this but I think I feel like we shouldn't model reusable APIs around the use case of the Navigation block which is very advanced and very adhoc. So for me, the solution might be to not use the support flag in this block (doesn't mean it case use global styles settings and styles though)

(Sorry clicked the wrong button)

@georgeh
Copy link
Contributor

georgeh commented Apr 16, 2021

@gziolo

Add a filter to the color hook so that we can append the submenu color controls from the block.

Can you expand on this idea? How this filter would work?

My thought was to wrap <ColorPanel> in the colors hook with a filter. Then in the Navigation block append the controls for submenus in the element array and they all get rendered together. I like this approach because it makes the UI of the hook extensible for other blocks.

An official API like the wp.blockEditor.registerBlockControl() idea mentioned would be better for my use case, I'd prefer something like that for the core panels.

I'll note that I've just been looking at extending the UI at this point and haven't come up with a strategy for implementing the colors yet. I'm going to spend some time manually implementing the colors without supports: color to get something working but I appreciate the feedback and ideas around how best to implement this.

@gziolo
Copy link
Member

gziolo commented Apr 20, 2021

I'm still wrapping my head around this but I think I feel like we shouldn't model reusable APIs around the use case of the Navigation block which is very advanced and very adhoc. So for me, the solution might be to not use the support flag in this block (doesn't mean it case use global styles settings and styles though)

I agree that the Navigation block is one of the most advanced blocks so it obviously has some special cases. As the first step, it makes sense to start with something simpler so the plan that @georgeh outlined stays in line with what you said:

I'm going to spend some time manually implementing the colors without supports: color to get something working but I appreciate the feedback and ideas around how best to implement this.

I still think that we should observe all the use cases we have with other blocks and try to identify common limitations so we could offer a more flexible API that satisfies some of the recurring patterns like colors applied to HTML elements that aren't block wrappers.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Apr 23, 2021
georgeh pushed a commit that referenced this issue Apr 27, 2021
jasmussen pushed a commit that referenced this issue May 7, 2021
georgeh pushed a commit that referenced this issue May 14, 2021
georgeh pushed a commit that referenced this issue May 17, 2021
georgeh pushed a commit that referenced this issue May 20, 2021
georgeh pushed a commit that referenced this issue May 25, 2021
georgeh pushed a commit that referenced this issue May 26, 2021
georgeh pushed a commit that referenced this issue May 27, 2021
georgeh pushed a commit that referenced this issue Jun 2, 2021
georgeh pushed a commit that referenced this issue Jun 9, 2021
georgeh pushed a commit that referenced this issue Jun 10, 2021
georgeh pushed a commit that referenced this issue Jun 14, 2021
georgeh pushed a commit that referenced this issue Jun 21, 2021
georgeh pushed a commit that referenced this issue Jun 22, 2021
georgeh pushed a commit that referenced this issue Jun 23, 2021
georgeh pushed a commit that referenced this issue Jun 29, 2021
georgeh pushed a commit that referenced this issue Jul 9, 2021
georgeh pushed a commit that referenced this issue Jul 12, 2021
georgeh added a commit that referenced this issue Jul 12, 2021
Closes #29963

Co-authored-by: Marcus Kazmierczak <marcus@mkaz.com>
Co-authored-by: jasmussen <joen@automattic.com>
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] Inspector Controls The interface showing block settings and the controls available for each block [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants