Update/theme support color palette #6437
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -929,7 +929,7 @@ 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 = current( (array) get_theme_support( 'editor-color-palette' ) ); | ||
|
There was a problem hiding this comment. I think this may be breaking back compatibility with the existing version: Maybe we should handle some condition to be backcompatible. There was a problem hiding this comment. My question would be, is this required? Gutenberg is still in development and from what I can see the theme support declaration have already changed. (Though, I don't know if it was made backwards compatible.) Also, is this backwards compatible too?: If needed, I can surely, edit my code to incorporate backwards compatibility. There was a problem hiding this comment. In my opinion backwards compatible in this case is not required. There was a problem hiding this comment. Hi all, // 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(
'wp-edit-post',
'console.warn( "' .
__( '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' ) .
'");'
);
}Bellow `$color_palette = current( (array) get_theme_support( 'editor-color-palette' ) );``, it should work with both our old versions: and Would it be possible to rebase this branch so we can test with our latest changes, e.g., the mechanism to hide color palettes if we don't have colors to choose? @mtias what are your thoughts? In your opinion, should we advance with this changes and set the color palette using a single array parameter instead of the current way (multiple parameters)? There was a problem hiding this comment. @jorgefilipecosta Thank you for the code provided, I have updated my commits with it. However, please note that this is the first GIT rebase I've ever done. Does feel like I've screwed something (judging by the number of commits). Please check and let me know. I might rather create a new branch again and redo changes for cleaner approach. Thanks! |
||
|
|
||
| // Backcompat for Color Palette set through `gutenberg` array. | ||
| if ( empty( $color_palette ) && ! empty( $gutenberg_theme_support[0]['colors'] ) ) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be also:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just added the array wrapper there because of @jorgefilipecosta's comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we map it to an ordered array before it gets exposed to JavaScript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gziolo
You mean like declaring the theme support with this:
and then transform it in PHP to a JS required form of:
?
If so, this is certainly doable. But I'd like some more input on this, more opinions (maybe from @jorgefilipecosta)? You see, current associated array implementation is more self-explanatory in my opinion and could actually serve well in case of future extending of the palette color features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's what I meant. I don't care as much, I just wanted to make life of theme developers easier and help them save some keystrokes 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Although I agree that it would be easier for themes to declare support using this syntax:
I think it could limit future advances as the color value itself is a string and not an object we can expand.
For example, one possible improvement I was thinking about was to let themes define the contexts in which colors appear (e.g: allow some colors only for background).
In the current syntax, it is a matter of allowing an optimal property:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like expected ;)