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

Make core colors classes and custom properties always available #31669

Merged
merged 10 commits into from
May 20, 2021
Merged

Make core colors classes and custom properties always available #31669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
108f919
Modify the merge algorithm so all the colors are kept
oandregal May 10, 2021
e0b6d6c
Do not show core colors in the UI controls
oandregal May 14, 2021
4ddd1de
Color palette and gradients are merged
oandregal May 18, 2021
b7fbedf
Do not add palette and gradients if empty
oandregal May 18, 2021
3aa6568
Fix linting issues
oandregal May 18, 2021
ee4ffe7
Cover the case in which the theme does not provide any color
oandregal May 19, 2021
9faec4d
Update merge algorithm for user colors
oandregal May 19, 2021
0c4653f
Update merge algorithm for colors in the server
oandregal May 19, 2021
c9fc39f
Fix color ordering
oandregal May 20, 2021
d45d090
Fix linting issues
oandregal May 20, 2021
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
2 changes: 1 addition & 1 deletion lib/class-wp-theme-json-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ public static function get_merged_data( $settings = array(), $origin = 'user' )
$result->merge( self::get_theme_data( $theme_support_data ) );

if ( 'user' === $origin ) {
$result->merge( self::get_user_data() );
$result->merge( self::get_user_data(), 'update' );
}

