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.

Sign up for GitHub

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

Use dropdown menus for nested color controls #37053

Closed
wants to merge 4 commits into from
+213 −77
Closed

Use dropdown menus for nested color controls #37053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecce34c
Refactor the colors UI in the inspector to use the navigator pattern
youknowriad Nov 29, 2021
57de133
Try adapting the padding to work in the classic inspector.
jasmussen Dec 1, 2021
8a0b5e2
Use dropdown menus for nested color controls
youknowriad Dec 2, 2021
79d75e0
Tweak the popover position
youknowriad Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 20 additions & 22 deletions packages/block-editor/src/hooks/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,31 @@
* WordPress dependencies
*/
import { useState, useEffect } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import PanelColorGradientSettings from '../components/colors-gradients/panel-color-gradient-settings';
import ContrastChecker from '../components/contrast-checker';
import InspectorControls from '../components/inspector-controls';
import { __unstableUseBlockRef as useBlockRef } from '../components/block-list/use-block-props/use-block-refs';
import ColorGradientControl from '../components/colors-gradients/control';
import useCommonSingleMultipleSelects from '../components/colors-gradients/use-common-single-multiple-selects';
import useSetting from '../components/use-setting';

function getComputedStyle( node ) {
return node.ownerDocument.defaultView.getComputedStyle( node );
}

export default function ColorPanel( {
settings,
setting,
clientId,
enableContrastChecking = true,
showTitle = true,
} ) {
const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState();
const [ detectedColor, setDetectedColor ] = useState();
const ref = useBlockRef( clientId );
const colorGradientSettings = useCommonSingleMultipleSelects();
colorGradientSettings.colors = useSetting( 'color.palette' );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

colorGradientSettings.gradients = useSetting( 'color.gradients' );

useEffect( () => {
if ( ! enableContrastChecking ) {
Expand Down Expand Up @@ -54,22 +56,18 @@ export default function ColorPanel( {
} );

return (
<InspectorControls>
<PanelColorGradientSettings
title={ __( 'Color' ) }
initialOpen={ false }
settings={ settings }
showTitle={ showTitle }
__experimentalHasMultipleOrigins
__experimentalIsRenderedInSidebar
>
{ enableContrastChecking && (
<ContrastChecker
backgroundColor={ detectedBackgroundColor }
textColor={ detectedColor }
/>
) }
</PanelColorGradientSettings>
</InspectorControls>
<div className="block-editor-panel-color-gradient-settings">
<ColorGradientControl
showTitle={ false }
{ ...colorGradientSettings }
{ ...setting }
/>
{ enableContrastChecking && (
<ContrastChecker
backgroundColor={ detectedBackgroundColor }
textColor={ detectedColor }
/>
) }
</div>
);
}
209 changes: 154 additions & 55 deletions packages/block-editor/src/hooks/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@ import { getBlockSupport } from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';
import { useRef, useEffect, useMemo, Platform } from '@wordpress/element';
import { createHigherOrderComponent } from '@wordpress/compose';
import {
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
__experimentalHStack as HStack,
FlexItem,
ColorIndicator,
PanelBody,
Dropdown,
} from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -28,6 +37,7 @@ import {
} from '../components/gradients';
import { cleanEmptyObject } from './utils';
import ColorPanel from './color-panel';
import InspectorControls from '../components/inspector-controls';
import useSetting from '../components/use-setting';

export const COLOR_SUPPORT_KEY = 'color';
Expand Down Expand Up @@ -294,6 +304,24 @@ export function ColorEdit( props ) {
} else if ( hasGradientColor ) {
gradientValue = style?.color?.gradient;
}
const backgroundValue = hasBackgroundColor
? getColorObjectByAttributeValues(
allSolids,
backgroundColor,
style?.color?.background
).color
: undefined;
const textColorValue = getColorObjectByAttributeValues(
allSolids,
textColor,
style?.color?.text
).color;
const linkColorValue = hasLinkColor
? getLinkColorFromAttributeValue(
allSolids,
style?.elements?.link?.color?.text
)
: undefined;

const onChangeColor = ( name ) => ( value ) => {
const colorObject = getColorObjectByColorValue( allSolids, value );
Expand Down Expand Up @@ -371,61 +399,132 @@ export function ColorEdit( props ) {
};

return (
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' && ! gradient && ! style?.color?.gradient
}
clientId={ props.clientId }
settings={ [
...( hasTextColor
? [
{
label: __( 'Text color' ),
onColorChange: onChangeColor( 'text' ),
colorValue: getColorObjectByAttributeValues(
allSolids,
textColor,
style?.color?.text
).color,
},
]
: [] ),
...( hasBackgroundColor || hasGradientColor
? [
{
label: __( 'Background color' ),
onColorChange: hasBackgroundColor
? onChangeColor( 'background' )
: undefined,
colorValue: getColorObjectByAttributeValues(
allSolids,
backgroundColor,
style?.color?.background
).color,
gradientValue,
onGradientChange: hasGradientColor
? onChangeGradient
: undefined,
},
]
: [] ),
...( hasLinkColor
? [
{
label: __( 'Link Color' ),
onColorChange: onChangeLinkColor,
colorValue: getLinkColorFromAttributeValue(
allSolids,
style?.elements?.link?.color?.text
),
clearable: !! style?.elements?.link?.color
?.text,
},
]
: [] ),
] }
/>
<InspectorControls>
<PanelBody
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we had a color indication when the panel was collapsed:
image

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're making them circular in #37028

Copy link
Contributor

@ciampo ciampo Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI #37028 just got merged

title={ __( 'Colors' ) }
className="block-editor__hooks-colors-panel"
>
<ItemGroup isBordered isSeparated>
{ ( hasBackgroundColor || hasGradientColor ) && (
<Dropdown
contentClassName="block-editor__hooks-colors-panel__dropdown"
renderToggle={ ( { onToggle } ) => {
return (
<Item onClick={ onToggle }>
<HStack justify="flex-start">
<FlexItem>
<ColorIndicator
colorValue={
gradientValue ??
backgroundValue
}
/>
</FlexItem>
<FlexItem>
{ __( 'Background' ) }
</FlexItem>
</HStack>
</Item>
);
} }
renderContent={ () => (
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' &&
! gradient &&
! style?.color?.gradient
}
clientId={ props.clientId }
setting={ {
label: __( 'Background color' ),
onColorChange: hasBackgroundColor
? onChangeColor( 'background' )
: undefined,
colorValue: backgroundValue,
gradientValue,
onGradientChange: hasGradientColor
? onChangeGradient
: undefined,
} }
/>
) }
/>
) }
{ hasTextColor && (
<Dropdown
contentClassName="block-editor__hooks-colors-panel__dropdown"
renderToggle={ ( { onToggle } ) => (
<Item onClick={ onToggle }>
<HStack justify="flex-start">
<FlexItem>
<ColorIndicator
colorValue={ textColorValue }
/>
</FlexItem>
<FlexItem>{ __( 'Text' ) }</FlexItem>
</HStack>
</Item>
) }
renderContent={ () => (
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' &&
! gradient &&
! style?.color?.gradient
}
clientId={ props.clientId }
setting={ {
label: __( 'Text color' ),
onColorChange: onChangeColor( 'text' ),
colorValue: textColorValue,
} }
/>
) }
/>
) }
{ hasLinkColor && (
<Dropdown
contentClassName="block-editor__hooks-colors-panel__dropdown"
renderToggle={ ( { onToggle } ) => (
<Item onClick={ onToggle }>
<HStack justify="flex-start">
<FlexItem>
<ColorIndicator
colorValue={ linkColorValue }
/>
</FlexItem>
<FlexItem>{ __( 'Links' ) }</FlexItem>
</HStack>
</Item>
) }
renderContent={ () => (
<ColorPanel
enableContrastChecking={
// Turn on contrast checker for web only since it's not supported on mobile yet.
Platform.OS === 'web' &&
! gradient &&
! style?.color?.gradient
}
clientId={ props.clientId }
setting={ {
label: __( 'Link Color' ),
onColorChange: onChangeLinkColor,
colorValue: getLinkColorFromAttributeValue(
allSolids,
style?.elements?.link?.color?.text
),
clearable: !! style?.elements?.link
?.color?.text,
} }
/>
) }
/>
) }
</ItemGroup>
</PanelBody>
</InspectorControls>
);
}

