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

jorgefilipecosta · 2018-03-12T15:53:35Z

If the theme sets the color palette to empty and disables custom colors, the color picker mechanism did not work correctly. It just showed a non-functional back color to choose from. Now we hide the color picker mechanism as in that case it is not possible to choose colors.

Closes: #5447

How Has This Been Tested?

Disable custom colors, and set the color pallete to empty. That can be done by adding the following lines to the functions.php of the theme:

add_theme_support( 'editor-color-palette' );
add_theme_support( 'disable-custom-colors' );

Verify in paragraph and button that the color panels are now hidden.
Enable some colors in the palette, and/or enable custom colors. Verify that if one or two the features are enabled the color panels are shown.

aduth · 2018-03-16T20:36:47Z

blocks/conditional-panel-color/index.js

+import { ifCondition, PanelColor, withContext } from '@wordpress/components';
+import { compose } from '@wordpress/element';
+
+export default compose(


Was trying to think of a way we could avoid duplicating the logic here between ColorPalette and ConditionalPanelColor. Unfortunate there's no easy way to avoid rendering a wrapper if none of its children render. I wonder if we could do a render prop which self-contains the null rendering:

<EditorColors> { ( { colors, disableCustomColors } ) => ( <PanelColor title={ __( 'Text Color' ) } colorValue={ textColor }> <ColorPalette value={ textColor } onChange={ ( colorValue ) => setAttributes( { textColor: colorValue } ) } colors={ colors } disableCustomColors={ disableCustomColors } /> </PanelColor> ) } </EditorColors>

const EditorColors = compose( withContext( 'editor' )( ( settings, props ) => ( { colors: props.colors || settings.colors, disableCustomColors: props.disableCustomColors !== undefined ? props.disableCustomColors : settings.disableCustomColors, } ) ), ifCondition( ( { colors, disableCustomColors } ) => ! ( isEmpty( colors ) && disableCustomColors ) ) )( ( { children, ...props } ) => children( props ) );

This way, we don't need ColorPalette to retrieve color settings from context, because they'd always be passed in.

Thoughts?

In either case, if we're creating a new component, it should be added to blocks/index.js to export on the global.

Hi @aduth thank you for providing an alternative implementation, I really like the EditorColors component idea. I applied it to this PR.

While I see the value of avoiding duplication, I think from a plugin author's perspective, duplication is fine or at least we could extract the logic into a EditorColors component but I think it should be baked in into these two components instead of using it at the block level. The block author could just use the ColorPalette or ColorPanel directly in that case.

Hi @youknowriad, I understand your points. PanelColor is a generic component in @wordpress/components and we should not add the context information or conditional render directly to it. Maybe we should keep the context in the ColorPalette but change it to use a component that retrieves colors from the context.
Then we create a new component HasColorsColorsToChoose or something similar that conditional render the children if colors are available or custom colors are enabled. And we use it to wrap color logic without changing how any of the existing components work, they would keep their existing functionality and would work alone.
The other alternative is similar to what I had at the start a ConditionalPanelColor component that just conditionally renders PanelColor, but now we make use of a component that retrieves the colors from the context so no code duplication exists when comparing to the ColoPalette.

The other alternative is similar to what I had at the start a ConditionalPanelColor component that just renders PanelColor , but now we make use of a component that retrieves the colors from the state so no code duplication exists when comparing to the ColoPalette.

I was thinking more in that vein. The same we we have wp.blocks.ColorPalette we could have we.blocks.ColorPanel with context awareness. We can push the logic further by extracting the generic Color Palette to the components module so we'd also have wp.components.ColorPalette and wp.components.ColorPanel without any context awareness. Thoughs?

I think it makes sense but maybe in the @blocks version, we should remove support for passing colors as props when that is a requirement the @components version can be used.
As if the behavior of the PanelColor in blocks is to not render if no colors are available if the plugin creator wants custom colors using the @blocks he would need to pass the colors also PanelColor which would only use them to check the conditional logic and does not need to know the colors for anything else.

makes sense to me 👍 just thinking how we'd ensure backwards compatibility in that case. Maybe support it for a release or two with a deprecated message?

Exactly, for the PanelColor @blocks no deprecation is necessary as it is something new the existing one is at @components and will stay untouched. For the ColorPalette we would support but with a deprecation message when the prop is used saying to change to the @components version.

gziolo · 2018-03-23T12:53:50Z

@jorgefilipecosta nice work. Can we go further and include ContrastCheckerWithFallbackStyles also in the <EditColors /> wrapper as it will never render when there are other color components disabled?

This code looks so similar that it might make sense even go further and simplify it to:

<EditorColors
	setAttributes={ setAttributes }
	textColor={ textColor }
	backgroundColor={ backgroundColor }
	node={ this.nodeRef }
	isLargeText={ true }
	initialOpen={ false }
/>

One of the block uses color instead of backgroundColor so maybe it should be updated in that case to have this similar. It might be too much of the abstraction, but it might also make it easier to apply colors to other blocks :)
Edit: Probably it should have a different name than EditorColors if this proposal makes sense :)

