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 all 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';
207 changes: 207 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,207 @@
/**
* 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, index ) => (
<ContrastChecker
key={ Component.displayName }
textColor={ colorSettings[ index ].value }
{ ...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(
( property, color, colorValue, customColor ) => ( { children } ) =>
// Clone children, setting the style property 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( property ) }`;
style = { [ property ]: colorValue, ...child.props.style };
} else if ( customColor ) {
className = `${ child.props.className } has-${ kebabCase( property ) }`;
style = { [ property ]: 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'.
property = 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(
property,
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 ?
_color.color :
attributes[ camelCase( `custom ${ name }` ) ],
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
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/index.native.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Block Creation Components
export { default as BlockControls } from './block-controls';
export { default as BlockEdit } from './block-edit';
export { default as BlockEdit, useBlockEditContext } from './block-edit';
export { default as BlockFormatControls } from './block-format-controls';
export { default as BlockIcon } from './block-icon';
export { default as BlockVerticalAlignmentToolbar } from './block-vertical-alignment-toolbar';
Expand Down
Loading