Color Block Support: Switch to ToolsPanel for displaying UI #34027
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
chrisvanpatten and
ellatrix
as code owners
|
Size Change: +1.15 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
aaronrobertshaw
added
[Feature] Block API
|
Thanks for all the tools menu efforts 👌 This is what I see:
So, ultimately I do think we need to make the color panel into a ToolsPanel, notably when we get a slew of color values such as link, link hover, link focus, and others which are important for global styles but should very rarely be customized on, for example, a single Paragraph block. However specifically for the Paragraph block (and every other block with a color panel I can think of, including Button and Group), I would expect at least text and background color options to be shown by default. The problem with color swatches being duplicated and taking up a lot of space still needs a solution such as this one from #27331:
— that, but updated to the design in #27473 (comment). It seems okay in the mean time to convert to the ToolsPanel, but still showing at least text and background by default. Happy to note that all supported colors are already shown in Global Styles, as they should be 👌 Question: do you have a list of blocks that are affected by this PR? And can we customize the defaults per-block type yet? |
|
We are holding off on this conversion until we have better color controls in place as talked about in #27331
|
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
f354fef
to
3b1875d
Compare
|
This PR has been rebased and updated with respect to the latest changes to the Hopefully, when the new color controls are ready there is minimal extra work required to finish this switch off. |
aaronrobertshaw
changed the base branch from
trunk
to
add/virtual-bubbling-for-block-support-slots
aaronrobertshaw
force-pushed
the
add/virtual-bubbling-for-block-support-slots
branch
2 times, most recently
from
7bc3639
to
84ba3b8
Compare
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
3b1875d
to
23b288b
Compare
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
23b288b
to
34f273f
Compare
There was a problem hiding this comment.
Hey @aaronrobertshaw , is this PR in need of a review? I can give it a deeper look after the required rebase, in case you need !
|
After discussions on the PR adding first and last visible item classes within a ToolsPanel, it was raised that with the current method of adding color controls to the grouped inspector controls panel, we could have non- The simplest option was to remove the use of If it is still desired that all color options appear within an |
Yep, this is the direction to follow (at least for now). We may just want to have @jasmussen to check the border styles (e.g. border color, radius..) to make sure it fits our design guidelines.
In case that was related to using |
aaronrobertshaw
force-pushed
the
add/tools-panel-items-first-and-last-classes
branch
from
966c29b
to
5fe3da5
Compare
Yes, I meant this possible course of action as an option only if we decided to enforce use of I'll aim to merge the PR adding the necessary first/last visible item classes to the |
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
0c2ca1d
to
7dfe709
Compare
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
7dfe709
to
30108e0
Compare
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
92f53cd
to
e5da492
Compare
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
e5da492
to
dae9fab
Compare
|
This is testing well for me ✅ In the inspector controls sidebar you should see the colors block support panel using the ToolsPanel. It will have a + icon for the "more" menu I did notice that, on the frontend, a container's link color overrides the link color of its inner blocks, but it's happening on trunk as well. Just noting here. I'll create an issue later if none exists.
|
Nice catch. I agree we should tackle that one separately as I don't believe it is directly related to this PR as you noted. |
|
Great work! 🦸 |
packages/block-editor/src/components/colors-gradients/tools-panel-color-dropdown.js
Show resolved
Hide resolved
| > | ||
| <Dropdown | ||
| className="block-editor-tools-panel-color-dropdown" | ||
| contentClassName="block-editor-panel-color-gradient-settings__dropdown-content" |
There was a problem hiding this comment.
This is reusing a classname from another component potentially creating style conflicts.
There was a problem hiding this comment.
Agreed. To maintain the visual consistency, are you okay with us duplicating all these styles?
| onClick={ onToggle } | ||
| aria-expanded={ isOpen } | ||
| className={ classnames( | ||
| 'block-editor-panel-color-gradient-settings__dropdown', |
There was a problem hiding this comment.
This is reusing a classname from another component potentially creating style conflicts.
| <InspectorControls.Slot | ||
| __experimentalGroup="color" | ||
| label={ __( 'Color' ) } | ||
| className="color-block-support-panel__inner-wrapper" |
There was a problem hiding this comment.
This class doesn't seem to follow our naming guidelines. We should start with the package name followed by the component name and a modified, so something like block-editor-block-inspector__color-slot
There was a problem hiding this comment.
I can create a PR to tidy this up.
| * must at least render a placeholder which would otherwise interfere | ||
| * with the `:last-child` styles. | ||
| */ | ||
| .block-editor-tools-panel-color-gradient-settings__item { |
There was a problem hiding this comment.
Should we move these styles to the component style itself (style.scss of the same folder of the component). Or are these styles specific to this particular usage of this component. If I'm not wrong this component is specific to hooks anyway.
There was a problem hiding this comment.
(Similar question for the remaining styles here)
There was a problem hiding this comment.
Currently, the component is only used in the color block supports hook but could be used to add a color control to any ToolsPanel. I can move these styles to live in the color-gradients/style.scss if that is better.
| @@ -0,0 +1,85 @@ | |||
| .color-block-support-panel { | |||
There was a problem hiding this comment.
Where does this style come from, it doesn't seem to follow our style guidelines.
There was a problem hiding this comment.
The classname comes from the BlockSupportsToolsPanel which wraps the grouped inspector controls slots for block supports in a ToolsPanel.
I can update the format of this class and the corresponding styles for the different block supports dimensions, border, typography etc.
|
Thanks for raising these issues and concerns @youknowriad. Are you ok with I'll also create another PR updating the class name for the block support panels, the corresponding styles, and e2e tests. Is there anything else you would like to see fixed up in this area? |
Depends on:
Related:
Description
This updates the UI for controlling colors provided by block supports to use the
ToolsPanelcomponent.ToolsPanelfor other block supports, this aims to improve consistency between the block support panels.ToolsPanelColorDropdowncomponent.ItemGroup.ItemGroup.ItemGroupstyling.ItemGroupwe could end up with non-color controls as children of theItemGroupbreaking its semantics (e.g. Contrast Checkers, help text, or anything else a 3rd party dev wishes to render into the slot)ItemGroupand leverages CSS ordering to force contrast checkers to the bottom of the panel.Note: Which color controls display by default (even without any value) for any given block will be assessed and updated via a separate PR. This only focuses on switching the panel component the controls are within.
How has this been tested?
Manually.
Test Instructions
ToolsPanel. It will have a+icon for the "more" menuScreenshots
Screen.Recording.2021-12-21.at.6.44.31.pm.mp4
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).