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

Color Palette support setup with single array argument (backwards compatible) #7619

Merged
merged 5 commits into from Jul 3, 2018

Conversation

Projects
None yet
6 participants
@webmandesign
Contributor

webmandesign commented Jun 28, 2018

Description

Allowing editor-color-palette theme support setup with a single array of colors parameter/argument. This way only one argument is passed to add_theme_support( 'editor-color-palette', $arg ) instead of multiple arguments for each color.

This code is backwards compatible with current implementation (colors are passed as multiple parameters/arguments).

Fixing issue #6425

(cc @chrisvanpatten and @jorgefilipecosta as per previous conversation)

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jul 3, 2018

Hi @webmandesign thank you for addressing backward compatibility 👍
@Luehrsen and @felixarntz, given your last useful insights on the color mechanisms would it be possible to share your thoughts on this PR?

// Backcompat for Color Palette set as multiple parameters.
if ( is_string( $color_palette ) || isset( $color_palette['color'] ) ) {
$color_palette = get_theme_support( 'editor-color-palette' );
wp_add_inline_script(

This comment has been minimized.

@Luehrsen

Luehrsen Jul 3, 2018

Contributor

Is inlining a console.warn the way how we notice about depreciations? I think using _doing_it_wrong() is a safer approach.

This is probably from line 1171, but I would love to hear what @gziolo thinks about that.

This comment has been minimized.

@webmandesign

webmandesign Jul 3, 2018

Contributor

The backcompat was actually suggested by @jorgefilipecosta so I've left it unchanged.

Should I remove the wp_add_inline_script part and introduce the _doing_it_wrong() for both instances of console.warn?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jul 3, 2018

Member

At the time of my suggestion _doing_it_wrong was not yet available.
I think we can now use the following:
_doing_it_wrong(
'add_theme_support()',
__( 'Setting colors using multiple parameters is deprecated. Please pass a single parameter with an array of colors. See https://wordpress.org/gutenberg/handbook/extensibility/theme-support/ for details.', 'gutenberg' ),
'3.2.0'
);

This comment has been minimized.

@webmandesign

webmandesign Jul 3, 2018

Contributor

@Luehrsen and @jorgefilipecosta I've went ahead and applied the _doing_it_wrong on both console.warn instances.

@@ -8,7 +8,7 @@ To opt-in for one of these features, call `add_theme_support` in the `functions.
```php
function mytheme_setup_theme_supported_features() {
add_theme_support( 'editor-color-palette',
add_theme_support( 'editor-color-palette', array(

This comment has been minimized.

@Luehrsen

Luehrsen Jul 3, 2018

Contributor

Do I guess the usage right, that you want to create the array somewhat programatically and just pass a variable to add_theme_support?

All in all I think it is a good method of doing things.

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Jul 3, 2018

Contributor

@Luehrsen I won't speak for @webmandesign but that's what we'd like to do. Also makes it a little easier to re-use the color values in other contexts, e.g. auto-generating inline CSS for the classes, etc.

(something like…

$colors = [ /* imagine I'm an array */ ];

add_theme_support( 'editor-color-palette', $colors );

foreach ( $colors as $color ) {
 $css .= ".{$color['name']} { color: {$color['color']} }\n"
}

// echo out $css somewhere

This comment has been minimized.

@Luehrsen

Luehrsen Jul 3, 2018

Contributor

Although I like the approach, I would probably have done that with get_theme_support(). I think that would add consistency throughout the themes and coding standards, as I would just have to search for 'editor-color-palette' to see all code that handles the theme colors.

foreach( get_theme_support( 'editor-color-palette' ) as $color ) {
     $css .= ".{$color['name'} { color: {$color['color']} }\n"
}

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Jul 3, 2018

Contributor

@Luehrsen That works as well! My code was just an example — but having a consistent input and output format makes sense I think, either way.

This comment has been minimized.

@webmandesign

webmandesign Jul 3, 2018

Contributor

@Luehrsen Yes, that's correct, I want to create the array programatically and then just pass it into add_theme_support. I think that's the best approach (right now I can't think of any other add_theme_support feature being passed with multiple arguments nowadays, actually).

@webmandesign

This comment has been minimized.

Contributor

webmandesign commented Jul 3, 2018

@jorgefilipecosta Backwards compatibility props goes to you :)

@Luehrsen

I think this change makes sense. Passing the colors as a single array makes handling the theme support easier.

@jorgefilipecosta

jorgefilipecosta approved these changes Jul 3, 2018 edited

This PR seems to work fine in my tests 👍 . It looks like there is some consensus that this approach is better than what we had. Thank you @webmandesign for the effort in bringing this changes. I added a new commit updating the deprecation version.

@jorgefilipecosta jorgefilipecosta merged commit 2b3f2b2 into WordPress:master Jul 3, 2018

2 checks passed

codecov/project 46.76% remains the same compared to e69dca4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@felixarntz

I'm in favor of this change, as it does make things easier for theme developers.

It does make it a bit complex for Gutenberg itself (i.e. this PR), I really liked from the original approach that you could simply call get_theme_support( 'editor-color-palette' ) and have the final result to use, without needing to do some $theme_support = $theme_support[0]. But we wanna make things easy for theme developers, not ourselves, so +1.

_doing_it_wrong(
'add_theme_support()',
__( 'Adding theme support using the `gutenberg` array is deprecated. See https://wordpress.org/gutenberg/handbook/extensibility/theme-support/ for details.', 'gutenberg' ),
'3.4.0'

This comment has been minimized.

@felixarntz

felixarntz Jul 3, 2018

Member

+1 for changing this to using _doing_it_wrong() instead of printing a JS warning. After all it's something done wrong in PHP. :)

// Backcompat for Color Palette set as multiple parameters.
if ( is_string( $color_palette ) || isset( $color_palette['color'] ) ) {
$color_palette = get_theme_support( 'editor-color-palette' );

This comment has been minimized.

@felixarntz

felixarntz Jul 3, 2018

Member

This is a solid implementation of BC, just a minor detail I think we should improve: I don't like calling get_theme_support() twice. I think we should make the original call to set $color_palette be only the (array) get_theme_support( 'editor-color-palette' ) part.

Then the if clause could check ! empty( $color_palette ) && isset( $color_palette[0]['color'] ), and if so, trigger the notice. Then an else clause could get the first array element to account for the new way.

One thing I'm not sure about, why is there an is_string() check. I might miss something, was it possible to provide strings only at some point?

This comment has been minimized.

@webmandesign

webmandesign Jul 3, 2018

Contributor

@felixarntz
From my tests, we can not use (array) get_theme_support( 'editor-color-palette' ) width ! empty( $color_palette ) check as if the theme does not claim the support, this would be returned (which is not en empty array):

array(1) {
	[0] => bool(false)
}

The is_string() check is there to accommodate the old way of setting colors as add_theme_support( 'editor-color-palette', '#fff', '#000', ... );. Then this was updated to setting colors with multiple array arguments.

This code should do the trick though:

	$color_palette           = (array) get_theme_support( 'editor-color-palette' );

	// Backcompat for Color Palette set as multiple parameters.
	if ( is_string( $color_palette[0] ) || isset( $color_palette[0]['color'] ) ) {
		_doing_it_wrong(
			'add_theme_support()',
			__( 'Setting colors using multiple parameters is deprecated. Please pass a single parameter with an array of colors. See https://wordpress.org/gutenberg/handbook/extensibility/theme-support/ for details.', 'gutenberg' ),
			'3.4.0'
		);
	} else {
		$color_palette = current( $color_palette );
	}

This comment has been minimized.

@webmandesign

webmandesign Jul 3, 2018

Contributor

@felixarntz I've went ahead and created #7695 addressing your concerns. Thanks for pointing this out!

webmandesign added a commit to webmandesign/gutenberg that referenced this pull request Jul 3, 2018

jorgefilipecosta added a commit that referenced this pull request Jul 5, 2018

Improving color palette backcompat (#7695)
Improving color palette theme support backwards compatibility as of @felixarntz [comment](#7619 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment