Skip to content

Commit

Permalink
Testing out generating block color classnames using the style engine.
Browse files Browse the repository at this point in the history
  • Loading branch information
ramonjd committed May 9, 2022
1 parent 2cb4e18 commit 393fac4
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 37 deletions.
66 changes: 32 additions & 34 deletions packages/block-editor/src/hooks/color.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@ import { createHigherOrderComponent } from '@wordpress/compose';
* Internal dependencies
*/
import {
getColorClassName,
getColorObjectByColorValue,
getColorObjectByAttributeValues,
} from '../components/colors';
import {
__experimentalGetGradientClass,
getGradientValueBySlug,
getGradientSlugByValue,
} from '../components/gradients';
Expand All @@ -34,6 +32,7 @@ import {
} from './utils';
import ColorPanel from './color-panel';
import useSetting from '../components/use-setting';
import { getClassnames } from '@wordpress/style-engine';

export const COLOR_SUPPORT_KEY = 'color';

Expand Down Expand Up @@ -252,52 +251,51 @@ export function addSaveProps( props, blockType, attributes ) {
return props;
}

const hasGradient = hasGradientSupport( blockType );

// I'd have preferred to avoid the "style" attribute usage here
const { backgroundColor, textColor, gradient, style } = attributes;

const shouldSerialize = ( feature ) =>
! shouldSkipSerialization( blockType, COLOR_SUPPORT_KEY, feature );

const colorStyles = {};

// Primary color classes must come before the `has-text-color`,

This comment has been minimized.

Copy link
@andrewserong

andrewserong May 9, 2022

Contributor

Cool idea exploring concatenating to var:preset|color|${ textColor } @ramonjd!

To respect this comment about the order that the classes need to be generated in, I wonder if that means we should also update the export in

export default [ background, gradient, text ];
so that it's in the same order as the lines in this file?

So:

export default [ text, gradient, background ];

Not sure if that'd help resolve some of the fixture failures? (Apologies if I'm way off here 😀)

This comment has been minimized.

Copy link
@ramonjd

ramonjd May 9, 2022

Author Member

Thanks for checking this out.

The preset value is an attempt to follow the logic we added in the backend https://github.com/WordPress/gutenberg/blob/trunk/lib/block-supports/colors.php#L84

To respect this comment about the order that the classes need to be generated in, I wonder if that means we should also update the export

I took this to mean that the has-something-color needs to be output before the generic has-text-color, which is something I made sure of in the text.ts file in this PR.

Swapping the order doesn't hurt though either though. It doesn't fix the fixture failures since those expect both generic classes to be at the end, e.g.,

has-white-color has-black-background-color has-text-color has-background

But I reasoned that there's little difference between that and

has-white-color has-text-color has-black-background-color has-background

so long as the generic classname comes after it's specific cousin.

I think most of them are due to the order anyway, so I can regenerate them.

This comment has been minimized.

Copy link
@ramonjd

ramonjd May 9, 2022

Author Member

By the way, I'm not going to say I'm 100% sure of anything so please correct me if I've got things muddled up 😄

This comment has been minimized.

Copy link
@andrewserong

andrewserong May 9, 2022

Contributor

Ah, excellent, thanks for clarifying that! That all makes sense to me, and sounds like it'll be safe for us to just regenerate the fixtures, like you mention 👍

// `has-background` and `has-link-color` classes to maintain backwards
// compatibility and avoid block invalidations.
const textClass = shouldSerialize( 'text' )
? getColorClassName( 'color', textColor )
: undefined;

const gradientClass = shouldSerialize( 'gradients' )
? __experimentalGetGradientClass( gradient )
: undefined;
if ( textColor && shouldSerialize( 'text' ) ) {
colorStyles.text = `var:preset|color|${ textColor }`;
}

const backgroundClass = shouldSerialize( 'background' )
? getColorClassName( 'background-color', backgroundColor )
: undefined;
if (
gradient &&
shouldSerialize( 'gradients' ) &&
hasGradientSupport( blockType )
) {
colorStyles.gradient = `var:preset|gradient|${ gradient }`;
}

const serializeHasBackground =
shouldSerialize( 'background' ) || shouldSerialize( 'gradients' );
const hasBackground =
backgroundColor ||
style?.color?.background ||
( hasGradient && ( gradient || style?.color?.gradient ) );
if ( backgroundColor && shouldSerialize( 'background' ) ) {
colorStyles.background = `var:preset|color|${ backgroundColor }`;
}

const newClassName = classnames(
props.className,
textClass,
gradientClass,
{
// Don't apply the background class if there's a custom gradient.
[ backgroundClass ]:
( ! hasGradient || ! style?.color?.gradient ) &&
!! backgroundClass,
'has-text-color':
shouldSerialize( 'text' ) &&
( textColor || style?.color?.text ),
'has-background': serializeHasBackground && hasBackground,
'has-link-color':
shouldSerialize( 'link' ) && style?.elements?.link?.color,
}
...getClassnames( {
...style,
color: {
...colorStyles,
},
elements: {
...style?.elements,
link: {
color: {
text: shouldSerialize( 'link' )
? style?.elements?.link?.color
: undefined,
},
},
},
} )
);
props.className = newClassName ? newClassName : undefined;

Expand Down
4 changes: 3 additions & 1 deletion packages/style-engine/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ export function getCSSRules(
): GeneratedCSSRule[] {
const rules: GeneratedCSSRule[] = [];
styleDefinitions.forEach( ( definition: StyleDefinition ) => {
rules.push( ...definition.generate( style, options ) );
if ( typeof definition.generate === 'function' ) {
rules.push( ...definition.generate( style, options ) );
}
} );

return rules;
Expand Down
20 changes: 20 additions & 0 deletions packages/style-engine/src/styles/elements/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Internal dependencies
*/
import type { Style } from '../../types';

const link = {
name: 'link',
getClassNames: ( style: Style ) => {
const classNames = [];
const styleValue: string | undefined =
style?.elements?.link?.color?.text;
if ( styleValue ) {
classNames.push( 'has-link-color' );
}

return classNames;
},
};

export default [ link ];
8 changes: 7 additions & 1 deletion packages/style-engine/src/styles/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@
* Internal dependencies
*/
import color from './color';
import elements from './elements';
import spacing from './spacing';
import typography from './typography';

export const styleDefinitions = [ ...color, ...spacing, ...typography ];
export const styleDefinitions = [
...color,
...elements,
...spacing,
...typography,
];
9 changes: 8 additions & 1 deletion packages/style-engine/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ export interface Style {
background?: CSSProperties[ 'backgroundColor' ];
gradient?: CSSProperties[ 'background' ];
};
elements?: {
link?: {
color?: {
text?: CSSProperties[ 'color' ];
};
};
};
}

export type StyleOptions = {
Expand All @@ -52,6 +59,6 @@ export type GeneratedCSSRule = {

export interface StyleDefinition {
name: string;
generate: ( style: Style, options: StyleOptions ) => GeneratedCSSRule[];
generate?: ( style: Style, options: StyleOptions ) => GeneratedCSSRule[];
getClassNames?: ( style: Style ) => string[];
}

0 comments on commit 393fac4

Please sign in to comment.