return $result;
Expand Down
82 changes: 67 additions & 15 deletions lib/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1076,33 +1076,85 @@ public function get_stylesheet( $type = 'all' ) {
* Merge new incoming data.
*
* @param WP_Theme_JSON $incoming Data to merge.
* @param string $update_or_remove Whether update or remove existing colors
* for which the incoming data has a duplicated slug.
*/
public function merge( $incoming ) {
$incoming_data = $incoming->get_raw_data();
$this->theme_json = array_replace_recursive( $this->theme_json, $incoming_data );
public function merge( $incoming, $update_or_remove = 'remove' ) {
$incoming_data = $incoming->get_raw_data();
$existing_data = $this->theme_json;

// The array_replace_recursive algorithm merges at the leaf level.
// For leaf values that are arrays it will use the numeric indexes for replacement.
// In those cases, what we want is to use the incoming value, if it exists.
//
// These are the cases that have array values at the leaf levels.
$properties = array();
$properties[] = array( 'color', 'palette' );
$properties[] = array( 'color', 'gradients' );
$properties[] = array( 'custom' );
$properties[] = array( 'spacing', 'units' );
$properties[] = array( 'typography', 'fontSizes' );
$properties[] = array( 'typography', 'fontFamilies' );
$this->theme_json = array_replace_recursive( $this->theme_json, $incoming_data );

// There are a few cases in which we want to merge things differently
// from what array_replace_recursive does.

// Some incoming properties should replace the existing.
$to_replace = array();
$to_replace[] = array( 'custom' );
$to_replace[] = array( 'spacing', 'units' );
$to_replace[] = array( 'typography', 'fontSizes' );
$to_replace[] = array( 'typography', 'fontFamilies' );

// Some others should be appended to the existing.
// If the slug is the same than an existing element,
// the $update_or_remove param is used to decide
// what to do with the existing element:
// either remove it and append the incoming,
// or update it with the incoming.
$to_append = array();
$to_append[] = array( 'color', 'duotone' );
$to_append[] = array( 'color', 'gradients' );
Copy link
Member

Choose a reason for hiding this comment

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

Already existing issue: It seems we need to include array( 'color', 'duotone' ) in either
to_append or to_replace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did the same as the other color palettes: core is always enqueued and the theme is appended.

$to_append[] = array( 'color', 'palette' );

$nodes = self::get_setting_nodes( $this->theme_json );
foreach ( $nodes as $metadata ) {
foreach ( $properties as $property_path ) {
$path = array_merge( $metadata['path'], $property_path );
foreach ( $to_replace as $path_to_replace ) {
$path = array_merge( $metadata['path'], $path_to_replace );
$node = _wp_array_get( $incoming_data, $path, array() );
if ( ! empty( $node ) ) {
gutenberg_experimental_set( $this->theme_json, $path, $node );
}
}
foreach ( $to_append as $path_to_append ) {
$path = array_merge( $metadata['path'], $path_to_append );
$incoming_node = _wp_array_get( $incoming_data, $path, array() );
$existing_node = _wp_array_get( $existing_data, $path, array() );

if ( empty( $incoming_node ) && empty( $existing_node ) ) {
continue;
}

$index_table = array();
$existing_slugs = array();
$merged = array();
foreach ( $existing_node as $key => $value ) {
$index_table[ $value['slug'] ] = $key;
$existing_slugs[] = $value['slug'];
$merged[ $key ] = $value;
}

$to_remove = array();
foreach ( $incoming_node as $value ) {
if ( ! in_array( $value['slug'], $existing_slugs, true ) ) {
$merged[] = $value;
} elseif ( 'update' === $update_or_remove ) {
$merged[ $index_table[ $value['slug'] ] ] = $value;
} else {
$merged[] = $value;
$to_remove[] = $index_table[ $value['slug'] ];
}
}

// Remove the duplicated values and pack the sparsed array.
foreach ( $to_remove as $index ) {
unset( $merged[ $index ] );
}
$merged = array_values( $merged );

gutenberg_experimental_set( $this->theme_json, $path, $merged );
}
}

}
Expand Down
72 changes: 48 additions & 24 deletions lib/experimental-default-theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,124 +6,148 @@
{
"name": "Black",
"slug": "black",
"color": "#000000"
"color": "#000000",
"origin": "core"
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 redundant to have "origin": "core", in all of the colors. Here they are defined in the core theme.json so our code should automatically assume all these colors of core origin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, thought about that. These are the trade-offs I've considered:

  1. We only write this once and themes don't have to, it's something internal to core so it's transparent to the consumers of the API.
  2. If our code needs to augment that data, that needs to run for every request and makes the logic more complex.
  3. We don't know how the UI control that uses this data is going to work.

With these trade-offs, I leaned towards implementing the simplest thing that works and that can scale as our needs evolve.

Alternatives I've considered:

  • What if core colors (or colors not to show in the UI) were stored in a different part? Too complex to introduce at the stage we're in. We need to account for every level (top-level, blocks) and have access to both colors in the client for the case the theme doesn't provide any color.

  • What if we add a showInUi flag instead of origin? Will work the same, not super-opinionated. Although I think origin is more future proof.

  • What if we add the origin to all the colors? It's data we pass to the client that we don't need at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect others to use this property? Something more descriptive like an array of which contexts to display the colors in UI may make sense.

My intuition would lean toward removing this property in the definition especially if we're unsure of usages by other consumers. Folks will copy paste examples, so I'd be a little careful here. We could do something like adding it in the in-memory representation or alternatively picking a different data structure like "core": { "settings": { "color": "pallete" : {. (I don't know this problem space very well, so just my quick takes, it may not make sense for this problem.)

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've already shared my rationale about why I'd think going forward with the simplest approach that works is fine. However, one thing I didn't consider was the copy/paste rationale. That's a good point and makes me having second thoughts about this, to be honest.

},
{
"name": "Cyan bluish gray",
"slug": "cyan-bluish-gray",
"color": "#abb8c3"
"color": "#abb8c3",
"origin": "core"
},
{
"name": "White",
"slug": "white",
"color": "#ffffff"
"color": "#ffffff",
"origin": "core"
},
{
"name": "Pale pink",
"slug": "pale-pink",
"color": "#f78da7"
"color": "#f78da7",
"origin": "core"
},
{
"name": "Vivid red",
"slug": "vivid-red",
"color": "#cf2e2e"
"color": "#cf2e2e",
"origin": "core"
},
{
"name": "Luminous vivid orange",
"slug": "luminous-vivid-orange",
"color": "#ff6900"
"color": "#ff6900",
"origin": "core"
},
{
"name": "Luminous vivid amber",
"slug": "luminous-vivid-amber",
"color": "#fcb900"
"color": "#fcb900",
"origin": "core"
},
{
"name": "Light green cyan",
"slug": "light-green-cyan",
"color": "#7bdcb5"
"color": "#7bdcb5",
"origin": "core"
},
{
"name": "Vivid green cyan",
"slug": "vivid-green-cyan",
"color": "#00d084"
"color": "#00d084",
"origin": "core"
},
{
"name": "Pale cyan blue",
"slug": "pale-cyan-blue",
"color": "#8ed1fc"
"color": "#8ed1fc",
"origin": "core"
},
{
"name": "Vivid cyan blue",
"slug": "vivid-cyan-blue",
"color": "#0693e3"
"color": "#0693e3",
"origin": "core"
},
{
"name": "Vivid purple",
"slug": "vivid-purple",
"color": "#9b51e0"
"color": "#9b51e0",
"origin": "core"
}
],
"gradients": [
{
"name": "Vivid cyan blue to vivid purple",
"gradient": "linear-gradient(135deg,rgba(6,147,227,1) 0%,rgb(155,81,224) 100%)",
"slug": "vivid-cyan-blue-to-vivid-purple"
"slug": "vivid-cyan-blue-to-vivid-purple",
"origin": "core"
},
{
"name": "Light green cyan to vivid green cyan",
"gradient": "linear-gradient(135deg,rgb(122,220,180) 0%,rgb(0,208,130) 100%)",
"slug": "light-green-cyan-to-vivid-green-cyan"
"slug": "light-green-cyan-to-vivid-green-cyan",
"origin": "core"
},
{
"name": "Luminous vivid amber to luminous vivid orange",
"gradient": "linear-gradient(135deg,rgba(252,185,0,1) 0%,rgba(255,105,0,1) 100%)",
"slug": "luminous-vivid-amber-to-luminous-vivid-orange"
"slug": "luminous-vivid-amber-to-luminous-vivid-orange",
"origin": "core"
},
{
"name": "Luminous vivid orange to vivid red",
"gradient": "linear-gradient(135deg,rgba(255,105,0,1) 0%,rgb(207,46,46) 100%)",
"slug": "luminous-vivid-orange-to-vivid-red"
"slug": "luminous-vivid-orange-to-vivid-red",
"origin": "core"
},
{
"name": "Very light gray to cyan bluish gray",
"gradient": "linear-gradient(135deg,rgb(238,238,238) 0%,rgb(169,184,195) 100%)",
"slug": "very-light-gray-to-cyan-bluish-gray"
"slug": "very-light-gray-to-cyan-bluish-gray",
"origin": "core"
},
{
"name": "Cool to warm spectrum",
"gradient": "linear-gradient(135deg,rgb(74,234,220) 0%,rgb(151,120,209) 20%,rgb(207,42,186) 40%,rgb(238,44,130) 60%,rgb(251,105,98) 80%,rgb(254,248,76) 100%)",
"slug": "cool-to-warm-spectrum"
"slug": "cool-to-warm-spectrum",
"origin": "core"
},
{
"name": "Blush light purple",
"gradient": "linear-gradient(135deg,rgb(255,206,236) 0%,rgb(152,150,240) 100%)",
"slug": "blush-light-purple"
"slug": "blush-light-purple",
"origin": "core"
},
{
"name": "Blush bordeaux",
"gradient": "linear-gradient(135deg,rgb(254,205,165) 0%,rgb(254,45,45) 50%,rgb(107,0,62) 100%)",
"slug": "blush-bordeaux"
"slug": "blush-bordeaux",
"origin": "core"
},
{
"name": "Luminous dusk",
"gradient": "linear-gradient(135deg,rgb(255,203,112) 0%,rgb(199,81,192) 50%,rgb(65,88,208) 100%)",
"slug": "luminous-dusk"
"slug": "luminous-dusk",
"origin": "core"
},
{
"name": "Pale ocean",
"gradient": "linear-gradient(135deg,rgb(255,245,203) 0%,rgb(182,227,212) 50%,rgb(51,167,181) 100%)",
"slug": "pale-ocean"
"slug": "pale-ocean",
"origin": "core"
},
{
"name": "Electric grass",
"gradient": "linear-gradient(135deg,rgb(202,248,128) 0%,rgb(113,206,126) 100%)",
"slug": "electric-grass"
"slug": "electric-grass",
"origin": "core"
},
{
"name": "Midnight",
"gradient": "linear-gradient(135deg,rgb(2,3,129) 0%,rgb(40,116,252) 100%)",
"slug": "midnight"
"slug": "midnight",
"origin": "core"
}
],
"duotone": [
Expand Down
24 changes: 22 additions & 2 deletions packages/block-editor/src/components/use-setting/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@ const deprecatedFlags = {
'spacing.customPadding': ( settings ) => settings.enableCustomSpacing,
};

const filterColorsFromCoreOrigin = ( path, setting ) => {
if ( path !== 'color.palette' && path !== 'color.gradients' ) {
return setting;
}

if ( ! Array.isArray( setting ) ) {
return setting;
}

const colors = setting.filter( ( color ) => color?.origin !== 'core' );

return colors.length > 0 ? colors : setting;
};

/**
* Hook that retrieves the editor setting.
* It works with nested objects using by finding the value at path.
Expand Down Expand Up @@ -76,15 +90,21 @@ export default function useSetting( path ) {
const experimentalFeaturesResult =
get( settings, blockPath ) ?? get( settings, defaultsPath );
if ( experimentalFeaturesResult !== undefined ) {
return experimentalFeaturesResult;
return filterColorsFromCoreOrigin(
path,
experimentalFeaturesResult
);
}

// 2 - Use deprecated settings, otherwise.
const deprecatedSettingsValue = deprecatedFlags[ path ]
? deprecatedFlags[ path ]( settings )
: undefined;
if ( deprecatedSettingsValue !== undefined ) {
return deprecatedSettingsValue;
return filterColorsFromCoreOrigin(
path,
deprecatedSettingsValue
);
}

// 3 - Fall back for typography.dropCap:
Expand Down
26 changes: 25 additions & 1 deletion packages/edit-site/src/components/editor/global-styles-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,31 @@ const GlobalStylesContext = createContext( {
/* eslint-enable no-unused-vars */
} );

const mergeTreesCustomizer = ( objValue, srcValue ) => {
const mergeTreesCustomizer = ( objValue, srcValue, key ) => {
// Users can add their own colors.
// We want to append them when they don't
// have the same slug as an existing color,
// otherwise we want to update the existing color instead.
if ( 'palette' === key ) {
const indexTable = {};
const existingSlugs = [];
const result = [ ...( objValue ?? [] ) ];
result.forEach( ( { slug }, index ) => {
indexTable[ slug ] = index;
existingSlugs.push( slug );
} );

( srcValue ?? [] ).forEach( ( element ) => {
if ( existingSlugs.includes( element?.slug ) ) {
result[ indexTable[ element?.slug ] ] = element;
} else {
result.push( element );
}
} );

return result;
}

// We only pass as arrays the presets,
// in which case we want the new array of values
// to override the old array (no merging).
Expand Down
17 changes: 16 additions & 1 deletion packages/edit-site/src/components/editor/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,28 @@ function getPresetMetadataFromStyleProperty( styleProperty ) {
return getPresetMetadataFromStyleProperty.MAP[ styleProperty ];
}

const filterColorsFromCoreOrigin = ( path, setting ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this get out of sync with the implementation in block editor?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel ultimately here, we'd need a way to get both

useSetting( 'color.palette' ) // return theme colors
useSetting( 'color.defaultPalette' ) // return default palette

and same for other settings. It's not important now, but we should make sure it's something we can achieve later.

if ( path !== 'color.palette' && path !== 'color.gradients' ) {
return setting;
}

if ( ! Array.isArray( setting ) ) {
return setting;
}

const colors = setting.filter( ( color ) => color?.origin !== 'core' );

return colors.length > 0 ? colors : setting;
};

export function useSetting( path, blockName = '' ) {
const settings = useSelect( ( select ) => {
return select( editSiteStore ).getSettings();
} );
const topLevelPath = `__experimentalFeatures.${ path }`;
const blockPath = `__experimentalFeatures.blocks.${ blockName }.${ path }`;
return get( settings, blockPath ) ?? get( settings, topLevelPath );
const setting = get( settings, blockPath ) ?? get( settings, topLevelPath );
return filterColorsFromCoreOrigin( path, setting );
}

export function getPresetVariable( styles, context, propertyName, value ) {
Expand Down
Loading