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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jul 3, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dc0ade2
Color Palette setup with single array argument
webmandesign Jun 28, 2018
8e0930a
Color Palette theme support setup with single array parameter
webmandesign Jun 28, 2018
1465a4b
Multiple parameters backcompat with `_doing_it_wrong` instead of `con…
webmandesign Jul 3, 2018
89e7254
Color Palette set through `gutenberg` array backcompat with `_doing_i…
webmandesign Jul 3, 2018
9c5a526
Updated versions
jorgefilipecosta Jul 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions docs/extensibility/theme-support.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@chrisvanpatten chrisvanpatten Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

array(
'name' => __( 'strong magenta', 'themeLangDomain' ),
'slug' => 'strong-magenta',
Expand All @@ -28,8 +28,8 @@ function mytheme_setup_theme_supported_features() {
'name' => __( 'very dark gray', 'themeLangDomain' ),
'slug' => 'very-dark-gray',
'color' => '#444',
)
);
),
) );
}

add_action( 'after_setup_theme', 'mytheme_setup_theme_supported_features' );
Expand All @@ -50,7 +50,7 @@ add_theme_support( 'align-wide' );
Different blocks have the possibility of customizing colors. Gutenberg provides a default palette, but a theme can overwrite it and provide its own:

```php
add_theme_support( 'editor-color-palette',
add_theme_support( 'editor-color-palette', array(
array(
'name' => __( 'strong magenta', 'themeLangDomain' ),
'slug' => 'strong-magenta',
Expand All @@ -70,8 +70,8 @@ add_theme_support( 'editor-color-palette',
'name' => __( 'very dark gray', 'themeLangDomain' ),
'slug' => 'very-dark-gray',
'color' => '#444',
)
);
),
) );
```

The colors will be shown in order on the palette, and there's no limit to how many can be specified.
Expand Down
21 changes: 15 additions & 6 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,19 +1147,28 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
// Initialize the editor.
$gutenberg_theme_support = get_theme_support( 'gutenberg' );
$align_wide = get_theme_support( 'align-wide' );
$color_palette = get_theme_support( 'editor-color-palette' );
$color_palette = current( (array) get_theme_support( 'editor-color-palette' ) );

// 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' );
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 );
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

_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'
);
}

// Backcompat for Color Palette set through `gutenberg` array.
if ( empty( $color_palette ) && ! empty( $gutenberg_theme_support[0]['colors'] ) ) {
$color_palette = $gutenberg_theme_support[0]['colors'];
}

if ( ! empty( $gutenberg_theme_support ) ) {
wp_add_inline_script(
'wp-edit-post',
'console.warn( "' .
__( 'Adding theme support using the `gutenberg` array is deprecated. See https://wordpress.org/gutenberg/handbook/extensibility/theme-support/ for details.', 'gutenberg' ) .
'");'
_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'
Copy link
Member

Choose a reason for hiding this comment

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

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

);
}

Expand Down