-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Introduce a support key for support global style colors in blocks #21021
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
Changes from all commits
dded0b1
6bedd0f
a7805db
f9b812b
37e578a
34f0584
d25356b
a9cc665
8812560
45a84dd
076224b
59eb590
1468aaa
92c9e3b
72d9905
583f1b2
dfca58c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,263 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import classnames from 'classnames'; | ||
| import { pickBy, isEqual, isObject, identity, mapValues } from 'lodash'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { addFilter } from '@wordpress/hooks'; | ||
| import { hasBlockSupport } from '@wordpress/blocks'; | ||
| import { createHigherOrderComponent } from '@wordpress/compose'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { useState, useEffect } from '@wordpress/element'; | ||
| import { useSelect } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { | ||
| getColorClassName, | ||
| getColorObjectByColorValue, | ||
| getColorObjectByAttributeValues, | ||
| } from '../components/colors'; | ||
| import PanelColorSettings from '../components/panel-color-settings'; | ||
| import ContrastChecker from '../components/contrast-checker'; | ||
| import InspectorControls from '../components/inspector-controls'; | ||
| import { getBlockDOMNode } from '../utils/dom'; | ||
|
|
||
| export const COLOR_SUPPORT_KEY = '__experimentalColor'; | ||
|
|
||
| export const cleanEmptyObject = ( object ) => { | ||
| if ( ! isObject( object ) ) { | ||
| return object; | ||
| } | ||
| const cleanedNestedObjects = pickBy( | ||
| mapValues( object, cleanEmptyObject ), | ||
| identity | ||
| ); | ||
| return isEqual( cleanedNestedObjects, {} ) | ||
| ? undefined | ||
| : cleanedNestedObjects; | ||
| }; | ||
|
|
||
| /** | ||
| * Filters registered block settings, extending attributes to include | ||
| * `backgroundColor` and `textColor` attribute. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we try to have a "style" attribute that blocks and global styles can use to store global styles related information, instead of using the hook to register a new attribute?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's already the case in this PR. These extra attributes are needed for the color palette classNames unless there's a way to define these with global styles that I'm not aware of?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think @mtias suggested that instead of using classes for predefined colors we should still use CSS vars anyway but the CSS vars would reference another vars.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we may have a way to store things in style that indicates another variable is being referenced.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can I have an example of CSS for named colors using variables?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in theme.json a theme may provide something like: This would generate the following vars during the compilation: When storing a color attribute if the value is equal to one of the color variables instead of storing the value we would store a reference to the variable: In the style attribute, we may use the var directly This solution has some disadvantages when compared with class usage:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting proposal and I can be onboard with that. Though, that's a bigger separate project. If we do this, we need to do it across blocks by measuring the pros and cons and communicating the changes properly. For this PR, I think we can keep the existing behavior and it can still be adapted in the future if we use CSS variables for palettes too.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we would use CSS variables for registered theme colors, we could avoid having all those class names stored in HTML content generated by the I think it feels like the very next step to explore. In theory, it could also open doors for having only one attribute used per color type as you could use CSS syntax for values. |
||
| * | ||
| * @param {Object} settings Original block settings | ||
| * @return {Object} Filtered block settings | ||
| */ | ||
| function addAttributes( settings ) { | ||
| if ( ! hasBlockSupport( settings, COLOR_SUPPORT_KEY ) ) { | ||
| return settings; | ||
| } | ||
|
|
||
| // allow blocks to specify their own attribute definition with default values if needed. | ||
| if ( ! settings.attributes.backgroundColor ) { | ||
| Object.assign( settings.attributes, { | ||
| backgroundColor: { | ||
| type: 'string', | ||
| }, | ||
| } ); | ||
| } | ||
| if ( ! settings.attributes.textColor ) { | ||
| Object.assign( settings.attributes, { | ||
| textColor: { | ||
| type: 'string', | ||
| }, | ||
| } ); | ||
| } | ||
|
|
||
| return settings; | ||
| } | ||
|
|
||
| /** | ||
| * Override props assigned to save component to inject colors classnames. | ||
| * | ||
| * @param {Object} props Additional props applied to save element | ||
| * @param {Object} blockType Block type | ||
| * @param {Object} attributes Block attributes | ||
| * @return {Object} Filtered props applied to save element | ||
| */ | ||
| export function addSaveProps( props, blockType, attributes ) { | ||
| if ( ! hasBlockSupport( blockType, COLOR_SUPPORT_KEY ) ) { | ||
| return props; | ||
| } | ||
|
|
||
| // I'd have prefered to avoid the "style" attribute usage here | ||
| const { backgroundColor, textColor, style } = attributes; | ||
|
|
||
| const backgroundClass = getColorClassName( | ||
| 'background-color', | ||
| backgroundColor | ||
| ); | ||
| const textClass = getColorClassName( 'color', textColor ); | ||
| props.className = classnames( props.className, backgroundClass, textClass, { | ||
| 'has-text-color': textColor || style?.color?.text, | ||
| 'has-background': backgroundColor || style?.color?.background, | ||
| } ); | ||
|
|
||
| return props; | ||
| } | ||
|
|
||
| /** | ||
| * Filters registered block settings to extand the block edit wrapper | ||
| * to apply the desired styles and classnames properly. | ||
| * | ||
| * @param {Object} settings Original block settings | ||
| * @return {Object} Filtered block settings | ||
| */ | ||
| export function addEditProps( settings ) { | ||
| if ( ! hasBlockSupport( settings, COLOR_SUPPORT_KEY ) ) { | ||
| return settings; | ||
| } | ||
| const existingGetEditWrapperProps = settings.getEditWrapperProps; | ||
| settings.getEditWrapperProps = ( attributes ) => { | ||
| let props = {}; | ||
| if ( existingGetEditWrapperProps ) { | ||
| props = existingGetEditWrapperProps( attributes ); | ||
| } | ||
| return addSaveProps( props, settings, attributes ); | ||
| }; | ||
|
|
||
| return settings; | ||
| } | ||
|
|
||
| const ColorPanel = ( { colorSettings, clientId } ) => { | ||
| const { getComputedStyle, Node } = window; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it’s not that different from what we had before but if you would see a way to extract a hook that operates on DOM and put it in its own file it might make it easier to replicate this behavior in React Native. Well, the best case scenario is it becomes no-op there 😃
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's similar and I agree, it seems like it could be its own hook. |
||
|
|
||
| const [ detectedBackgroundColor, setDetectedBackgroundColor ] = useState(); | ||
| const [ detectedColor, setDetectedColor ] = useState(); | ||
|
|
||
| useEffect( () => { | ||
| const colorsDetectionElement = getBlockDOMNode( clientId ); | ||
| setDetectedColor( getComputedStyle( colorsDetectionElement ).color ); | ||
|
|
||
| let backgroundColorNode = colorsDetectionElement; | ||
| let backgroundColor = getComputedStyle( backgroundColorNode ) | ||
| .backgroundColor; | ||
| while ( | ||
| backgroundColor === 'rgba(0, 0, 0, 0)' && | ||
| backgroundColorNode.parentNode && | ||
| backgroundColorNode.parentNode.nodeType === Node.ELEMENT_NODE | ||
| ) { | ||
| backgroundColorNode = backgroundColorNode.parentNode; | ||
| backgroundColor = getComputedStyle( backgroundColorNode ) | ||
| .backgroundColor; | ||
| } | ||
|
|
||
| setDetectedBackgroundColor( backgroundColor ); | ||
| } ); | ||
|
|
||
| return ( | ||
| <InspectorControls> | ||
| <PanelColorSettings | ||
| title={ __( 'Color settings' ) } | ||
| initialOpen={ false } | ||
| colorSettings={ colorSettings } | ||
| > | ||
| <ContrastChecker | ||
| backgroundColor={ detectedBackgroundColor } | ||
| textColor={ detectedColor } | ||
| /> | ||
| </PanelColorSettings> | ||
| </InspectorControls> | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Override the default edit UI to include new inspector controls for block | ||
| * color, if block defines support. | ||
| * | ||
| * @param {Function} BlockEdit Original component | ||
| * @return {Function} Wrapped component | ||
| */ | ||
| export const withBlockControls = createHigherOrderComponent( | ||
| ( BlockEdit ) => ( props ) => { | ||
| const { name: blockName } = props; | ||
| const colors = useSelect( ( select ) => { | ||
| return select( 'core/block-editor' ).getSettings().colors; | ||
| }, [] ); | ||
|
|
||
| if ( ! hasBlockSupport( blockName, COLOR_SUPPORT_KEY ) ) { | ||
| return <BlockEdit key="edit" { ...props } />; | ||
| } | ||
| const { style, textColor, backgroundColor } = props.attributes; | ||
|
|
||
| const onChangeColor = ( name ) => ( value ) => { | ||
| const colorObject = getColorObjectByColorValue( colors, value ); | ||
| const attributeName = name + 'Color'; | ||
| const newStyle = { | ||
| ...style, | ||
| color: { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like here we should filter out keys that were set to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not easy since it's nested and we should filter out the ones that are not coming from this particular hook.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't the end of the world if those values are stored as empty, it's just nice optimization to have in place as it is technically not necessary. |
||
| ...style?.color, | ||
| [ name ]: colorObject?.slug ? undefined : value, | ||
| }, | ||
| }; | ||
| const newNamedColor = colorObject?.slug | ||
| ? colorObject.slug | ||
| : undefined; | ||
| props.setAttributes( { | ||
| style: cleanEmptyObject( newStyle ), | ||
| [ attributeName ]: newNamedColor, | ||
| } ); | ||
| }; | ||
|
|
||
| return [ | ||
| <ColorPanel | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are cases that are not background color or text color, e.g: blocks may allow choosing border color and hoover colors: Other blocks have multiple text areas (multiple rich text elements) and have multiple color pickers for each text area plus color pickers for the separators: For these cases, the plan is to expand this API in the future? Or should they use the UI components plus useColors or the UI Components plus withColors?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me, probably a custom implementation using the reusable components. I'm still not sure about withColors or useColors, I'm not convinced by both. We'll see.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't worry about it too much at the moment. It will naturally evolve together with the development of other design-related features lead by @ItsJonQ. At some point, I expect that Global Styles and the block editor features API will intersect with this logic. This PR provides the best possible defaults that make the most sense as confirmed in the follow-up PRs. Once we introduce more color-related features like borders, hover and focus states, we could control them with block editor features per site or per block.
It raises the question of whether they could be refactored to use nested blocks that would solve the issue. |
||
| key="colors" | ||
| clientId={ props.clientId } | ||
| colorSettings={ [ | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had trouble understanding the API of the PanelColorSettings. I believe it should be documented properly. |
||
| { | ||
| label: __( 'Text Color' ), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't we going to support configurable labels? E.g: color property may not be a text color in some contexts it may be an icon color for example, in the cover a background color is not a background it is "Overlay".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's not a text color or a background color, the CSS variable names and global styles config doesn't make sense, so this hook shouldn't be used in that case. |
||
| onChange: onChangeColor( 'text' ), | ||
| colors, | ||
| value: getColorObjectByAttributeValues( | ||
| colors, | ||
| textColor, | ||
| style?.color?.text | ||
| ).color, | ||
| }, | ||
| { | ||
| label: __( 'Background Color' ), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are assuming a block will always have a text and a background color, currently, the heading block just has a text color. Should we still support these use cases?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, It's easy enough (commented about it on #21039 ), I'm trying to see how far we can go without any config.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In cases that support gradient backgrounds, the UI for gradients should appear in the same panel as colors, should these hooks also handle gradients?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I wonder if we should auto-support gradients for all blocks that support background colors but regardless, I believe this hook should support gradients in the future.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About whether we should auto-add the sub-element when gradients are used, ideally no but we might have to, the goal is to always be generic though as we need to be able to support consistent features across blocks way more quickly than what we do today. If we sacrifice some flexibility sometimes, it's fine. |
||
| onChange: onChangeColor( 'background' ), | ||
| colors, | ||
| value: getColorObjectByAttributeValues( | ||
| colors, | ||
| backgroundColor, | ||
| style?.color?.background | ||
| ).color, | ||
| }, | ||
| ] } | ||
| />, | ||
| <BlockEdit key="edit" { ...props } />, | ||
| ]; | ||
| }, | ||
| 'withToolbarControls' | ||
| ); | ||
|
|
||
| addFilter( | ||
| 'blocks.registerBlockType', | ||
| 'core/color/addAttribute', | ||
| addAttributes | ||
| ); | ||
|
|
||
| addFilter( | ||
| 'blocks.getSaveContent.extraProps', | ||
| 'core/color/addSaveProps', | ||
| addSaveProps | ||
| ); | ||
|
|
||
| addFilter( | ||
| 'blocks.registerBlockType', | ||
| 'core/color/addEditProps', | ||
| addEditProps | ||
| ); | ||
|
|
||
| addFilter( | ||
| 'editor.BlockEdit', | ||
| 'core/color/with-block-controls', | ||
| withBlockControls | ||
| ); | ||


Uh oh!
There was an error while loading. Please reload this page.