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

Add color reference mechanism #21490

Closed
wants to merge 2 commits into from

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Apr 8, 2020

This PR changes the way named colors are implemented. Instead of using classes for named colors this PR's proposes to use the same mechanism we use for custom colors but instead of having the value inline we reference a variable representing the color value.

The variables are generated using a mechanism implemented with global styles.
In order to have the CSS variables always available, this PR exposes global styles functionality that was behind flags (new settings and printing the variables so side effects are expected). Related: wp-cli/i18n-command#163

This PR also proposes that colors are passed using theme.json. Although currently, we can not use that for colors because soon files can not be translatable so sweet the data structure on the PHP side.

This PR removes the need for themes to create color classes.

How has this been tested?

I verified using predefined colors on the paragraphs and headings works as expected.
I verified paragraphs and headings previously created using predefined colors, are migrated to the new reference mechanism.
I verified it was possible to change colors by using theme.json (although not recommended right now) by changing file experimental-default-global-styles.json to:

{
	"color": {
		"primary": "#52accc",
		"background": "white",
		"text": "black"
	},
	"colors": [
		{ "name": "Pale pink", "slug": "pale-pink", "color": "#f78da7" },
		{ "name": "Vivid red", "slug": "vivid-red", "color": "#cf2e2e" },
		{
			"name": "Luminous vivid orange",
			"slug": "luminous-vivid-orange",
			"color": "#ff6900"
		},
		{
			"name": "Luminous vivid amber",
			"slug": "luminous-vivid-amber",
			"color": "#fcb900"
		}
	]
}

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Block] Heading Affects the Headings Block [Block] Paragraph Affects the Paragraph Block Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Apr 8, 2020
@github-actions
Copy link

github-actions bot commented Apr 8, 2020

Size Change: +215 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-editor/index.js 106 kB +215 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.48 kB 0 B
build/block-directory/style-rtl.css 788 B 0 B
build/block-directory/style.css 788 B 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 7.2 kB 0 B
build/block-library/editor.css 7.2 kB 0 B
build/block-library/index.js 119 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.62 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 14 kB 0 B
build/edit-site/style-rtl.css 5.52 kB 0 B
build/edit-site/style.css 5.53 kB 0 B
build/edit-widgets/index.js 8.05 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

type: 'string',
},
} );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It is very cool that we avoid the need for all these custom attributes.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I'd like to try to understand the pros/cons of this approach in order to know how/if to move forward with it, for me:

  • I like that custom colors and palette colors work in the exact same way, same attribute...
  • I like that theme authors don't have to write any CSS for their palettes to work properly.
  • Potentially, opens the door for InnerBlocks specific color palettes. (In this section of the page, the palette is different)

What could be considered as downsides:

  • No smart behaviors (auto-apply text color when choosing a background color), while I understand why some theme authors do it, I feel it's not the right thing to do anyway because the behavior becomes inconsistent with custom colors and can be confusing.
  • Inline style to apply the variable instead of a className: Again for me I don't see it as a downside, because choosing a variable, or choosing a className is similar in terms of reusability, we only need to change the value in a single place...

Backwards compatibility:

  • For BC, it looks like themes need to keep their current classNames CSS for existing content.

Potential way-forward:

Is it possible to make this new behavior the default for new palettes (palettes provided using theme.json), document it as the canonical way to define palettes and keep the previous behavior for some time for themes that still use the old way of providing a palette (potentially throw a console log or something about it being deprecated)?

@jorgefilipecosta
Copy link
Member Author

Is it possible to make this new behavior the default for new palettes (palettes provided using theme.json), document it as the canonical way to define palettes and keep the previous behavior for some time for themes that still use the old way of providing a palette (potentially throw a console log or something about it being deprecated)?

Hi @youknowriad, Although this PR allows specifying colors using theme.json, right now in practice is not possible in to do it -- colors have names that need to be translatable and JSON files are not translatable. So I guess new palettes will still not be specified in theme.json but using a PHP call.

I guess we could achieve something similar if we use the new mechanism when a theme.json file is specified but we don't use if theme.json file does not exist.
The downside would be that we have two mechanisms two maintain (at least for some time).

@youknowriad
Copy link
Contributor

So I guess new palettes will still not be specified in theme.json but using a PHP call.

I think we can work with the CLI team to fix that.

@youknowriad
Copy link
Contributor

The downside would be that we have two mechanisms two maintain (at least for some time).

If we don't do it, we'd potentially break some themes. Maybe it's fine but we'd have to measure the impact properly. What number of themes do have smart logics?

