Rename theme support for wide images to `align-wide`. #4153

getsource · 2017-12-22T20:24:55Z

Description

Changes theme support for wide images from being within the gutenberg theme support array with a wide-images bool to adding it with: add_theme_support( 'align-wide' );

Additionally, updates documentation and related JS variable names to match.

How Has This Been Tested?

This was tested by adding theme support for align-wide to Twenty Seventeen.
Caveat: The demo content still has a wide image included, and it displays this way whether or not align-wide is enabled in the theme. As far as I can tell, this was also the case with the previous name.

Types of changes

This is a breaking change for themes that have been added wide image support using the previously supported method. They would need to change to the new naming for the wide and full image controls to display.
Fixes: #4113

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

youknowriad

Nice work here!

youknowriad · 2017-12-22T20:35:22Z

docs/themes.md

@@ -34,7 +37,7 @@ Different blocks have the possibility of customizing colors. Gutenberg provides

 ```php
 add_theme_support( 'gutenberg', array(
-   'colors' => array(
+	'colors' => array(


Should we do the same for the colors? I mean dropping the gutenberg code name. Maybe something like add_theme_support( 'colors', $colors );

Yeah, thinking that makes sense. Should that happen under a different issue?

I'm fine doing it in the same PR, that way we can check for the "deprecated" theme support only once.

Okay. Ended up doing a first run with only wide images changes/warnings first. If that's close, I'll expand it to colors.

youknowriad · 2017-12-22T20:37:05Z

lib/client-assets.php

@@ -817,7 +818,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
 	$allowed_block_types = apply_filters( 'allowed_block_types', true );

 	$editor_settings = array(
-		'wideImages'       => ! empty( $gutenberg_theme_support[0]['wide-images'] ),
+		'alignWide'        => $align_wide,


I'm thinking for one Gutenberg release or two, we should support both and maybe warn the user (console.warn for instance) if we detect the usage of the old gutenberg theme support?

Sure, can do. Wasn't sure how much backcompat is expected at the moment.

Yeah! not sure either, but keeping it for one or two releases seems reasonable to me. We're doing the same thing (warning) for the deprecated block attributes definition.

I love the idea of supporting both for a few releases and maybe throwing a _doing_it_wrong or similar for wideImages

getsource · 2017-12-22T22:36:54Z

editor/components/provider/index.js

+				"See https://wordpress.org/gutenberg/handbook/reference/theme-support/"
+			) );
+		}
+


Not sure if I'm doing this in the right spot. Feel free to point me in another direction if not.

RE Travis errors:
Looks like I need to use a different quote style here, which is easy enough (whoops). That won't fix that it doesn't like that I'm outputting to the console, so going to dig a bit more on the other warning you referenced @youknowriad.

Looks like in the other case (https://github.com/WordPress/gutenberg/blob/master/blocks/hooks/matchers.js#L12), the console warning is being ignored through a comment.

Pushed another commit that should help ESLint / Travis pass.

Yep, it helps if you can install an eslint plugin in your favorite IDE to catch the errors "live". You can also run npm run lint locally.

That said, I wonder if this check should be done server-side instead,

if (! empty( $gutenberg_theme_support)) { wp_add_inline_script( 'wp-editor', 'console.warn("message")' ); }

and update the setting

$editor_settings = array( 'alignWide' => $align_wide || ! empty( $gutenberg_theme_support[0]['wide-images'] ) )

That way the client is consistent. What do you think?

Thanks! For some reason, I (incorrectly) assumed that lint was run as part of build when using npm run dev. Have been running it manually to check since learning otherwise, because some of the editing is in vim, and I don't have an eslint plugin installed there yet.

I'd been assuming the client/JS side due to the console.warn() suggestion, but have no problem at all with that route.

If we're going to solve it server-side, should it be with _doing_it_wrong like @aaronjorbin suggested, or is sending the warning across via wp_add_inline_script() more idiomatic and/or likely to be noticed by devs?

If we're going to solve it server-side, should it be with _doing_it_wrong like @aaronjorbin suggested, or is sending the warning across via wp_add_inline_script() more idiomatic and/or likely to be noticed by devs?

Not strong opinion myself. I'd personally use console.warn for consistency with the other "depreacted" API we have in Gutenberg. I don't know if plugins/theme authors are already familiar with the _doing_it_wrong behavior in other parts of WordPress, in which case, it would be a better choice.

In other words, I'm personally ok with both. The important thing is that the user notices it. What do you think @aaronjorbin ?

I'll go ahead and put something together, and can always swap out the method upon feedback.

Another question: Any suggestions on the best naming for the colors option, since I'm assuming add_theme_support( 'colors' ); isn't appropriate?

editor-color-palette perhaps?

editor-color-palette

That sounds good.

mcsf · 2018-01-02T19:30:52Z

This looks quite good so far! @getsource, let us know once you'd like another look (per #4153 (comment)).

aduth · 2018-01-02T21:49:39Z

Noting that this change is also proposed in the implementation of #4069 (see "Implementation notes").

getsource · 2018-01-03T19:10:21Z

I think this is ready for another look. Added colors to the mix and moved the warning. We could throw both a warning and a _doing_it_wrong(), too. Open to either one.

Re: @aduth's comment, if this, or part of this, is best done in a different pull request or issue, I'm fine with that. Just let me know so that I don't continue here. :)

youknowriad

Nice work 👍

aduth · 2018-01-04T21:08:24Z

docs/themes.md

@@ -4,43 +4,42 @@ By default, blocks provide their styles to enable basic support for blocks in th

 Some advanced block features require opt-in support in the theme itself as it's difficult for the block to provide these styles, they may require some architecting of the theme itself, in order to work well.

-To opt-in for one of these features, we should call `add_theme_support( 'gutenberg', $features )` in the `functions.php` file of the theme. For example:
+To opt-in for one of these features, we should call `add_theme_support( 'feature-name', $optional-parameters )` in the `functions.php` file of the theme. For example:


Noting that this is a syntax error on $optional-parameters (dashes aren't valid in variable name). I think we might want to drop the arguments here since they're a bit misleading (reader might not interpret 'feature-name' as dummy text).

To opt in for one of these features, we should call add_theme_support in the functions.php file of the theme:

Thanks for pointing that out! Wasn't sure what was optimal here.

Good suggestion, especially since there's a full example immediately below. Added a commit to make this change. I also removed the "we" wording, since it didn't seem to be used in other places in this doc, and it was a little confusing who it was referring to. If that's not wanted, I can revert it.

Changes theme support for wide images from being within the `gutenberg` theme support array with a `wide-images` bool to adding it with: `add_theme_support( 'align-wide' );` Additionally, updates documentation and variable names to match. See: #4113

Includes backcompat for declaring wide image support with `add_theme_support` and `wide-images` inside the gutenberg array. Issues a JS console warning with a link to documentation if it is not declared with `add_theme_support( 'align-wide' );`.

- When warning about `wide-images` usage, use single quotes for formatting. - Whitelist usage of `console` for `wide-images` warning.

- Moves the console warning to be generated on the server side - Moves `colors` out of the array as well, with `editor-color-palette` as a replacement theme support flag. - Updates docs to reflect both of these.

… the command more generic WordPress/gutenberg#4153

aduth · 2019-01-23T23:47:22Z

Let's formally deprecate it, I think: #13458

youknowriad reviewed Dec 22, 2017

View reviewed changes

youknowriad requested review from mcsf, karmatosed, jasmussen and aduth December 22, 2017 20:47

getsource commented Dec 22, 2017

View reviewed changes

youknowriad approved these changes Jan 4, 2018

View reviewed changes

aduth reviewed Jan 4, 2018

View reviewed changes

getsource added 5 commits January 4, 2018 17:01

Resolve eslint errors.

465599a

- When warning about `wide-images` usage, use single quotes for formatting. - Whitelist usage of `console` for `wide-images` warning.

Move alerts to PHP and move colors out of array.

730107b

- Moves the console warning to be generated on the server side - Moves `colors` out of the array as well, with `editor-color-palette` as a replacement theme support flag. - Updates docs to reflect both of these.

Remove dummy variables in docs for theme features.

77e1c5f

youknowriad merged commit c9a809d into WordPress:master Jan 5, 2018

BinaryMoon added a commit to BinaryMoon/granule that referenced this pull request Jan 5, 2018

Change full width image support to match changes in Gutenberg to make…

c0b6233

… the command more generic WordPress/gutenberg#4153

aduth mentioned this pull request Jan 23, 2019

Plugin: Deprecate gutenberg theme support #13458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename theme support for wide images to `align-wide`. #4153

Rename theme support for wide images to `align-wide`. #4153

getsource commented Dec 22, 2017

youknowriad left a comment

youknowriad Dec 22, 2017

getsource Dec 22, 2017

youknowriad Dec 22, 2017

getsource Dec 22, 2017

youknowriad Dec 22, 2017

getsource Dec 22, 2017

youknowriad Dec 22, 2017

aaronjorbin Dec 22, 2017

getsource Dec 22, 2017

getsource Dec 22, 2017

getsource Dec 22, 2017

youknowriad Dec 23, 2017

getsource Dec 23, 2017

youknowriad Dec 23, 2017

getsource Jan 2, 2018 •

edited

Loading

getsource Jan 2, 2018

mcsf Jan 2, 2018

mcsf commented Jan 2, 2018

aduth commented Jan 2, 2018

getsource commented Jan 3, 2018

youknowriad left a comment

aduth Jan 4, 2018

getsource Jan 4, 2018

aduth commented Jan 23, 2019

Rename theme support for wide images to align-wide. #4153

Rename theme support for wide images to align-wide. #4153

Conversation

getsource commented Dec 22, 2017

Description

How Has This Been Tested?

Types of changes

Checklist:

youknowriad left a 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

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

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

Choose a reason for hiding this comment

getsource Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Jan 2, 2018

aduth commented Jan 2, 2018

getsource commented Jan 3, 2018

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jan 23, 2019

Rename theme support for wide images to `align-wide`. #4153

Rename theme support for wide images to `align-wide`. #4153

getsource Jan 2, 2018 •

edited

Loading