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

Use same mappings for the style properties everywhere #24320

Closed
wants to merge 8 commits into from
Closed

Use same mappings for the style properties everywhere #24320

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ae4e783
Centralize mappings
oandregal Aug 2, 2020
7c1e46a
Organize style props within __experimentalStyles
oandregal Aug 2, 2020
bd3cce8
Global Styles: take data from __experimentalStyles key
oandregal Aug 2, 2020
9ad9900
Migrate styles test to new __experimentalStyles key
oandregal Aug 2, 2020
9df7f24
Migrate hooks to use __experimentalStyles
oandregal Aug 2, 2020
96e4c33
Remove old support keys
oandregal Aug 2, 2020
258ebcd
Make linter happy
oandregal Aug 2, 2020
71a5978
Make linter happy
oandregal Aug 2, 2020
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
45 changes: 27 additions & 18 deletions lib/global-styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,21 @@
* @package gutenberg
*/

/**
* Returns the paths to get the information about a particular
* style property from the block.json or the theme.json.
*/
function gutenberg_experimental_global_styles_get_mappings() {
return array(
'line-height' => array( 'typography', 'lineHeight' ),
'font-size' => array( 'typography', 'fontSize' ),
'background' => array( 'color', 'gradient' ),
'background-color' => array( 'color', 'background' ),
'color' => array( 'color', 'text' ),
'--wp--style--color--link' => array( 'color', 'link' )
);
}