Expand Down
33 changes: 33 additions & 0 deletions packages/block-editor/src/hooks/color.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
.block-editor__hooks-colors-panel {
.components-dropdown {
display: block;
}

// Allow horizontal overflow so the size-increasing color indicators don't cause a scrollbar.
.components-navigator-screen {
overflow-x: visible;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NavigatorScreen already has a overflow-x: 'auto', why do we need to force it to visible ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Comment on lines +2 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@jasmussen jasmussen Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Posted in context of the wrong comment.


// @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);
}
Comment on lines +11 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With #37028 merged, I believe these styles can be removed

}

@include break-medium() {
.block-editor__hooks-colors-panel__dropdown .components-popover__content {
margin-right: #{ math.div($sidebar-width, 2) + $grid-unit-20 } !important;
margin-top: #{ -($grid-unit-60 + $grid-unit-15) } !important;
}
}
1 change: 1 addition & 0 deletions packages/block-editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
@import "./hooks/border.scss";
@import "./hooks/dimensions.scss";
@import "./hooks/typography.scss";
@import "./hooks/color.scss";

@import "./components/block-toolbar/style.scss";
@import "./components/inserter/style.scss";
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/panel/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
&.is-opened {
padding: $grid-unit-20;
}

// Don't cascade the padding into nested panels.
.components-panel__body {
padding: 0;
}
Comment on lines +33 to +35
Copy link
Contributor

@ciampo ciampo Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a CHANGELOG entry for this change to the Panel component?

}

.components-panel__header {
Expand Down