@jorgefilipecosta jorgefilipecosta force-pushed the add/color-reference-mechanism branch from 4c49453 to 4b9c3c8 Compare April 9, 2020 20:37
@jorgefilipecosta jorgefilipecosta force-pushed the add/color-reference-mechanism branch from 4b9c3c8 to 4b851fb Compare April 9, 2020 21:58
@jorgefilipecosta
Copy link
Member Author

Hi @youknowriad, some updates were performed in this PR:

  • It keeps the existing color classes mechanism when theme.json is not used, so we keep the functionality for current themes exactly as it was.
  • Instead of storing the var in the attribute, I implemented a syntax in global styles that allows referencing variables.

$settings['__experimentalGlobalStylesUserEntityId'] = gutenberg_experimental_global_styles_get_user_cpt_id();
if ( gutenberg_experimental_global_styles_is_site_editor() ) {
$settings['__experimentalGlobalStylesUserEntityId'] = gutenberg_experimental_global_styles_get_user_cpt_id();
}

$global_styles = array_merge(
gutenberg_experimental_global_styles_get_core(),
gutenberg_experimental_global_styles_get_theme()
);

$settings['__experimentalGlobalStylesBase'] = $global_styles;
Copy link
Member

Choose a reason for hiding this comment

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

mmm, is __experimentalGlobalStylesBase needed by the block editor?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems it is not needed and it seems we have other settings not used at all. I will open a separate PR to remove unused settings.

Copy link
Member

Choose a reason for hiding this comment

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

mmm, how so? __experimentalGlobalStylesUserEntityId is used in the site editor to retrieve the CPT through the entities API and __experimentalGlobalStylesBase is used in the site editor as well to know which is the base styles the user will overwrite.

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'm missing something for example when searching for __experimentalGlobalStylesUserEntityId in the edit-site package nothing is found, how does edit-site accesses the setting?

Copy link
Member

Choose a reason for hiding this comment

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

It's used by the GlobalStylesProvider but it the branch that introduces that hasn't landed yet.

dirname( dirname( __FILE__ ) ) . '/experimental-default-global-styles.json'
);
// Todo: Remove this when colors are properly set on themes.json
// For now colors can not be set there because json files are not translatable.
if ( empty( $core_styles['colors'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

We have these at the JavaScript level as well (packages/block-editor/src/store/defaults.js). Can we perhaps not to have them here and let the block-editor handle the defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to have the defaults here, in order to generate CSS vars on the frontend for each color. I think we should remove the defaults from the JS side to avoid the duplication, but as for now we only run this code if global styles are enabled it seems safer to leave the defaults there.

@oandregal
Copy link
Member

oandregal commented May 5, 2020

I really like the direction here. I'm inclined to think some of the things this PR does we should merge right away while others should wait. Let me explain my thinking:

1) Overrides the editor color settings from a theme.json declaration if it is present [WAIT]

This is not ready due to lacking i18n support in JSON files. We're also storing color defaults in PHP in addition to having them stored in JavaScript. I think it'd be best to wait until the JSON situation is resolved.

2) Switches preset colors from classes to inlined CSS vars & opens up Global Styles mechanism to block editor. [WAIT]

  • This requires enqueuing the embedded stylesheet generated from global styles to the block editor. I'd rather wait for this until we solve some of the global styles issues. My limited experience in migrating to the style attribute & having witnessed the aftermath of the button markup changes urges me to think that we should move at two velocities: be more conservative with changes at the block-level (changes here affect hosts that use the plugin in production) and more liberal with changes at the global-level (we hide things behind an experimental flag, so we have more leeway to course-correct if necessary).

  • If I got this comment right Riad has suggested looking at not using CSS variables, and I think that's a fair point. Although I believe using CSS vars for style presets is still a good fit, perhaps we don't need to after all. It'd be safer if we wait to implement these block-level changes until we have a more concrete answer of how the global-level works.

3) Exposes color presets and introduces a namespace for colors. [MERGE]

I think this is great and I also had a local experiment that I dusted off yesterday for all style presets (colors, gradients, font-sizes) that I published at #22076 It doesn't matter to me if we refactor this PR to address only this point or we merge 22076, whatever you think is best would work for me.

Related to how to namespace the palettes/style presets, I was thinking we could use a namespace to accommodate "all the things themes": colors, gradients, and font-sizes style presets; but potentially any other theme support (wp-block-styles, align-wide, etc) and even the ability to disable the palettes (disable-custom-colors, disable-custom-gradients, disable-custom-font-sizes). In #22076 I propose using --wp--theme but I'm not attached to it and will be happy to consider any other choice that could work for all the potential things if we needed to.

Final note

This turned out to be a long comment! Thanks for your patience.

Do you think this is a fair assessment of the current situation and what this PR does? How do you feel about all this? Should we move forward with 3 and let 1 and 2 wait for a bit?