/**
* Whether the current theme has a theme.json file.
*
Expand Down Expand Up @@ -269,19 +284,20 @@ function gutenberg_experimental_global_styles_get_theme() {
* @return array Style features supported by the block.
*/
function gutenberg_experimental_global_styles_get_supported_styles( $supports ) {
$style_features = array(
'--wp--style--color--link' => array( '__experimentalColor', 'linkColor' ),
'background-color' => array( '__experimentalColor' ),
'background' => array( '__experimentalColor', 'gradients' ),
'block-align' => array( 'align' ),
'color' => array( '__experimentalColor' ),
'font-size' => array( '__experimentalFontSize' ),
'line-height' => array( '__experimentalLineHeight' ),
$mappings = array_merge(
gutenberg_experimental_global_styles_get_mappings(),
// TODO: remove from here and access directly from lib/blocks.php
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the block-align property was added 0d08cda I think we can fix this in a subsequent PR: it should be removed from here and lib/blocks.php refactored to use the block registry instead of global-styles.php

array(
'block-align' => array( 'align' )
)
);

$supported_features = array();
foreach ( $style_features as $style_feature => $path ) {
if ( gutenberg_experimental_get( $supports, $path ) ) {
foreach ( $mappings as $style_feature => $path ) {
if ( gutenberg_experimental_get(
$supports,
array_merge( array( '__experimentalStyles' ) , $path )
) ) {
$supported_features[] = $style_feature;
}
}
Expand Down Expand Up @@ -370,14 +386,7 @@ function gutenberg_experimental_global_styles_get_block_data() {
* @return array Containing a set of css rules.
*/
function gutenberg_experimental_global_styles_flatten_styles_tree( $styles ) {
$mappings = array(
'line-height' => array( 'typography', 'lineHeight' ),
'font-size' => array( 'typography', 'fontSize' ),
'background' => array( 'color', 'gradient' ),
'background-color' => array( 'color', 'background' ),
'color' => array( 'color', 'text' ),
'--wp--style--color--link' => array( 'color', 'link' ),
);
$mappings = gutenberg_experimental_global_styles_get_mappings();

$result = array();

Expand Down
6 changes: 3 additions & 3 deletions packages/block-editor/src/hooks/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
import { cleanEmptyObject } from './utils';
import ColorPanel from './color-panel';

export const COLOR_SUPPORT_KEY = '__experimentalColor';
export const COLOR_SUPPORT_KEY = '__experimentalStyles.color';

const hasColorSupport = ( blockType ) =>
Platform.OS === 'web' && hasBlockSupport( blockType, COLOR_SUPPORT_KEY );
Expand All @@ -42,7 +42,7 @@ const hasLinkColorSupport = ( blockType ) => {

const colorSupport = getBlockSupport( blockType, COLOR_SUPPORT_KEY );

return isObject( colorSupport ) && !! colorSupport.linkColor;
return isObject( colorSupport ) && !! colorSupport.link;
};

const hasGradientSupport = ( blockType ) => {
Expand All @@ -52,7 +52,7 @@ const hasGradientSupport = ( blockType ) => {

const colorSupport = getBlockSupport( blockType, COLOR_SUPPORT_KEY );

return isObject( colorSupport ) && !! colorSupport.gradients;
return isObject( colorSupport ) && !! colorSupport.gradient;
};

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/font-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
import { cleanEmptyObject } from './utils';
import { createHigherOrderComponent } from '@wordpress/compose';

export const FONT_SIZE_SUPPORT_KEY = '__experimentalFontSize';
export const FONT_SIZE_SUPPORT_KEY = '__experimentalStyles.typography.fontSize';

/**
* Filters registered block settings, extending attributes to include
Expand Down
3 changes: 2 additions & 1 deletion packages/block-editor/src/hooks/line-height.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import { useSelect } from '@wordpress/data';
import LineHeightControl from '../components/line-height-control';
import { cleanEmptyObject } from './utils';

export const LINE_HEIGHT_SUPPORT_KEY = '__experimentalLineHeight';
export const LINE_HEIGHT_SUPPORT_KEY =
'__experimentalStyles.typography.lineHeight';

/**
* Inspector control panel containing the line height related configuration
Expand Down
8 changes: 8 additions & 0 deletions packages/block-library/src/columns/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@
"__experimentalColor": {
"gradients": true,
"linkColor": true
},
"__experimentalStyles": {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this config, we'd want to:

  • Define whether something is enabled or not
  • Potentially define whether it uses presets
  • Potentially enable/disable custom values
  • Potentially choose whether to show selectors in inspector/toolbar
  • Potentially other configs I'm not thinking of right now.

Do you think this new config scales properly to all these kind of configs?
I do wonder whether this new config is more tailored toward applying styles and not configuring available controls for a block. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I'm thinking about separate keys per each thing. This is matching the styles subtree of the theme.json, the same way __experimentalFeatures in block.json matches the features subtree of the theme.json (which, atm, is only implemented by the paragraph block, though). If we wanted to add presets or UI configurations, we can do as separate keys within supports as well.

A couple of questions that I have are:

  1. Is this enough granularity for blocks that define many contexts (ex: heading block => core/heading/h1, core/heading/h2)? Both the current mechanism and this reorganization make all headings share the same features they support.

  2. What's the relation between block.json and the lib/experimental-theme.json that comes with core? Is block.json for enabling/disabling things and the experimental-theme.json for setting the values? Does it make sense to conflate the two? If we conflate them, the first question takes more importance as, for values, we do need different things per context.

All in all, given that this is a mere reorganization and that it doesn't introduce any block deprecations, etc, I think it's fine to go with this. It brings some needed clarity and alignment between block.json and theme.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if theme.json and block.json work in coordination, I'm not sure we should map one over the other tbh.

If I'm a block author's, I want to enable colors and configure how the controls show, I don't really care about applying styles aside maybe a default value, and it's weird tbh if these are in separate keys.

If I'm a theme author, I want to define a color palette for the editor, potentially define separate palettes for specific blocks and I want to enable/disable custom colors or palettes. I also want to give default values for colors. In this case, separate keys may be good.

It seems like two different ways of thinking about these configs.

I do wonder if it's too early to be aligning configs and thinking about what keys to use... Maybe we should focus now to adding more features and configs to all blocks and themes and once we have all of these under experimental flags or keys, we can decide how to align with a clear view.

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 I may have introduced some confusion. What we currently have is:

  1. If core blocks want to

    1. Declare support for a style property => use the block.json
    2. Set a default value for a style property => use core's lib/experimental-theme.json

  2. Theme authors want to set palettes, styles, etc => use the theme's theme.json

  3. Plugin authors want to

    1. Declare support for a style property => use the block.json
    2. Set a default value for a style property => we don't have an API for this.

I was referring to conflating 1.1 and 1.2. I think I may agree with you that probably is a good idea to keep them separated. However, we'd then need to find a solution for 3.2. I don't have an answer for this yet, just sharing early thoughts to get the conversation started.

As for the tree organization, I'm not too opinionated, as long as we use the same shape everywhere (block attributes, theme.json spec, and block supports). At the moment, block supports is the only place that uses a different shape. I guess we can fix what we have now for clarity and iterate later when we gain a better knowledge about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, block supports is the only place that uses a different shape. I guess we can fix what we have now for clarity and iterate later when we gain a better knowledge about this?

I was personally arguing that given the fact that block authors and theme authors think differently about these features, it make sense to have a different shape.

A block author think about the color support for his block wholistically and not separately (enabling, configuring presets, and potentially a default value)

which means: (block.json)

color: {
   enabled: true,
   gradients: true,
   ui: ['toolbar']
   palette: {} // Can define a palette that overrides the default editor palette
   default: "somevalue"  // may or may not be needed,
   allowCustomColor: true,
   allowCustomGradients:  true, 
}

but for a theme author (theme.json), he thinks differently, he thinks first about applying config globally and then go specific per block if needed. We also don't want to control the UI for blocks here. so for me:

presets: {
    global: {
       color: {}
    },
    core/some-block: {
       color: {}
    },
},

features: {
    global: {
       allowCustomColors: true,
    },
    core/some-block: {
      allowCustomColors: false,
    },
},

styles: {
    global: {
       color: 'black',
    },
    core/some-block: {
      color: 'red',
    },
}

also from a theme's author perspective, presets and features might be the same thing (just controlling the editor) so I wonder if they should be separate but I guess that's a separate discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, let's put this PR in the backburner.

After some PRs I'm working on are merged (example: #24416) I was thinking of starting to port some of the theme_supports features to this new system. That may be a good opportunity to re-evaluate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Riad, in case you missed it, more food for thought that's related to what we discussed here: #22887

"color": {
"background": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

In the client, the background and text keys are not used, only support for __experimentalStyles.color is checked. Although we could remove them from here and make the server verification do the same, this will make the support keys to have different shape than the block attributes and theme.json. I lean towards making the client be more strict about these checks in a subsequent PR and actually verify that background and text are true.

Thoughts?

Copy link
Member Author

@oandregal oandregal Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

See #24298 (comment) as an example where conflating background and text is forcing us to do things that we may not want to do.

"gradient": true,
"link": true,
"text": true
}
}
}
}
8 changes: 8 additions & 0 deletions packages/block-library/src/group/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@
"__experimentalColor": {
"gradients": true,
"linkColor": true
},
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"link": true,
"text": true
}
}
}
}
11 changes: 11 additions & 0 deletions packages/block-library/src/heading/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@
"core/heading/h5": "h5",
"core/heading/h6": "h6"
},
"__experimentalStyles": {
"color": {
"background": true,
"link": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
},
"__unstablePasteTextInline": true
}
}
8 changes: 8 additions & 0 deletions packages/block-library/src/media-text/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,14 @@
"__experimentalColor": {
"gradients": true,
"linkColor": true
},
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"link": true,
"text": true
}
}
}
}
11 changes: 11 additions & 0 deletions packages/block-library/src/paragraph/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,17 @@
}
},
"__experimentalSelector": "p",
"__experimentalStyles": {
"color": {
"background": true,
"link": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
},
"__unstablePasteTextInline": true
}
}
14 changes: 13 additions & 1 deletion packages/block-library/src/post-author/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@
"gradients": true,
"linkColor": true
},
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"link": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
13 changes: 12 additions & 1 deletion packages/block-library/src/post-comments-count/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@
"gradients": true
},
"__experimentalFontSize": true,
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
14 changes: 13 additions & 1 deletion packages/block-library/src/post-comments-form/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@
"linkColor": true
},
"__experimentalFontSize": true,
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"link": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
14 changes: 13 additions & 1 deletion packages/block-library/src/post-comments/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@
"gradients": true,
"linkColor": true
},
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"link": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
13 changes: 12 additions & 1 deletion packages/block-library/src/post-date/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@
"gradients": true
},
"__experimentalFontSize": true,
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
14 changes: 13 additions & 1 deletion packages/block-library/src/post-excerpt/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@
"gradients": true,
"linkColor": true
},
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"link": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
11 changes: 11 additions & 0 deletions packages/block-library/src/post-title/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@
"core/post-title/h5": "h5",
"core/post-title/h6": "h6",
"core/post-title/p": "p"
},
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
13 changes: 12 additions & 1 deletion packages/block-library/src/site-tagline/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@
"gradients": true
},
"__experimentalFontSize": true,
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
13 changes: 12 additions & 1 deletion packages/block-library/src/site-title/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@
"gradients": true
},
"__experimentalFontSize": true,
"__experimentalLineHeight": true
"__experimentalLineHeight": true,
"__experimentalStyles": {
"color": {
"background": true,
"gradient": true,
"text": true
},
"typography": {
"fontSize": true,
"lineHeight": true
}
}
}
}
Loading