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

Block Editor: Implement new colors hook. #16781

Merged
merged 15 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
4 changes: 4 additions & 0 deletions packages/block-editor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,10 @@ _Related_

- <https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/url-popover/README.md>

<a name="useBlockEditContext" href="#useBlockEditContext">#</a> **useBlockEditContext**

Undocumented declaration.

<a name="Warning" href="#Warning">#</a> **Warning**

Undocumented declaration.
Expand Down
8 changes: 6 additions & 2 deletions packages/block-editor/src/components/block-edit/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,22 @@ import { noop } from 'lodash';
/**
* WordPress dependencies
*/
import { createContext } from '@wordpress/element';
import { createContext, useContext } from '@wordpress/element';
import { createHigherOrderComponent } from '@wordpress/compose';

const { Consumer, Provider } = createContext( {
const Context = createContext( {
name: '',
isSelected: false,
focusedElement: null,
setFocusedElement: noop,
clientId: null,
} );
const { Provider, Consumer } = Context;

export { Provider as BlockEditContextProvider };
export function useBlockEditContext() {
return useContext( Context );
}

/**
* A Higher Order Component used to inject BlockEdit context to the
Expand Down
3 changes: 2 additions & 1 deletion packages/block-editor/src/components/block-edit/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Component } from '@wordpress/element';
* Internal dependencies
*/
import Edit from './edit';
import { BlockEditContextProvider } from './context';
import { BlockEditContextProvider, useBlockEditContext } from './context';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the changes in this file, since you can access clientId from the context, you can call updateBlockAttributes and getBlockAttributes without extra context values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't subscribe the consumer to changes though, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

useSelect would

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. I was thinking of the Block API functions. I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just feels a lot clunkier to have to use the context to get the id and then pass it to useSelect.

Don't you think a useBlockEditContext with attributes and the setter will be very useful for people building custom hooks that need access to attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so personally, I feel like the context should contain the minimum possible things because

  • these are new APIs
  • semantically speaking, the context says, we're editing that block (id) and it should be enough for the consumer.

if we add these, the question becomes: Will we add a new value there each time we need another selector/action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will we add a new value there each time we need another selector/action?

I don't think we could, because it would be limited to the props that BlockEdit receives. I just tend to favor context over selector/subscription boilerplate, but I can see why this aligns more with the rest of the codebase and see the value in having a single way of doing things:

69865dd


class BlockEdit extends Component {
constructor() {
Expand Down Expand Up @@ -44,3 +44,4 @@ class BlockEdit extends Component {
}

export default BlockEdit;
export { useBlockEditContext };
1 change: 1 addition & 0 deletions packages/block-editor/src/components/colors/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ export {
createCustomColorsHOC,
default as withColors,
} from './with-colors';
export { default as __experimentalUseColors } from './use-colors';
205 changes: 205 additions & 0 deletions packages/block-editor/src/components/colors/use-colors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
/**
* External dependencies
*/
import memoize from 'memize';
import { kebabCase, camelCase, startCase } from 'lodash';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useSelect, useDispatch } from '@wordpress/data';
import {
useCallback,
useMemo,
Children,
cloneElement,
} from '@wordpress/element';

/**
* Internal dependencies
*/
import PanelColorSettings from '../panel-color-settings';
import ContrastChecker from '../contrast-checker';
import InspectorControls from '../inspector-controls';
import { useBlockEditContext } from '../block-edit';

const ColorPanel = ( {
title,
colorSettings,
colorPanelProps,
contrastCheckerProps,
components,
panelChildren,
} ) => (
<PanelColorSettings
title={ title }
initialOpen={ false }
colorSettings={ colorSettings }
{ ...colorPanelProps }
>
{ contrastCheckerProps &&
components.map( ( Component ) => (
<ContrastChecker
key={ Component.displayName }
textColor={ Component.color }
{ ...contrastCheckerProps }
/>
) ) }
{ typeof panelChildren === 'function' ?
panelChildren( components ) :
panelChildren }
</PanelColorSettings>
);
const InspectorControlsColorPanel = ( props ) => (
<InspectorControls>
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need to assume the panel is nested inside InspectorControls and we can left the choice for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where else would you render it?

Copy link
Member

Choose a reason for hiding this comment

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

In a placeholder for example as part of a flow to initialize a block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather expose that under InspectorControlsColorPanel.ColorPanel. I think the inspector usage is the most common.

<ColorPanel { ...props } />
</InspectorControls>
);

export default function __experimentalUseColors(
colorConfigs,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have a useColors or a useColor hook used multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plural, because we want one panel inspector controls per list of colors, not per color.

{
panelTitle = __( 'Color Settings' ),
colorPanelProps,
contrastCheckerProps,
panelChildren,
} = {
panelTitle: __( 'Color Settings' ),
},
deps = []
) {
const { clientId } = useBlockEditContext();
const { attributes, settingsColors } = useSelect(
( select ) => {
const { getBlockAttributes, getSettings } = select( 'core/block-editor' );
return {
attributes: getBlockAttributes( clientId ),
settingsColors: getSettings().colors,
};
},
[ clientId ]
);
const { updateBlockAttributes } = useDispatch( 'core/block-editor' );
const setAttributes = useCallback(
( newAttributes ) => updateBlockAttributes( clientId, newAttributes ),
[ updateBlockAttributes, clientId ]
);

const createComponent = useMemo(
() =>
memoize(
( attribute, color, colorValue, customColor ) => ( { children } ) =>
// Clone children, setting the style attribute from the color configuration,
// if not already set explicitly through props.
Children.map( children, ( child ) => {
let className = child.props.className;
let style = child.props.style;
if ( color ) {
className = `${ child.props.className } has-${ kebabCase(
Copy link
Member

Choose a reason for hiding this comment

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

We have a small function that computes the class name from the color slug getColorClassName could we use it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one works slightly differently. It takes a kebab cased context name and the slug. Here we have both the color and attribute name in camel case and need to convert one or both depending on whether it's a custom color or not.

color
) }-${ kebabCase( attribute ) }`;
style = { [ attribute ]: colorValue, ...child.props.style };
} else if ( customColor ) {
className = `${ child.props.className } has-${ kebabCase( attribute ) }`;
style = { [ attribute ]: customColor, ...child.props.style };
}
return cloneElement( child, {
className,
style,
} );
} ),
{ maxSize: colorConfigs.length }
),
[ colorConfigs.length ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it that relying on colorConfigs.length was a trade-off between the convenience for the hook consumer of not having to cache colorConfigs, while retaining some sort of relationship between the input and the memo?

I can see why it works, but it's strictly speaking not correct. Just noting it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For each use of the hook we need a memoized function with a cache space for each item in colorConfigs. That's the usage of the length here. It's the only dependency, the array itself is not a dependency.

);
const createSetColor = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

useCallback is likely more adequate here and in similar places. Why useMemo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid calling memoize unnecessarily.

() =>
memoize(
( name, colors ) => ( newColor ) => {
const color = colors.find( ( _color ) => _color.color === newColor );
setAttributes( {
[ color ? camelCase( `custom ${ name }` ) : name ]: undefined,
} );
setAttributes( {
[ color ? name : camelCase( `custom ${ name }` ) ]: color ?
color.slug :
newColor,
} );
},
{
maxSize: colorConfigs.length,
}
),
[ setAttributes, colorConfigs.length ]
);

return useMemo( () => {
const colorSettings = [];

const components = colorConfigs.reduce( ( acc, colorConfig ) => {
if ( typeof colorConfig === 'string' ) {
colorConfig = { name: colorConfig };
}
const {
name, // E.g. 'backgroundColor'.
attribute = name, // E.g. 'backgroundColor'.

panelLabel = startCase( name ), // E.g. 'Background Color'.
componentName = panelLabel.replace( /\s/g, '' ), // E.g. 'BackgroundColor'.

color = colorConfig.color,
colors = settingsColors,
} = {
...colorConfig,
color: attributes[ colorConfig.name ],
};

// We memoize the non-primitives to avoid unnecessary updates
// when they are used as props for other components.
const _color = colors.find( ( __color ) => __color.slug === color );
acc[ componentName ] = createComponent(
attribute,
color,
_color && _color.color,
attributes[ camelCase( `custom ${ name }` ) ]
);
acc[ componentName ].displayName = componentName;
acc[ componentName ].color = color;
acc[ componentName ].setColor = createSetColor( name, colors );

const newSettingIndex =
colorSettings.push( {
value: color,
onChange: acc[ componentName ].setColor,
label: panelLabel,
colors,
} ) - 1;
// These settings will be spread over the `colors` in
// `colorPanelProps`, so we need to unset the key here,
// if not set to an actual value, to avoid overwriting
// an actual value in `colorPanelProps`.
if ( ! colors ) {
delete colorSettings[ newSettingIndex ].colors;
}

return acc;
}, {} );

const wrappedColorPanelProps = {
title: panelTitle,
colorSettings,
colorPanelProps,
contrastCheckerProps,
components: Object.values( components ),
panelChildren,
};
return {
...components,
ColorPanel: <ColorPanel { ...wrappedColorPanelProps } />,
InspectorControlsColorPanel: (
<InspectorControlsColorPanel { ...wrappedColorPanelProps } />
),
};
}, [ attributes, setAttributes, ...deps ] );
Copy link
Member

Choose a reason for hiding this comment

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

Should color config be a dependency here? colorConfig may totally change the expected output of the component. I guess we did not added it as a dependency because if users pass an array inline on each re-render a new reference is passed, could we automatically generate the dependencies from the config e.g: generate an array of [ name, attribute, name, attribute ...], If we also pass all properties of panel setting props we could avoid the need for dependencies.

Currently, we are relying on devs passing the dependencies and I think it will be something developers will easily miss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

colorConfig shouldn't be dynamic in most cases. If it is, they can use deps to invalidate it. This is a better DX than having to memoize colorConfig for all simple use cases.

}
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export { default as AlignmentToolbar } from './alignment-toolbar';
export { default as Autocomplete } from './autocomplete';
export { default as BlockAlignmentToolbar } from './block-alignment-toolbar';
export { default as BlockControls } from './block-controls';
export { default as BlockEdit } from './block-edit';
export { default as BlockEdit, useBlockEditContext } from './block-edit';
Copy link
Member

Choose a reason for hiding this comment

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

Should we add this export also to the native version of the file packages/block-editor/src/components/index.native.js?

Otherwise, headings on mobile will break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch!

export { default as BlockFormatControls } from './block-format-controls';
export { default as BlockIcon } from './block-icon';
export { default as BlockNavigationDropdown } from './block-navigation/dropdown';
Expand Down
96 changes: 35 additions & 61 deletions packages/block-library/src/heading/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,48 +13,30 @@ import HeadingToolbar from './heading-toolbar';
*/
import { __ } from '@wordpress/i18n';
import { PanelBody } from '@wordpress/components';
import { compose } from '@wordpress/compose';
import { createBlock } from '@wordpress/blocks';
import {
AlignmentToolbar,
BlockControls,
InspectorControls,
RichText,
withColors,
PanelColorSettings,
__experimentalUseColors,
} from '@wordpress/block-editor';
import { memo } from '@wordpress/element';

const HeadingColorUI = memo(
function( {
textColorValue,
setTextColor,
} ) {
return (
<PanelColorSettings
title={ __( 'Color Settings' ) }
initialOpen={ false }
colorSettings={ [
{
value: textColorValue,
onChange: setTextColor,
label: __( 'Text Color' ),
},
] }
/>
);
}
);

function HeadingEdit( {
attributes,
setAttributes,
mergeBlocks,
onReplace,
className,
textColor,
setTextColor,
} ) {
const { TextColor, InspectorControlsColorPanel } = __experimentalUseColors(
Copy link
Contributor

Choose a reason for hiding this comment

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

Good work, let's try to open follow-up PRs to use in other blocks to see how it behaves, if it's good enough.

Copy link
Member

Choose a reason for hiding this comment

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

What does <TextColor> do? This doesn't seem to be documented anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

It applies the text color as inline style to the wrapped component.

Copy link
Member

Choose a reason for hiding this comment

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

Why not pass the style directly to the element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's less verbose this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on the fence personally, if it's just about "style", I guess being explicit is better (passing styles). But I believe it also handles classNames and merging these props which ultiimately can result in a lot of duplicated code.

Copy link
Member

Choose a reason for hiding this comment

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

It looks a bit like magic right now. It's not very obvious what it does. I'll look around sometime to see how it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I believe it also handles classNames and merging these props which ultiimately can result in a lot of duplicated code.

Yes

[ { name: 'textColor', attribute: 'color' } ],
Copy link
Member

Choose a reason for hiding this comment

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

At the start, our colors API explicitly made block developers identify what was the attribute and what was the CSS property:

colorContext: 'color',
colorAttribute: 'textColor',

People provided feedback in reviews that maybe we could just use something in this format { textColor: 'color' }, where the key represents the Gutenberg attribute and the value of the CSS context/property where the color is used. Would not this format fit here? What were the reasons for the change and go back to explicitly identify the Gutenberg attribute and CSS property?

We have functions and docs where we call the CSS property 'color' the color context. And currently, some code that exists outside of the hook calls 'textColor' colorAttributeName. I think naming 'color' attribute may be confusing. Could we keep the current name 'context'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{ textColor: 'color' } is super confusing and not self explanatory.

color here is an attribute, it's not context. I think it makes a lot more sense to stick to the terminology that the web platform uses.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @epiqueras, The problem is that the term attribute in Gutenberg has a very specific meaning. I think people will get confused.
On the web, we normally call "color", "background-color" CSS properties. I think if we call "property" we still stick to the terminology that the web platform uses while removing a source of possible confusion for developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, property makes more sense.

{
contrastCheckerProps: { backgroundColor: 'white', isLargeText: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you decide which color from the first argument to use in the color contrast checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks all of them.

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 we should not check the contrast of all the colors. Imagining a scenario where we use the hook with background color and text color, the contrast checked should not check the background color.
Another point is that I think we should not hardcode a color for contrast checking (currently we assume the background is always white). Themes may set the heading background to block, and in that case, white text color is perfectly fine.
I think users should be able to configure the color pairs used in contrast checking.
We currently also have some mechanisms to retrieve colors using browser computed styles functions when they are not set. This mechanism will need to be improved, e.g., it was created before nesting existed and didn't handle it. As an example of the imporvement needed for nesting, If we have a heading with black color inside a group block with block background color if the theme does not explicitly set a heading background color we should show a contrast warning.
Having all contrast functionality in this hook makes it very huge. I think the contrast checking should be outside of this hook, going in the direction of what @mcsf referred (multiple hooks) the developers use useColor for the color logic useContrast for the contrast logic.
The contrast hook can contain the logic of the fallback styles we currently have, and receive some color values retrieved from useColors and simply returns a warning message or null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should not check the contrast of all the colors. Imagining a scenario where we use the hook with background color and text color, the contrast checked should not check the background color.

They can separate it into two hook calls or provide their own custom ConstrastChecker child.

Another point is that I think we should not hardcode a color for contrast checking (currently we assume the background is always white). Themes may set the heading background to block, and in that case, white text color is perfectly fine.

Nothing stops that backgroundColor property from being a variable.

This mechanism will need to be improved, e.g., it was created before nesting existed and didn't handle it. As an example of the imporvement needed for nesting, If we have a heading with black color inside a group block with block background color if the theme does not explicitly set a heading background color we should show a contrast warning.

I think that sort of contrast checking behavior belongs in another hook in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Should on the heading we use a hardcoded value for background on contracts checking of white? By default, the default WordPress theme does not contain a white background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I changed it to detect the background of the nearest parent that has one.

},
[]
);

const { align, content, level, placeholder } = attributes;
const tagName = 'h' + level;

Expand All @@ -71,43 +53,35 @@ function HeadingEdit( {
<p>{ __( 'Level' ) }</p>
<HeadingToolbar isCollapsed={ false } minLevel={ 1 } maxLevel={ 7 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
</PanelBody>
<HeadingColorUI
setTextColor={ setTextColor }
textColorValue={ textColor.color }
/>
</InspectorControls>
<RichText
identifier="content"
tagName={ tagName }
value={ content }
onChange={ ( value ) => setAttributes( { content: value } ) }
onMerge={ mergeBlocks }
onSplit={ ( value ) => {
if ( ! value ) {
return createBlock( 'core/paragraph' );
}
{ InspectorControlsColorPanel }
<TextColor>
Copy link
Member

Choose a reason for hiding this comment

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

It seems this component would also be useful in the save function as the classes/styles we should add follow the same logic. Do you think it would be possible to have something identical there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would be nice, but first we need to mock the React dispatcher in our serializer so that hooks don't throw errors.

<RichText
identifier="content"
tagName={ tagName }
value={ content }
onChange={ ( value ) => setAttributes( { content: value } ) }
onMerge={ mergeBlocks }
onSplit={ ( value ) => {
if ( ! value ) {
return createBlock( 'core/paragraph' );
}

return createBlock( 'core/heading', {
...attributes,
content: value,
} );
} }
onReplace={ onReplace }
onRemove={ () => onReplace( [] ) }
className={ classnames( className, {
[ `has-text-align-${ align }` ]: align,
'has-text-color': textColor.color,
[ textColor.class ]: textColor.class,
} ) }
placeholder={ placeholder || __( 'Write heading…' ) }
style={ {
color: textColor.color,
} }
/>
return createBlock( 'core/heading', {
...attributes,
content: value,
} );
} }
onReplace={ onReplace }
onRemove={ () => onReplace( [] ) }
className={ classnames( className, {
[ `has-text-align-${ align }` ]: align,
} ) }
placeholder={ placeholder || __( 'Write heading…' ) }
/>
</TextColor>
</>
);
}

export default compose( [
withColors( 'backgroundColor', { textColor: 'color' } ),
] )( HeadingEdit );
export default HeadingEdit;