@youknowriad
Copy link
Contributor

I think without introducing the block level support, this PR loses a bit of its importance cause right now, no one is using theme.json yet. That said, I agree that it should be done with care: nudge people to use the "new" palettes instead and softly deprecate the old ones without any breakage.

I think it'd be best to wait until the JSON situation is resolved.

I think the JSON situation won't be resolved until we merge the PR :) (because we need an available API to implement it)

I don't mind if we only merge the CSS variables generation first though.

@oandregal
Copy link
Member

oandregal commented May 5, 2020

I think the JSON situation won't be resolved until we merge the PR :)

You're right! Poor phrasing on my part. I meant that this part of the PR should land after the CLI PR.

@jorgefilipecosta
Copy link
Member Author

  1. Overrides the editor color settings from a theme.json declaration if it is present [WAIT]

This is not ready due to lacking i18n support in JSON files.

It seems the work to have i18n in JSON files is ongoing. I think we should not wait for that work. We can continue the global styles work, without the strings being translated for now. When that work is done, the strings will be translatable. It seems they are independent things, and we should not feel blocked.

  1. Switches preset colors from classes to inlined CSS vars & opens up Global Styles mechanism to block editor. [WAIT]

This requires enqueuing the embedded stylesheet generated from global styles to the block editor. I'd rather wait for this until we solve some of the global styles issues.

What are the issues we need to address that you think specifically block this task?

My limited experience in migrating to the style attribute & having witnessed the aftermath of the button markup changes urges me to think that we should move at two velocities: be more conservative with changes at the block-level (changes here affect hosts that use the plugin in production) and more liberal with changes at the global-level (we hide things behind an experimental flag, so we have more leeway to course-correct if necessary).

This PR does not apply any markup/style change unless a theme.json file is present, so it seems safe.

From my point of view, if we still want to use CSS variables instead of classes for predefined colors, it seems like this PR could be merged without any blocker after rebased to account for the changes in #22076.
If we don't want CSS vars for predefined colors and change direction and continue with the classes, I guess we should close the PR.

@swissspidy
Copy link
Member

@jorgefilipecosta can you point me to the definition of what a theme.json file can contain? Which parts need to be internationalized? Just colors?

@oandregal
Copy link
Member

@swissspidy for internationalization, it's probably relevant to take into account all the presets we already allow defining for the block editor: color palette, font sizes, and gradients. We'd probably want all of them to be defined in theme.json and not requiring themes to register them via PHP.

We're still iterating on the theme.json shape, this is the direction we're aiming for.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented May 11, 2020

Ideally, we would have a way to easily configure the theme.json paths that are translated. I guess a possible simple version would be to translate all values under a name key as for now all the values we need to translate are under a name key.
But ideally we would have something configurable where we set the paths that are translated in a very easy way e.g:


themeJSONtranslatePaths = [
	[ 'config', 'color', 'palette', /.*/, 'name' ]
]

Would make all name properties objects part of the array config.color.palette translatable.

@swissspidy
Copy link
Member

No idea where you'd want to put that config, but adding it to the theme.json file sounds like a bad idea - people would forget to add it, etc.
Anywho, I'd recommend discussing this outside this PR 🙂 Something like the block registration RFC would be great.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented May 11, 2020

No idea where you'd want to put that config, but adding it to the theme.json file sounds like a bad idea - people would forget to add it, etc.
Anywho, I'd recommend discussing this outside this PR 🙂 Something like the block registration RFC would be great.

Sorry, I think I did not explain correctly, that config would be something internal and static, Gutenberg and WordPress core developers can change it when adding new features to theme.json. But the structure would not be part of themes.json.

@schlessera
Copy link
Member

Ideally, such a config would be provided where WP-CLI can read it. So instead of hard-coding translatable string locations in WP-CLI, it would automatically keep in sync with Core/Gutenberg when trying to extract these strings. This makes it easier to adapt the definitions, as it would never be blocked by missing tooling support from then on.

@jorgefilipecosta jorgefilipecosta force-pushed the add/color-reference-mechanism branch from 4b851fb to fddbf02 Compare May 27, 2020 11:11
@jorgefilipecosta
Copy link
Member Author

Ideally, such a config would be provided where WP-CLI can read it. So instead of hard-coding translatable string locations in WP-CLI, it would automatically keep in sync with Core/Gutenberg when trying to extract these strings. This makes it easier to adapt the definitions, as it would never be blocked by missing tooling support from then on.

Hi @schlessera, thank you for sharing your thoughts. I totally agree with what you said. A possible place where the configuration could live is on the Gutenberg repository, so it is close with the code that reads the JSON files.
WP CLI could access it by using a GitHub link e.g: https://raw.githubusercontent.com/WordPress/gutenberg/master/package.json

