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

Update: Remove duplicate style mappings from global styles #25145

Closed
wants to merge 1 commit into from
Closed

Update: Remove duplicate style mappings from global styles #25145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 25 additions & 19 deletions packages/edit-site/src/components/editor/global-styles-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
/**
* External dependencies
*/
import { get } from 'lodash';
import { get, kebabCase, reduce } from 'lodash';

/**
* WordPress dependencies
*/
import { __EXPERIMENTAL_STYLE_MAPPINGS } from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import {
STYLE_PROPS,
PRESET_CATEGORIES,
LINK_COLOR_DECLARATION,
} from './utils';
import { PRESET_CATEGORIES, LINK_COLOR_DECLARATION } from './utils';

const mergeTrees = ( baseData, userData ) => {
// Deep clone from base data.
Expand Down Expand Up @@ -55,19 +56,24 @@ export default ( blockData, baseTree, userTree ) => {
* @return {Array} An array of style declarations.
*/
const getBlockStylesDeclarations = ( blockSupports, blockStyles ) => {
const declarations = [];
Object.keys( STYLE_PROPS ).forEach( ( key ) => {
if (
blockSupports.includes( key ) &&
get( blockStyles, STYLE_PROPS[ key ], false )
) {
declarations.push(
`${ key }: ${ get( blockStyles, STYLE_PROPS[ key ] ) }`
);
}
} );

return declarations;
return reduce(
__EXPERIMENTAL_STYLE_MAPPINGS,
function ( declarations, stylePath, rawKey ) {
const key = rawKey.startsWith( '--' )
? rawKey
: kebabCase( rawKey );
if (
blockSupports.includes( key ) &&
get( blockStyles, stylePath, false )
) {
declarations.push(
`${ key }: ${ get( blockStyles, stylePath ) }`
);
}
return declarations;
},
[]
);
};

/**
Expand Down
13 changes: 0 additions & 13 deletions packages/edit-site/src/components/editor/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,9 @@
/* CSS properties */
export const FONT_SIZE = 'font-size';
export const LINK_COLOR = '--wp--style--color--link';
Copy link
Member

Choose a reason for hiding this comment

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

Don't we move the link color prop as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

The link color is used on this file and the other file so it makes sense to keep it as is and share it.

export const BACKGROUND_COLOR = 'background-color';
export const GRADIENT_COLOR = 'background';
export const TEXT_COLOR = 'color';
export const LINE_HEIGHT = 'line-height';

/* Supporting data */
export const GLOBAL_CONTEXT = 'global';
export const PRESET_CATEGORIES = [ 'color', 'font-size', 'gradient' ];
export const STYLE_PROPS = {
[ FONT_SIZE ]: 'typography.fontSize',
[ LINE_HEIGHT ]: 'typography.lineHeight',
[ TEXT_COLOR ]: 'color.text',
[ BACKGROUND_COLOR ]: 'color.background',
[ GRADIENT_COLOR ]: 'color.gradient',
[ LINK_COLOR ]: 'color.link',
};
export const LINK_COLOR_DECLARATION = `a { color: var(${ LINK_COLOR }, #00e); }`;

/* Helpers for unit processing */
Expand Down
11 changes: 5 additions & 6 deletions packages/edit-site/src/components/sidebar/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import {
BACKGROUND_COLOR,
LINK_COLOR,
TEXT_COLOR,
GRADIENT_COLOR,
} from '../editor/utils';
import { LINK_COLOR } from '../editor/utils';

const BACKGROUND_COLOR = 'background-color';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these variables be moved to hooks/utils as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is fine to use background-color when needed. We are at the panel label so probably the panel should know the supports key is background-color. Moving the variables would only mean one uses BACKGROUND_COLOR instead of background-color which is not a big win.

const GRADIENT_COLOR = 'background';
const TEXT_COLOR = 'color';

export default ( {
context: { supports, name },
Expand Down
5 changes: 4 additions & 1 deletion packages/edit-site/src/components/sidebar/typography-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { FONT_SIZE, LINE_HEIGHT, fromPx, toPx } from '../editor/utils';
import { fromPx, toPx } from '../editor/utils';

const LINE_HEIGHT = 'line-height';
const FONT_SIZE = 'font-size';

export default ( {
context: { supports, name },
Expand Down