aduth

Not relevant to the changes here, but: What should be expected if I add this to my theme?

add_theme_support( 'disable-custom-colors' );
add_theme_support( 'editor-color-palette', [] );

No idea where it's added, but this shows the color picker, with a single option for black. Doesn't seem correct to me (and even if it's working-as-intended, certainly not intuitive).

aduth · 2018-03-23T20:12:22Z

blocks/editor-colors/index.js

+		} )
+	),
+	ifCondition(
+		( { colors, disableCustomColors } ) => ! ( isEmpty( colors ) && disableCustomColors )


I spent far too much time staring at this to make sense of it. Seems correct, not sure how we could make it clearer, but doing De Morgan's in my head a few times and thinking in inverses with disabling made this quite difficult to discern.

jorgefilipecosta · 2018-03-26T18:34:30Z

Hi @gziolo I think it totally makes sense for the contrast checker to be inside the "conditional render", I missed it and I moved it inside.
Regarding an abstraction that includes the contrast verification, I'm thinking about it and maybe we should go for a component that abstracts the text color picker, the background color picker, and the contrast logic, to make block creation easier. For example in button, color logic occupies a significant percentage of the LOC's in the block. We will have to do a refactor to contrast logic (to remove a dependency on a CSS background rule) and during that refactor and after we land the class mechanism I think it would be a good idea to check what we can abstract. And create a component that does bigger abstraction.

I thought a little bit more and for now, I renamed EditorColors to ColorContext to make more evident that this is just a context retrieval component and not a UI mechanism. I also remove the conditional render from it and added a new prop hasColorsToChoose that blocks can make use to conditional render the children or for example show a warning or use predefined colors if colors are mandatory for the block to make sense.
I imagine that if we create a color abstraction component that includes text, background and contrast verification it will make use of this ColorContext Component to retrieve the color palette.

Hi @aduth,

Not relevant to the changes here, but: What should be expected if I add this to my theme?

add_theme_support( 'disable-custom-colors' );
add_theme_support( 'editor-color-palette', [] );
No idea where it's added, but this shows the color picker, with a single option for black. Doesn't seem correct to me (and even if it's working-as-intended, certainly not intuitive).

That happens, because we define the colors as arguments instead of as an array:
e.g:
add_theme_support( 'editor-color-palette',
'#a156b4',
'#d0a5db',
'#eee',
'#444'
);.
So when we use add_theme_support( 'editor-color-palette', [] ); The editor thinks we have one color the color "[]" and even sets the color attribute to "[]". The same happens if we set a color to any other invalid value e.g: add_theme_support( 'editor-color-palette', 1 );.
If we want to handle this problem I think the best way is to perform a validation step that checks if colors are valid and if not displays a warning in the console.
The PR #5273 changes how themes should set the colors so as part of its backward compatibility we could add a validation to the colors that the theme sets and show a warning if they are not valid. The warning would handle the case add_theme_support( 'editor-color-palette', [] ); as in this case we are setting an invalid color.

Thank you both for reviews!

gziolo · 2018-03-27T11:34:19Z

Regarding an abstraction that includes the contrast verification, I'm thinking about it and maybe we should go for a component that abstracts the text color picker, the background color picker, and the contrast logic, to make block creation easier. For example in button, color logic occupies a significant percentage of the LOC's in the block. We will have to do a refactor to contrast logic (to remove a dependency on a CSS background rule) and during that refactor and after we land the class mechanism I think it would be a good idea to check what we can abstract. And create a component that does bigger abstraction.

Sounds great, but definitely not for this PR :)

gziolo · 2018-03-27T11:45:05Z

blocks/library/button/index.js

-							isLargeText={ true }
-						/> }
+						<ColorContext>
+							{ ( { colors, disableCustomColors, hasColorsToChoose } ) => ( hasColorsToChoose ? [


It might make sense to refactor this code to work with Fragment component instead of an array:

{ ( { colors, disableCustomColors, hasColorsToChoose } ) => ( hasColorsToChoose && ( <Fragment> <PanelColor title={ __( 'Background Color' ) } colorValue={ color }> .... </Fragment> ) }

It removes the need to have , and key :)

I always question my self about what to use arrays or fragment component. For this cases, I will try to default to Fragment from now on. The code was updated.

gziolo · 2018-03-27T12:03:52Z

It looks like this refactoring doesn't play well with CSS styles for the color notice:

I tested with both Paragraph and Button blocks.

gziolo

ContrastCheckerWithFallbackStyles needs to be fixed.

jorgefilipecosta · 2018-03-27T13:53:54Z

Hi @gziolo, I also found out about the problem with the contrast checker while testing this PR. Turns out it was not related to the changes here and also happened on the master (it was caused during a refactor to Notices). My PR that fixes it, was already merged #5800. I rebased this PR so this problem should not happen now.

youknowriad · 2018-03-27T14:44:25Z

blocks/color-palette/index.js

@@ -77,12 +77,3 @@ export function ColorPalette( { colors, disableCustomColors = false, value, onCh
 		</div>
 	);
 }
-
-export default withContext( 'editor' )(


This is a breaking change in how this component works, you need to provide the colors as a prop now. Should we provide a BC alternative?

ColorPalette become a general purpose component after this refactoring. What we could do to provide backward compatibility is move ColorPallete to components module and still expose it in here with the context applied for the 2 upcoming release cycles. We would use wp.components.ColorPallete for the core blocks. What do you think?

youknowriad · 2018-03-27T14:46:43Z

blocks/library/button/index.js

-							backgroundColor={ color }
-							isLargeText={ true }
-						/> }
+						<ColorContext>


Why did you choose to implement this as a component with children as function. I'm a little bit concerned about this because it's not so obvious for block authors? Do you think it's better to bake these checks inside PanelColor and ColorPalette instead?

Hi @youknowriad, this pattern was used to avoid code duplication #5570 (comment).

@youknowriad, I would rather further consolidate those color panels as proposed in #5570 (comment).

Hmm, this would work to:

<PanelColor title={ __( 'Background Color' ) } attributeName="color" value={ color } />

😃
I guess it would have to be:

<PanelColor title={ __( 'Background Color' ) } onChange={ ( colorValue ) => setAttributes( { color: colorValue } ) } value={ color } />

jorgefilipecosta · 2018-04-10T09:19:46Z

This PR was updated yesterday I followed @gziolo and @youknowriad suggestions and we still have no code duplication. The components/color-palette/index.js is exactly equal to the one we had in blocks with just the context removed.

gziolo

I had some more comments. It also needs to be rebased.

gziolo · 2018-04-16T08:13:16Z

blocks/panel-color/index.js

+/**
+ * WordPress dependencies
+ */
+import { PanelColor } from '@wordpress/components';


It can be alsoimported as:

import { PanelColor as PanelColorComponent } from '@wordpress/components';

Then we can name this component the same way as the file: PanelColor.

gziolo · 2018-04-16T08:15:09Z

blocks/panel-color/index.js

+	);
+}
+
+export default withColorContext( ConditionalPanelColor );


To avoid if ( ! hasColorsToChoose ) { check it can be expressed as:

export default compose( withColorContext, ifCondition( ( { hasColorsToChoose } ) => hasColorsToChoose ), )( PanelColor );

Aside: ifCondition could support string to pass a prop name :)

gziolo · 2018-04-16T08:19:44Z

components/color-palette/index.js

+		<div className="blocks-color-palette">
+			{ map( colors, ( color ) => {
+				const style = { color: color };
+				const className = classnames( 'blocks-color-palette__item', { 'is-active': value === color } );


Class names are no longer valid. They should be prefixed with components- not blocks- because they were moved to a different module.

gziolo · 2018-04-16T08:23:42Z

blocks/color-palette/index.js

 /**
 * WordPress dependencies
 */
-import { Dropdown, withContext } from '@wordpress/components';
-import { __, sprintf } from '@wordpress/i18n';
+import { ColorPalette } from '@wordpress/components';


Do we need to keep this file since we always use PanelColor in all core blocks?

Yes, in order to be back compatible with blocks created by the community and that use @blocks/ColorPalette. Since the file is very small we could export it from @blocks directly, but we would be importing other components and use a different logic in the file. Using a file allows us to use the same export logic in the @blocks file, and after two versions we can deprecate this and delete this file.

jorgefilipecosta · 2018-04-16T12:14:47Z

Hi @gziolo, thank you for the catches, the feedback was addressed.

If the theme sets the colors palette to empty and disable custom colors, the color picker mechanism did not work correctly. It just showed a non-functional back color to choose from. Now we hide the color picker mechanism as in that case it is not possible to choose colors.

jorgefilipecosta · 2018-05-02T10:05:21Z

This PR was rebased and all feedback was addressed.

gziolo

Let's ship it 🚢
It had enough iterations, works as advertised and looks solid 👍

jorgefilipecosta · 2018-05-02T12:14:35Z

Thank you for the review @gziolo!

jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label Mar 12, 2018

jorgefilipecosta self-assigned this Mar 12, 2018

jorgefilipecosta requested a review from a team March 16, 2018 19:33

aduth reviewed Mar 16, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch from 13fc90d to 8d3fe23 Compare March 22, 2018 23:09

aduth reviewed Mar 23, 2018

View reviewed changes

aduth mentioned this pull request Mar 23, 2018

Implemented mechanism to use classes for configured colors. #5273

Merged

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch from 8d3fe23 to f7fef36 Compare March 26, 2018 18:00

gziolo reviewed Mar 27, 2018

View reviewed changes

gziolo requested changes Mar 27, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch from f7fef36 to fca1496 Compare March 27, 2018 13:50

youknowriad reviewed Mar 27, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch from fca1496 to 8626263 Compare April 10, 2018 00:10

gziolo reviewed Apr 16, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch 2 times, most recently from 494f181 to 3d10db5 Compare April 16, 2018 11:52

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch from 3d10db5 to d4adfbc Compare April 30, 2018 18:35

jorgefilipecosta force-pushed the fix/hide-color-panels-if-colors-are-available branch from d4adfbc to 340789b Compare May 2, 2018 09:55

jorgefilipecosta added this to the 2.8 milestone May 2, 2018

gziolo approved these changes May 2, 2018

View reviewed changes

jorgefilipecosta merged commit 608af70 into master May 2, 2018

jorgefilipecosta deleted the fix/hide-color-panels-if-colors-are-available branch May 2, 2018 12:14

samikeijonen mentioned this pull request Jan 19, 2019

Hide the Font Size picker on Paragraph blocks when there is only one size defined #11628

Closed

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

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

Conversation

jorgefilipecosta commented Mar 12, 2018 • edited Loading

How Has This Been Tested?

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 27, 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

gziolo commented Mar 23, 2018 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 26, 2018

gziolo commented Mar 27, 2018

gziolo Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Mar 27, 2018 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 27, 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

gziolo Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta commented Apr 10, 2018 • edited Loading

gziolo left a comment • 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

jorgefilipecosta Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta commented Apr 16, 2018

jorgefilipecosta commented May 2, 2018

gziolo left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented May 2, 2018

jorgefilipecosta commented Mar 12, 2018 •

edited

Loading

jorgefilipecosta Mar 27, 2018 •

edited

Loading

gziolo commented Mar 23, 2018 •

edited

Loading

gziolo Mar 27, 2018 •

edited

Loading

gziolo commented Mar 27, 2018 •

edited

Loading

jorgefilipecosta commented Mar 27, 2018 •

edited

Loading

gziolo Mar 28, 2018 •

edited

Loading

jorgefilipecosta commented Apr 10, 2018 •

edited

Loading

gziolo left a comment •

edited

Loading

jorgefilipecosta Apr 16, 2018 •

edited

Loading