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.

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: Use theme and core styles as defaults on the global styles #26786

Merged

Conversation

jorgefilipecosta
Copy link
Member

Currently, the themes can set styles using theme.json but on the global styles editing UI even if the theme sets some styles, things appear empty, making it hard for the user the make small adjustments.

This PR makes sure the editing UI of global styles reflects the styles set by the theme and core.

How has this been tested?

I added the following config to theme.json of 2021 blocks theme:

	"core/post-date": {
		"styles": {
			"typography": {
				"fontSize": "40px",
				"lineHeight": "3"
			},
			"color": {
				"background": "#f00",
				"text": "#0f0"
			}
		}
	}

I opened the site editor, and on the global styles sidebar, Post Date panel I verified the UI reflected the styles configured by the theme.

@github-actions
Copy link

github-actions bot commented Nov 6, 2020

Size Change: +531 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/annotations/index.js 3.77 kB -3 B (0%)
build/autop/index.js 2.84 kB +2 B (0%)
build/block-editor/index.js 133 kB +12 B (0%)
build/block-library/index.js 147 kB -14 B (0%)
build/block-serialization-default-parser/index.js 1.87 kB +1 B
build/blocks/index.js 48 kB +2 B (0%)
build/components/index.js 171 kB +11 B (0%)
build/components/style.css 15.3 kB +1 B
build/compose/index.js 9.9 kB +4 B (0%)
build/core-data/index.js 14.8 kB -1 B
build/edit-navigation/index.js 11.1 kB -3 B (0%)
build/edit-post/index.js 306 kB +2 B (0%)
build/edit-site/index.js 23.4 kB +533 B (2%)
build/edit-site/style-rtl.css 3.95 kB +2 B (0%)
build/edit-site/style.css 3.95 kB +1 B
build/edit-widgets/index.js 26.3 kB -3 B (0%)
build/editor/index.js 42.5 kB -5 B (0%)
build/element/index.js 4.62 kB -2 B (0%)
build/format-library/index.js 6.87 kB +4 B (0%)
build/hooks/index.js 2.16 kB -2 B (0%)
build/keyboard-shortcuts/index.js 2.52 kB -5 B (0%)
build/keycodes/index.js 1.94 kB -1 B
build/list-reusable-blocks/index.js 3.1 kB +1 B
build/notices/index.js 1.77 kB -1 B
build/nux/index.js 3.4 kB -2 B (0%)
build/plugins/index.js 2.56 kB -1 B
build/rich-text/index.js 13.3 kB -4 B (0%)
build/server-side-render/index.js 2.77 kB -2 B (0%)
build/url/index.js 4.06 kB -1 B
build/viewport/index.js 1.84 kB +5 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.71 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.91 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/data-controls/index.js 821 B 0 B
build/data/index.js 8.74 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.52 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.43 kB 0 B
build/edit-post/style.css 6.42 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 713 B 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.83 kB 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo gziolo added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Nov 8, 2020
@oandregal
Copy link
Member

👋 I've done some testing of this PR and this is what I see.

How it works in the master branch

The UI controls only reflect user choices but not base styles (theme+core), hence the controls show the default value when the user hasn't chosen any:

Captura de ecrã de 2020-11-10 13-05-08

How it works in this branch (using preset values)

Updated the theme.json of TwentyTwentyOne blocks theme to look like this:

	"core/post-date": {
		"styles": {
			"typography": {
				"fontSize": "var(--wp--preset--font-size--extra-small)",
				"lineHeight": "var(--wp--custom--line-height--body)"
			},
			"color": {
				"background": "var(--wp--preset--color--black)",
				"text": "var(--wp--preset--color--green)"
			}
		}
	},

When I add the block to the editor I see it correctly styled, but the UI controls behave weirdly:

  • font size value is set to custom
  • line height value is unaffected (takes the default 1.5)
  • color controls
    • have a preview color (square with color alongside the control name), but it's white, not the proper color
    • the color value is not marked as selected

Captura de ecrã de 2020-11-10 12-56-43

How it works in this branch (using raw values)

Updated the theme.json of TwentyTwentyOne blocks theme to use the corresponding raw values for the presets above:

	"core/post-date": {
		"styles": {
			"typography": {
				"fontSize": "16px",
				"lineHeight": "1.7"
			},
			"color": {
				"background": "#000000",
				"text": "#D1E4DD"
			}
		}
	},

The editor is correctly styled and the UI controls do reflect properly the values:

Captura de ecrã de 2020-11-10 12-59-50

@oandregal
Copy link
Member

Based on my testing above, my first instinct is to think that we want it to work when using presets, otherwise it produces confusing results. It's highly likely that theme authors use presets to set the values of the style properties most of the time ― TwentyTwentyOne does and so the other themes I've checked.

@jorgefilipecosta jorgefilipecosta force-pushed the update/use-theme-and-core-styles-as-defaults-on-global-styles branch from 7fc85d3 to 651526d Compare November 11, 2020 19:58
@jorgefilipecosta
Copy link
Member Author

Hi @nosolosw,

Thank you for the review. We now handle the case where variable references are used.

@oandregal
Copy link
Member

I think we could do with some tightening up of the API, so it doesn't see an explosion for each degree of freedom we want to give the consumer. I think we should optimize for the main use case:

  • getStyleProperty( context, property ) => returns the user or fallbacks to base styles (merged style property)
  • setStyleProperty( context, property ) => sets the user property

The question is how do we allow to retrieve the specific user values if there's one. There are a few options:

  1. Consumers specify whether they want the default value or all (merged and user): getStyleProperty( context, property, all = false ).
// Main use case: get merged value.
const color = getStyleProperty( 'core/paragraph', 'color' ); 

// Use case: get both values.
const { user, merged } = getStyleProperty( 'core/paragraph', 'color', true );
  1. Consumers specify the origin they want to get data from: getStyleProperty( contex, property, origin = 'merged' ). origin can be 'merged' (default behaviour) or 'user'.
// Main use case: get merged value.
const color = getStyleProperty( 'core/paragraph', 'color' );

// Use case: get both values.
const color = getStyleProperty( 'core/paragraph', 'color' );
const userColor = getStyleProperty( 'core/paragraph', 'color', 'user' );
  1. Add a new method getStylePropertyFromUserOrigin.
// Main use case: get merged value.
const color = getStyleProperty( 'core/paragraph', 'color' );

// Use case: get both values.
const color = getStyleProperty( 'core/paragraph', 'color' );
const userColor = getStylePropertyFromUserOrigin( 'core/paragraph', 'color' );

I listed them in order of personal preference. I'd rather not do 3, though. What do you think?

@jorgefilipecosta jorgefilipecosta force-pushed the update/use-theme-and-core-styles-as-defaults-on-global-styles branch from 651526d to db78beb Compare November 12, 2020 13:21
@jorgefilipecosta
Copy link
Member Author

Hi @nosolosw thank you for the suggestions. I updated the code to use option 2.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

I submitted a fix to prevent some bad data input, other than that this works so can 🚢

As a potential iteration on this code, I'd like getValueFromVariable and getPresetVariable hidden from the UI panels. They shouldn't have to know these internal things, rather this should be getStyleProperty / setStyleProperty responsibility to do.

@oandregal oandregal merged commit 881df45 into master Nov 12, 2020
@oandregal oandregal deleted the update/use-theme-and-core-styles-as-defaults-on-global-styles branch November 12, 2020 16:23
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 12, 2020
@oandregal oandregal mentioned this pull request Nov 26, 2020
82 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants