Implemented mechanism to use classes for configured colors. #5273

jorgefilipecosta · 2018-02-26T23:13:42Z

This PR changes the colors data structure to specify a name for the color besides just the color value.
That name can be used to solve a11y issues namely: #2699. And also to generate classes e.g: has-vivid-red-background-color.
Themes can make use of the same mechanism when specifying custom colors. The use of classes allows them a better customization of blocks, e.g: themes can set hover background color of a button in function of the chosen normal background color, this was not possible until now.

No design/ behavior changes are expected, things work exactly as before just the markup is different.

This PR implements a newly added, withColors HoC. This HoC abstracts the logic to get and set colors/classes. So other components will be able to reuse the logic.

How Has This Been Tested?

Add a paragraph verify that, when we use a color (text and/ background), things work as expected in the editor and front end, verify that for the predefined colors a class is used and for custom colors, an inline style is used.

aduth · 2018-03-23T21:16:42Z

Themes can make use of the same mechanism when specifying custom colors.

What's the backwards-compatibility story?

aduth · 2018-03-23T21:37:58Z

What's the backwards-compatibility story?

Even worse, all paragraph blocks which had previously applied a color no longer have said color after these changes.

aduth · 2018-03-23T21:17:23Z

editor/components/provider/index.js

-		'#eee',
-		'#abb8c3',
-		'#313131',
+		{	name: 'pale pink',


Consistency: Property on own line. Same with next object. Remainder of objects are correct.

aduth · 2018-03-23T21:20:30Z

blocks/color-mechanism/index.js

@@ -0,0 +1,90 @@
+/**


"Mechanism" suffix seems an inconsequential addition. Maybe just colors. Though it's still confusing to me what to expect this file to contain. I tend to like one export per file, not a mixed set of higher-order components and utility functions.

aduth · 2018-03-23T21:21:08Z

blocks/library/paragraph/index.js

 			width,
 		} = attributes;

 		const fontSize = this.getFontSize();

+		const textColor = colors( 'text', 'textColor', 'customTextColor' );


I happened to land at this line of code without any other context, and am baffled by what to expect this line of code to be doing at a glance. Probably a sentiment we can expect to be shared by future casual observers.

I happened to land at this line of code without any other context, and am baffled by what to expect this line of code to be doing at a glance. Probably a sentiment we can expect to be shared by future casual observers.

Still confused here, even with the change in function name.

'text', 'textColor', 'customTextColor' doesn't have any significance to me, and I certainly wouldn't trust myself to apply the correct ordering in my own implementation for whatever they do mean.

Related: https://www.refactoring.com/catalog/introduceNamedParameter.html

aduth · 2018-03-23T21:22:59Z

blocks/library/paragraph/editor.scss

@@ -1,4 +1,4 @@
-.editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph {
+.editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph:not( .has-background ) {


Also mentioned at #5658 (comment), but why do we assign a white background at all? What about when themes override default editor styles and provide their own background colors?

The reason why we set a white background is related to the contrast verification. If the background was never set getComputedStyles returns back background (if I remember correctly). So adding a background fixes the problem. If themes change the background it also works well because as long as a background is set we are able to retrieve it. The problem happens when no background is set. As we get back background color when background is not set we cannot differentiate and assume is white background because in fact it may be set to black. I will try to research this in more detail maybe there is a way to detect when the background is not set and in that case assume white.

Can we set the white background on the fly, when the contrast checker is working?

aduth · 2018-03-23T21:24:09Z

blocks/color-mechanism/index.js

+ */
+export function getColorClass( colorContextName, colorName ) {
+	if ( ! colorContextName || ! colorName ) {
+		return undefined;


undefined is redundant. return; returns undefined.

aduth · 2018-03-23T21:28:54Z

blocks/color-mechanism/index.js

+export function withColors( WrappedComponent ) {
+	const ComponentWithColorContext = withContext( 'editor' )(
+		( settings, props ) => {
+			const colors = get( settings, 'colors', [] );


Minor: When Lodash sees a string as its second argument, it triggers its "parse into path array" logic that I've made a point to discourage, so even though we're only going one level deep, still worthwhile to wrap in an array:

const colors = get( settings, [ 'colors' ], [] );

aduth · 2018-03-23T21:32:52Z

blocks/color-mechanism/index.js

+			};
+		} )( WrappedComponent );
+
+	const NewComponent = ( props ) => {


I think we already have a handful of names for what we assign as an higher-order-enhanced component, and NewComponent isn't one of them. I'd be inclined not to add to this problem.

My preference is EnhancedComponent, though I've also seen WrappedComponent (which is confusing because it's unclear if "Wrapped" is the wrapping of a component, or the component being wrapped).

aduth · 2018-03-23T21:35:13Z

blocks/color-palette/index.js

@@ -23,7 +23,7 @@ export function ColorPalette( { colors, disableCustomColors = false, value, onCh

 	return (
 		<div className="blocks-color-palette">
-			{ map( colors, ( color ) => {
+			{ map( colors, ( { color } ) => {


In my console:

Warning: Each child in an array or iterator should have a unique "key" prop.

Edit: Actually, this is because I had #5570 (review) in place. Still an issue, though not certain it's introduced here. No warning when many color options exist.

I was not able to replicate, I think it was not introduced here as the color value used in the key should be unique.

aduth · 2018-03-23T21:42:28Z

blocks/library/paragraph/index.js

@@ -302,9 +307,15 @@ const schema = {
 	textColor: {
 		type: 'string',
 	},
+	customTextColor: {


This proliferation of attributes is excessive. Particularly to my previous comment of existing blocks, can't we have a single attribute, the hex of the color? Shouldn't matter if it's custom or not. If it aligns with an existing hex for a named color, we can identify it by that. I thought this is what we were doing with font size, though I've not kept up with what we decided for #5269.

Hi @aduth, before this PR made use of a single attribute and a logic to differentiate between predefined (classes) and custom values, in the fontSizes PR #5269 it was decided to use two different attributes for custom and predefined values, so this PR was updated to match the implementation used for fonts.

aduth · 2018-03-23T21:42:56Z

editor/components/provider/index.js

+			color: '#00d084',
+		},
+		{
+			name: 'pale cyan-blue',


Is dash here intentional? Why is it "green cyan", yet "cyan-blue" ?

jorgefilipecosta · 2018-03-23T22:08:14Z

Hi @aduth,

Even worse, all paragraph blocks which had previously applied a color no longer have said color after these changes.

In the fontSizes PR where we faced the same problem #5269 we discussed a little bit about backward compatibility and how the blocks are getting bigger because of it.

Here backward compatibility is even more complex because we can not make use of type like in the approach I used on the font. My plan is to add a migrate function, and keep it there for 2 versions, that migrates color... attributes starting with '#' to customColor... attribute. It is not perfect because the user may have changed an attribute to "red" in the code and we will not be able to migrate that one because we can not differentiate it from the new attribute, but in the normal cases where user did not change code to use named CSS colors it will work fine. The migration logic will follow the same logic of the existing one for fonts depending on when are this changes released the attributes can even be migrated together updating the existing migration functions for font.

Themes can make use of the same mechanism when specifying custom colors.

What's the backward-compatibility story?

Regarding backward compatibility for colors specified in themes, I think the simplest approach is to detect themes setting colors in the old mechanism add a warning that the old mechanism has been deprecated and ask developers to migrate.
Now besides the name in the colors themes have to provide classes, and themes that made use of the old approach probably don't have the classes created.
The other approach to the theme migration of color palette is to allow colors in the palette to not have a name (for some versions), and in this cases set color using customColor... attribute like we do in color picker.

Deprecation logic (mainly the theme one if we go for the second approach) will make the code more complex and harder to review so my plan is first to validate the logic than I add an isolated commit with a deprecation logic that we can revert after 2 versions passed following the normal deprecation cycle.

jasmussen · 2018-03-24T07:58:34Z

What's the backwards-compatibility story?

Even worse, all paragraph blocks which had previously applied a color no longer have said color after these changes.

I feel like @mtias might have a stronger opinion than I, and I would just note before I say anything, this is just my personal opinion. I defer to you all, and the leads, for any calls here.

With that said, I think we should move as fast as we can towards the desired end-state, even if in a situation like this, back compat is dropped. As soon as we enter the merge proposal phase, this gets harder to do, and if we have to do it we should do it before the "Try Gutenberg" callout that's about to land in WordPress 4.9.5. Given that it's only posts that are authored in Gutenberg that use paragraph colors, I suspect it's not the end of the world if those disappear.

In other words, since we have so much to work on to wrap Gutenberg, we should be okay with the lack of back compat to previous versions of Gutenberg in this particular situation.

jorgefilipecosta · 2018-03-26T14:42:20Z

Hi @aduth, thank you for your feedback, I tried to address it and this PR was updated. The most notable change is in the structure of @blocks/colors.

Thank you for sharing your thoughts on deprecation @jasmussen.

aduth · 2018-03-28T23:35:54Z

blocks/colors/style.scss

@@ -0,0 +1,87 @@
+p.has-pale-pink-background-color {


Are these styles meant to be limited to p elements?

Sorry I updated the code but missed the answer. During the first version, I expected the styles to be per theme and the class to applied always to the most external dom element. So themes would need to specify how to apply the color to different blocks. But the overhead would be huge. It is better if we just set the rule according to its "context" (background, color) and then blocks are responsible to apply the class to any dom element they have.

aduth · 2018-03-28T23:37:34Z

blocks/library/paragraph/index.js

 		} );

 		const styles = {
-			backgroundColor: backgroundColor,
-			color: textColor,
+			backgroundColor: ! backgroundColor ? customBackgroundColor : undefined,


I'm a bit confused here by this logic. When will backgroundColor be assigned as a style?

Should this be...

backgroundColor: customBackgroundColor || backgroundColor,

Same problem on the following line.

The logic backgroundColor: ! backgroundColor ? customBackgroundColor : undefined, was correct it means if we don't have a named color use the custom color. But it was not intuitive I changed to backgroundColor: backgroundClass ? undefined : customBackgroundColor which means don't set any style if we have a class set the background color to customBackgroundColor if we don't have a class.

youknowriad · 2018-04-18T08:43:26Z

I think the right behavior might be to include these styles if the theme doesn't define a custom palette.

jorgefilipecosta · 2018-04-18T09:08:08Z

Hi @youknowriad,

I think the right behavior might be to include these styles if the theme doesn't define a custom palette.

I totally agree with you and we were loading the styles, during my last rebase I made a typo and they were not appearing :( I corrected this thank you for your tests 👍

The comments were added.

youknowriad · 2018-04-23T15:41:06Z

Not sure If I'm doing something wrong, but I have a JS error as soon as I load Gutenberg with this PR

jorgefilipecosta · 2018-04-23T18:06:43Z

Hi @youknowriad, I think the errors were being thrown because a function we used meanwhile was deprecated, not sure why the error happened in your setup because the PR was not rebased since the deprecation/removal ( it had no conflict and in the previous branch state it was not yet removed), but I don't see other explanation. I rebased and removed the usage of the deprecated function. The errors should not happen now. Thank you for your tests!

gziolo · 2018-04-24T08:40:41Z

<p style="background-color:vivid cyan blue;color:very light gray" class="has-background">
	<p class="has-background has-very-light-gray-color has-vivid-cyan-blue-background-color">@shakespeare&nbsp; sdasa @shakespeare</p>
</p>

This is what I got when I opened my post updated with this branch in master.

youknowriad

LGTM 👍

youknowriad · 2018-04-24T08:48:43Z

Oh I missed @gziolo 's comment, but the upgrade worked for me. Might be good to investigate though

gziolo

What about all old post that were created using colors that were expressed as hex values? Should they be migrated to class names when opened with this changes applied? In my testing, they remain unchanged but I can see some logs on the JS console suggesting that they were rendered using deprecation strategy.

gziolo · 2018-04-24T08:49:15Z

docs/extensibility/theme-support.md

-		'#eee',
-		'#444'
+		array(
+			'name' => 'strong magenta',


Why not use the name as key and the color as value?

array( 'strong magenta' => '#a156b4', )

?

Because we want themes to be able to set the order in which colors appear in the palette, and when this gets converted to a JS object we would not have the guarantee that the order would be kept as the object is an unordered collection of properties.

Can we map it to an ordered array before it gets exposed to JavaScript?

PHP doesn't care as much about the order when strings are involved.

gziolo · 2018-04-24T08:51:07Z

docs/extensibility/theme-support.md

+}
+```
+
+The class name is built appending 'has-', followed by the class name in kebab case and ending with the context name.


the class name in kebab case - shouldn't it be the class name *using* kebab case? I'm not quite sure, someone could confirm.

I agree that the class name *using* kebab casesounds better I will update 👍

gziolo · 2018-04-24T08:55:36Z

Oh I missed @gziolo 's comment, but the upgrade worked for me. Might be good to investigate though

Not sure if this is an actual issue, just noting that it is strange that it behaves like this when I switched branches. It probably isn't the real world use case where someone downgrades versions :)

jorgefilipecosta · 2018-04-24T08:55:40Z

Hi @gziolo,

<p style="background-color:vivid cyan blue;color:very light gray" class="has-background">
	<p class="has-background has-very-light-gray-color has-vivid-cyan-blue-background-color">@shakespeare&nbsp; sdasa @shakespeare</p>
</p>

This is what I got when I opened my post updated with this branch in master.

I think this is the expected behavior, the master is not aware of this changes in the save logic here, so if we open a post created with this branch in master it gets broken as master interprets textColor and backgroundColor that now are names that represent classes as the color values.
If we open a post created in master in this branch it will work as we are back-compatible, but the contrary does not work as master does not understand the new save mechanism (the future). When we merge this into master the problem does not happen anymore as master save will be this one.

jorgefilipecosta · 2018-04-24T09:47:51Z

Regarding the existing posts, the colors they have, are migrated to use customColors... version so everything should work as before.

gziolo · 2018-04-24T10:25:32Z

Thanks for making it all clear. Feel free to proceed as suggested by @youknowriad.

… inline styles.

…s#5273) Implemented mechanism to use classes for configured colors instead of inline styles.

rchipka · 2018-05-15T16:25:59Z

I think ideally the generated class names should have some Gutenberg-specific prefix and the class name should include some indication that the class shouldn't be targeted directly (as it could change).

I address some of these concerns in this issue.

jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress [Feature] Extensibility The ability to extend blocks or the editing experience labels Feb 26, 2018

jorgefilipecosta self-assigned this Feb 26, 2018

jorgefilipecosta changed the title ~~In progress - Implemented mechanism to use classes for configured colors. instead of…~~ In progress - Implemented mechanism to use classes for configured colors. Feb 26, 2018

jorgefilipecosta force-pushed the update/colors-as-classes branch from c19d512 to 4965d17 Compare February 26, 2018 23:17

mtias mentioned this pull request Mar 2, 2018

Blocks and Theme Styling Overview #5360

Closed

jorgefilipecosta mentioned this pull request Mar 8, 2018

Predefined set of font sizes #5269

Merged

jorgefilipecosta force-pushed the update/colors-as-classes branch from 4965d17 to f95623c Compare March 21, 2018 00:06

jorgefilipecosta changed the title ~~In progress - Implemented mechanism to use classes for configured colors.~~ Implemented mechanism to use classes for configured colors. Mar 21, 2018

jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Mar 21, 2018

jorgefilipecosta requested a review from a team March 21, 2018 12:10

aduth reviewed Mar 23, 2018

View reviewed changes

jorgefilipecosta mentioned this pull request Mar 23, 2018

Try improving the side UI for nested blocks #5658

Merged

jorgefilipecosta force-pushed the update/colors-as-classes branch from f95623c to 4493aee Compare March 26, 2018 14:23

This was referenced Mar 26, 2018

Hide color pickers in paragraph and button if no colors are available. #5570

Merged

Implement nesting in cover image block #5452

Closed

aduth reviewed Mar 28, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/colors-as-classes branch 3 times, most recently from 2480364 to 9b99d2e Compare March 29, 2018 12:13

afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 3, 2018

youknowriad modified the milestones: 2.7, 2.8 Apr 18, 2018

jorgefilipecosta force-pushed the update/colors-as-classes branch from 8c40fc7 to 8c42103 Compare April 18, 2018 09:02

jorgefilipecosta force-pushed the update/colors-as-classes branch from 8c42103 to 66a8a26 Compare April 18, 2018 09:56

jorgefilipecosta requested a review from a team April 23, 2018 15:09

jorgefilipecosta force-pushed the update/colors-as-classes branch from 66a8a26 to dbb862d Compare April 23, 2018 17:56

jorgefilipecosta force-pushed the update/colors-as-classes branch from dbb862d to dabd1d7 Compare April 23, 2018 19:21

youknowriad approved these changes Apr 24, 2018

View reviewed changes

gziolo reviewed Apr 24, 2018

View reviewed changes

jorgefilipecosta force-pushed the update/colors-as-classes branch from dabd1d7 to 3647989 Compare April 24, 2018 09:50

jorgefilipecosta added 2 commits April 25, 2018 09:27

Implemented mechanism to use classes for configured colors instead of…

1595a3f

… inline styles.

Post rebase changes.

1e8ea5a

jorgefilipecosta force-pushed the update/colors-as-classes branch from 3647989 to 1e8ea5a Compare April 25, 2018 08:45

jorgefilipecosta merged commit 0489c56 into master Apr 25, 2018

jorgefilipecosta deleted the update/colors-as-classes branch April 25, 2018 08:54

nuzzio pushed a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Implemented mechanism to use classes for configured colors. (WordPres…

bc3a2be

…s#5273) Implemented mechanism to use classes for configured colors instead of inline styles.

webmandesign mentioned this pull request Apr 26, 2018

Update/theme support color palette #6437

Closed

aduth mentioned this pull request Apr 15, 2019

Documentation: Clarify best practices surrounding color palette names #14989

Closed

		@@ -1,4 +1,4 @@
		.editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph {
		.editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph:not( .has-background ) {

Implemented mechanism to use classes for configured colors. #5273

Implemented mechanism to use classes for configured colors. #5273

Conversation

jorgefilipecosta commented Feb 26, 2018 • edited Loading

How Has This Been Tested?

aduth commented Mar 23, 2018

aduth commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 23, 2018 • edited Loading

jasmussen commented Mar 24, 2018

jorgefilipecosta commented Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 18, 2018 • edited Loading

jorgefilipecosta commented Apr 18, 2018 • edited Loading

youknowriad commented Apr 23, 2018

jorgefilipecosta commented Apr 23, 2018 • edited Loading

gziolo commented Apr 24, 2018

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 24, 2018

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Apr 24, 2018

jorgefilipecosta commented Apr 24, 2018 • edited Loading

jorgefilipecosta commented Apr 24, 2018

gziolo commented Apr 24, 2018

rchipka commented May 15, 2018

jorgefilipecosta commented Feb 26, 2018 •

edited

Loading

jorgefilipecosta Mar 23, 2018 •

edited

Loading

jorgefilipecosta Apr 17, 2018 •

edited

Loading

jorgefilipecosta commented Mar 23, 2018 •

edited

Loading

jorgefilipecosta commented Mar 26, 2018 •

edited

Loading

youknowriad commented Apr 18, 2018 •

edited

Loading

jorgefilipecosta commented Apr 18, 2018 •

edited

Loading

jorgefilipecosta commented Apr 23, 2018 •

edited

Loading

jorgefilipecosta commented Apr 24, 2018 •

edited

Loading