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

Implemented mechanism to use classes for configured colors. #5273

Merged
merged 2 commits into from Apr 25, 2018

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Feb 26, 2018

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.

@jorgefilipecosta jorgefilipecosta self-assigned this Feb 26, 2018
@jorgefilipecosta 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 jorgefilipecosta force-pushed the update/colors-as-classes branch from c19d512 to 4965d17 Feb 26, 2018
@gziolo gziolo added this to In progress in Extensibility Feb 28, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from 4965d17 to f95623c Mar 21, 2018
@jorgefilipecosta 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 jorgefilipecosta requested a review from WordPress/gutenberg-core Mar 21, 2018
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Mar 23, 2018

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

What's the backwards-compatibility story?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Mar 23, 2018

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.

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

"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.

width,
} = attributes;

const fontSize = this.getFontSize();

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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.

This comment has been minimized.

Copy link
@aduth

aduth Mar 28, 2018

Member

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

@@ -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 ) {

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 23, 2018

Author Member

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.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Mar 24, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

undefined is redundant. return; returns undefined.

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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' ], [] );
};
} )( WrappedComponent );

const NewComponent = ( props ) => {

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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).

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

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 17, 2018

Author Member

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

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 23, 2018

Author Member

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.

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 23, 2018

Member

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

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Mar 23, 2018

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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 24, 2018

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 jorgefilipecosta force-pushed the update/colors-as-classes branch from f95623c to 4493aee Mar 26, 2018
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Mar 26, 2018

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.

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 28, 2018

Member

Are these styles meant to be limited to p elements?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2018

Author Member

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.

} );

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

This comment has been minimized.

Copy link
@aduth

aduth Mar 28, 2018

Member

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 29, 2018

Author Member

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.

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch 3 times, most recently from 2480364 to 9b99d2e Mar 29, 2018
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Apr 18, 2018

While testing this, I noticed that the classNames are not automatically loaded in the theme, is this the intended behavior? Seems like a regression for me, especially since colors were applied properly to the output in the previous release?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Apr 18, 2018

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

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from 8c40fc7 to 8c42103 Apr 18, 2018
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Apr 18, 2018

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.

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from 8c42103 to 66a8a26 Apr 18, 2018
@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Apr 23, 2018
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Apr 23, 2018

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

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from 66a8a26 to dbb862d Apr 23, 2018
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Apr 23, 2018

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!

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from dbb862d to dabd1d7 Apr 23, 2018
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Apr 24, 2018

<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.

Copy link
Contributor

youknowriad left a comment

LGTM 👍

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Apr 24, 2018

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

Copy link
Member

gziolo left a comment

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.

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

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 24, 2018

Member

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

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

?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 24, 2018

Author Member

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.

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 26, 2018

Member

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.

}
```

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

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 24, 2018

Member

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

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 24, 2018

Author Member

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

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Apr 24, 2018

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

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Apr 24, 2018

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

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Apr 24, 2018

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

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from dabd1d7 to 3647989 Apr 24, 2018
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Apr 24, 2018

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

@jorgefilipecosta jorgefilipecosta force-pushed the update/colors-as-classes branch from 3647989 to 1e8ea5a Apr 25, 2018
@jorgefilipecosta jorgefilipecosta merged commit 0489c56 into master Apr 25, 2018
2 checks passed
2 checks passed
codecov/project 44.1% (+0.06%) compared to eabf682
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Extensibility automation moved this from In progress to Done Apr 25, 2018
@jorgefilipecosta jorgefilipecosta deleted the update/colors-as-classes branch Apr 25, 2018
nuzzio added a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018
…s#5273)

Implemented mechanism to use classes for configured colors instead of inline styles.
@rchipka

This comment has been minimized.

Copy link

rchipka commented May 15, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.