For the configuration shape, I guess the same configuration should be used for block.json and theme.json. I guess the configuration could like this

{
'block.json': [ 'name' ],
'theme.json': [ 'config', 'color', 'palette', /.*/, 'name' ],
}

cc: @gziolo, @nosolosw

@jorgefilipecosta
Copy link
Member Author

According to the feedback from @nosolosw I'm going to close this PR for now. Theme.json shape is not yet totally stable and passing parts of that data structure to color components etc would make changing things harder in the future. When the shape is more stable this PR should be reopened.

@youknowriad
Copy link
Contributor

youknowriad commented May 27, 2020

it seems to me that the unstable state of theme.json is already clear by the fact that the file is named experimental-theme.json

this suggests that we may be able to implement this in the blocks and just make it stable by renaming the file when we think it's ready for prime time.

@oandregal
Copy link
Member

To clarify, I think there are two different things here that are conflated:

The feedback I gave above, in which I point to CSS variables as something that needs more work. We are only using them for presets at the moment. Although we'll investigate to expand them to the block implicit attributes is not clear that we'll end up using them, so, in my view, it seems premature to use them at the block level.

The other bit is about the shape of the theme.json file: Jorge and I were talking about using this file to connect with the existing editor subsystem (presets, etc), which right now use the presets defined via add_theme_support. This is something we have to do at some point. My suggestion is to do it later when some other things are clarified/resolved: how to express nested content, consolidate things that are in block editor's store (defaults) + the core's theme.json, etc. Right now, changing the shape of theme.json is easy because it's only used by code in global-styles.php (an experimental thing). If we start using it to connect the inner working of the editor, shape changes will become more costly, for no real benefit in my view. At the moment, IMHO, the value lies on expanding the theme.json features (more blocks, more properties) and in iterating in the things that are unknown (nested content).

@youknowriad
Copy link
Contributor

The feedback I gave above, in which I point to CSS variables as something that needs more work. We are only using them for presets at the moment

I'm not suggesting to use CSS variables for something else, I'm suggesting to use inline styles that reference the theme presets at the block level. color: var( --wp-them-preset-red ).

@oandregal
Copy link
Member

oandregal commented Jun 2, 2020

@youknowriad sorry it took me so long to respond. I had this comment ready but, apparently, I didn't hit the send button.


I'd like to explore an alternative: what if we inlined the actual values for every user modification (presets -custom and not- as well as style variations)? Have we explored that in the past?

My thinking goes like this. What should happen with user modifications when they change themes?

  1. User modifications should persist when another theme is activated. If so, using classes and inlining CSS custom properties that are theme-specific is problematic. As soon as the theme changes, those classes and properties are no longer enqueued or have a different meaning for the new theme. If we consider user modifications permanent, it seems reasonable to:

    1. Use inline styles for all of them (custom values, presets, style variations).
    2. Limit the styles to a known subset and let themes configure them. Example: only make 5 kinds of font presets available (smaller, small, normal, large, larger) and themes can only tweak the values, they can't modify the list.
  2. User modifications should not persist when another theme is activated. In that case, I'd say we need a different system from what we have now, one that can be reset when the theme changes. To me, this presumes that we don't attach styles to the block directly (classes or inline) but have them external to the block. This requires being able to tell apart a specific block from other blocks in the front (exposing block ids).

From the options above, the more robust (predictable results) seems to be 1.1. What are your thoughts on this?

cc @jorgefilipecosta

@youknowriad
Copy link
Contributor

I'd like to explore an alternative: what if we inlined the actual values for every user modification (presets -custom and not- as well as style variations)? Have we explored that in the past?

This is how the original implemention of the palette worked. We had a lot of push back for it (inline styles) and we wanted to be able to support changing the colors across the entire site using global styles (or similar adhoc implementations in customizer...)

@jorgefilipecosta
Copy link
Member Author

As referred in #22722 (comment). I think we should not use direct value. Becase as @youknowriad referred we want to allow users to globally change colors across a website even for existing blocks in all posts/areas direct value does not offer that.

Regarding the disadvantage, "when the user changes themes, the block style is broken because the new theme does not have the same colors". If predefined colors are kept and the theme is changed, the old colors may not look good at all on the new theme, and the design may appear broken as well. There may be cases where the design may look even more broken if predefined colors are kept than if the defaults (no color selected) of the new theme are applied.
I imagine given that theme.json/"global styles" allow user customization core or plugins may even offer an option to keep the presets of the old theme in user customizations when changing themes if desired.

Between classes and CSS variables I don't have a strong preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block [Block] Paragraph Affects the Paragraph Block Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants