Use dropdown menus for nested color controls #37053
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
shaunandrews,
mtias,
ciampo,
jasmussen and
jorgefilipecosta
|
Size Change: +629 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
Just taking this for a quick spin, I see this: I like it. It does address the sliding-within-a-panel concern voiced on the other thread, and it absolutely compresses the interface which is a huge benefit over what's shipping. I wish we could position the flyout a bit better — open down and leftwards according to the top left corner of the item, but I suspect we're a bit limited by the popover component there, correct? Edit: At least open down and left would eliminate the jump from the contrast checker, right? |
|
@jasmussen Indeed we're limited by the popover component. What I did here is a CSS tweak to try to improve things (but I don't like it, ideally, we should have more options in the Popover component itself). Let me know how it feels now. |
|
I understand your reluctance, but I think I like it more than the previous version that had it jump when it changed size. We probably need to revisit the precise positioning in light of the presence of scrollbars: but it feels better than before. I tried restoring the |
| ] } | ||
| /> | ||
| <InspectorControls> | ||
| <PanelBody |
There was a problem hiding this comment.
I think the component PanelColorGradientSettings should be updated to be dropdown-based. The component API:
<PanelColorGradientSettings
title="Colors"
colors={ ... }
disableCustomColors={ true }
settings={ [
{
value: '#000',
onChange: noop,
label: 'Border color',
},
{
value: '#111',
onChange: noop,
label: 'Background color',
},
] }
/>
Can easily accommodate this UI where each color setting starts to be rendered on a new dropdown. As an advantage blocks like navigation and social links (and third-party blocks with custom color implementation) would automatically take advantage of this new UI.
As part of this component UI change, we would also need to remove the usage of the component from the Global Styles UI e.g: from packages/edit-site/src/components/global-styles/screen-background-color.js. We can use the ColorPalette Component directly the only thing we would lose is the tabbing between gradient and color but we can easily add that.
| ] } | ||
| /> | ||
| <InspectorControls> | ||
| <PanelBody |
There was a problem hiding this comment.
Previously we had a color indication when the panel was collapsed:
And now we are removing this indication. Should we keep it, or was the removal intentional, and the new design does not contain the color indication?
There was a problem hiding this comment.
We can keep them but they should be circular. Also keep in mind these panels should evolve to ToolsPanel and have resets, not be collapsible (yet), etc.
There was a problem hiding this comment.
We're making them circular in #37028
There was a problem hiding this comment.
FYI #37028 just got merged
| } ) { | ||
| const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState(); | ||
| const [ detectedColor, setDetectedColor ] = useState(); | ||
| const ref = useBlockRef( clientId ); | ||
| const colorGradientSettings = useCommonSingleMultipleSelects(); | ||
| colorGradientSettings.colors = useSetting( 'color.palette' ); |
There was a problem hiding this comment.
We were showing multiple color palettes default, theme, and user, and now we are just showing a single palette. I guess we can use the hook useMultipleOriginColorsAndGradients to get all the palettes.
Yes, the way I see it, this itemgroup-based system replaces the existing color system entirely. |
There was a problem hiding this comment.
the issue there is that the components for editing colors grew organically over time and are not in a very good state, there are special cases and components everywhere and I don't want to introduce another special case or another component there.
That is very true, and in fact there is a tracking issue that tries to summarise what would need to be done — so far, though, most of the tasks haven't been picked up yet (more details in this comment).
Indeed we're limited by the popover component. What I did here is a CSS tweak to try to improve things (but I don't like it, ideally, we should have more options in the Popover component itself)
It would be better if we could make improvements to the component itself, rather than introducing ad-hoc fixes which make it then even harder to make changes to the original component.
Also, is there a reason why we keep using the legacy Dropdown component instead of Flyout? We started a conversation in #33391 around its usage but then never followed up (cc @diegohaz )
| .components-dropdown { | ||
| display: block; | ||
| } | ||
|
|
||
| // Allow horizontal overflow so the size-increasing color indicators don't cause a scrollbar. | ||
| .components-navigator-screen { | ||
| overflow-x: visible; | ||
| } |
There was a problem hiding this comment.
We should not rely on internal components classnames, as they are not part of the public APIs and could change in the future.
A better approach would be to set a custom className in the specific context of the block editor's color panel, and then use that new custom classname to apply styles (both for dropdown and navigator screen)
There was a problem hiding this comment.
Edit: Posted in context of the wrong comment.
|
|
||
| // Allow horizontal overflow so the size-increasing color indicators don't cause a scrollbar. | ||
| .components-navigator-screen { | ||
| overflow-x: visible; |
There was a problem hiding this comment.
NavigatorScreen already has a overflow-x: 'auto', why do we need to force it to visible ?
There was a problem hiding this comment.
That rule can be removed. I suspect it's a remnant as this PR was forked from the one that included the navigator. I didn't feel very comfortable with targetting those classes either.
| .components-panel__body { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
Could you add a CHANGELOG entry for this change to the Panel component?
|
I'm not sure that the "transparent checkerboard" shown when a color is not set is very clear to the user:
I'm not sure if using a double popover can cause any accessibility issues (cc @diegohaz and @mirka) |
| // @todo: this can be removed when https://github.com/WordPress/gutenberg/pull/37028 lands. | ||
| .component-color-indicator { | ||
| margin-left: 0; | ||
| display: block; | ||
| border-radius: 50%; | ||
| border: 0; | ||
| height: 24px; | ||
| width: 24px; | ||
| padding: 0; | ||
| background-image: | ||
| repeating-linear-gradient(45deg, $gray-200 25%, transparent 25%, transparent 75%, $gray-200 75%, $gray-200), | ||
| repeating-linear-gradient(45deg, $gray-200 25%, transparent 25%, transparent 75%, $gray-200 75%, $gray-200); | ||
| background-position: 0 0, 25px 25px; | ||
| background-size: calc(2 * 5px) calc(2 * 5px); | ||
| } |
There was a problem hiding this comment.
With #37028 merged, I believe these styles can be removed
|
Thanks all for your feedback, @jorgefilipecosta opened a PR that takes this approach and apply it more consistently to all blocks using the ColorGradientsSettings component. #37067 I'm closing this one in favor of it. |
This is an alternative to #36952
The idea is to simplify/clarify the colors UI in block inspectors using the ItemGroup approach used in Global Styles. The current PR doesn't use navigation but uses Dropdown components instead.
It does surface the double popover issue when clicking the custom color selector but for some reason it doesn't bother me much.
We can definitely take this approach a bit further by introducing navigation inside the popover to go to the custom color selector, the issue there is that the components for editing colors grew organically over time and are not in a very good state, there are special cases and components everywhere and I don't want to introduce another special case or another component there.