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

Themes: Add the wideImages theme support config #2021

Merged
merged 6 commits into from Jul 31, 2017

Conversation

Projects
None yet
7 participants
@youknowriad
Contributor

youknowriad commented Jul 26, 2017

closes #1724

The backend passes an editorSettings variable when initializing the editor. This object could contain any global editor setting such as the wideImages flag for now.

I'm storing these settings in the editor's store but we could decide to pass it as prop down to where it's being used.

By default, the wideImages is disabled and we could opt-in by calling add_theme_support( 'wide-images' );

@youknowriad youknowriad self-assigned this Jul 26, 2017

@youknowriad youknowriad requested review from mtias, jasmussen and aduth Jul 26, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jul 26, 2017

Contributor

Yaay, thank you for working on this.

I'm now not seeing the wide and full wide options running Twenty Seventeen (which is to be expected, that's the point, 👍 👍).

I did, however, try to add add_theme_support( 'wide-images' ); to the twentyseventeen_setup() function in functions.php, and a reload did not seem to bring those buttons back. Do I need to do anything else to test?

This PR will also put a fire under us to start blockifying Twenty Seventeen so we have a great example theme that works great with Gutenberg.

Contributor

jasmussen commented Jul 26, 2017

Yaay, thank you for working on this.

I'm now not seeing the wide and full wide options running Twenty Seventeen (which is to be expected, that's the point, 👍 👍).

I did, however, try to add add_theme_support( 'wide-images' ); to the twentyseventeen_setup() function in functions.php, and a reload did not seem to bring those buttons back. Do I need to do anything else to test?

This PR will also put a fire under us to start blockifying Twenty Seventeen so we have a great example theme that works great with Gutenberg.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 26, 2017

Contributor

I did, however, try to add add_theme_support( 'wide-images' ); to the twentyseventeen_setup() function in functions.php, and a reload did not seem to bring those buttons back. Do I need to do anything else to test?

I did try that but directly in the gutenberg code, maybe it's a bug related to the order of function calls (looking)

Contributor

youknowriad commented Jul 26, 2017

I did, however, try to add add_theme_support( 'wide-images' ); to the twentyseventeen_setup() function in functions.php, and a reload did not seem to bring those buttons back. Do I need to do anything else to test?

I did try that but directly in the gutenberg code, maybe it's a bug related to the order of function calls (looking)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 26, 2017

Contributor

@jasmussen I just added add_theme_support( 'wide-images' ); at the end of the functions file in the same theme and it worked. Maybe we should delay the editor initialization after the after_setup_theme hook? I'm not an expert in WP hooks, I'd rather ask for advice here @aduth

Contributor

youknowriad commented Jul 26, 2017

@jasmussen I just added add_theme_support( 'wide-images' ); at the end of the functions file in the same theme and it worked. Maybe we should delay the editor initialization after the after_setup_theme hook? I'm not an expert in WP hooks, I'd rather ask for advice here @aduth

Show outdated Hide outdated blocks/library/cover-image/index.js
@@ -52,7 +52,7 @@ registerBlockType( 'core/cover-image', {
<BlockAlignmentToolbar
value={ align }
onChange={ updateAlignment }
controls={ validAlignments }
controls={ [ 'left', 'center', 'right' ].concat( settings.wideImages ? [ 'wide', 'full' ] : [] ) }

This comment has been minimized.

@aduth

aduth Jul 26, 2017

Member

Should we bake this into the BlockAlignmentToolbar component, since it already has some knowledge of wide and full widths? Maybe as a boolean prop.

@aduth

aduth Jul 26, 2017

Member

Should we bake this into the BlockAlignmentToolbar component, since it already has some knowledge of wide and full widths? Maybe as a boolean prop.

Show outdated Hide outdated lib/client-assets.php
@@ -566,7 +566,13 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
);
// Initialize the editor.
wp_add_inline_script( 'wp-editor', 'wp.api.init().done( function() { wp.editor.createEditorInstance( \'editor\', window._wpGutenbergPost ); } );' );
$editor_settings = array(
'wideImages' => get_theme_support( 'wide-images' ),

This comment has been minimized.

@aduth

aduth Jul 26, 2017

Member
  • Instead of one-off theme supports, we might consider a single umbrella theme support key, whose values is an array with one or more of wide-images, etc? Depends if we think "wide-images" has value as a theme support outside just the editor.
  • What about themes which support full images, but not wide, or vice-versa? Should we be more granular here?
@aduth

aduth Jul 26, 2017

Member
  • Instead of one-off theme supports, we might consider a single umbrella theme support key, whose values is an array with one or more of wide-images, etc? Depends if we think "wide-images" has value as a theme support outside just the editor.
  • What about themes which support full images, but not wide, or vice-versa? Should we be more granular here?

This comment has been minimized.

@youknowriad

youknowriad Jul 27, 2017

Contributor

Good questions, I'm happy to use whatever we settle on. So you're proposing something like:

add_theme_support( 'gutenberg-theme-support', [ 'wide-images' ] );

I kind of like the one liner if you want to enable a feature (I mean the current approach) but no strong opinion here. cc @jasmussen

@youknowriad

youknowriad Jul 27, 2017

Contributor

Good questions, I'm happy to use whatever we settle on. So you're proposing something like:

add_theme_support( 'gutenberg-theme-support', [ 'wide-images' ] );

I kind of like the one liner if you want to enable a feature (I mean the current approach) but no strong opinion here. cc @jasmussen

This comment has been minimized.

@youknowriad

youknowriad Jul 27, 2017

Contributor

Oh! I just saw the proposals down here. This is already answered in a way.

@youknowriad

youknowriad Jul 27, 2017

Contributor

Oh! I just saw the proposals down here. This is already answered in a way.

Show outdated Hide outdated lib/client-assets.php
'wideImages' => get_theme_support( 'wide-images' ),
);
wp_add_inline_script( 'wp-editor', 'wp.api.init().done( function() {'
. 'wp.editor.createEditorInstance( \'editor\', window._wpGutenbergPost, ' . json_encode( $editor_settings ) . ' ); '

This comment has been minimized.

@aduth

aduth Jul 26, 2017

Member

Above we're already creating a global object via wp_localize_script, used to initialize TinyMCE settings. I like in this case that we're more explicit about passing an options object to the createEditorInstance (vs. assuming globals from code itself), but wondering if we should merge $editor_settings into wpEditorL10n and pass this object from the corresponding property e.g. wpEditorL10n.settings

@aduth

aduth Jul 26, 2017

Member

Above we're already creating a global object via wp_localize_script, used to initialize TinyMCE settings. I like in this case that we're more explicit about passing an options object to the createEditorInstance (vs. assuming globals from code itself), but wondering if we should merge $editor_settings into wpEditorL10n and pass this object from the corresponding property e.g. wpEditorL10n.settings

This comment has been minimized.

@westonruter

westonruter Jul 27, 2017

Member

Instead of moving more values to the global wpEditorL10n should we not opt to move more values into the object being passed into createEditorInstance? I think we should eliminate wp_localize_script() and explicitly pass what is being exported as wpEditorL10n into the editor here as well. There's going to be a need to export more data from PHP into the editor, such as the server's date formatting in #2013. So the more that we can establish a single pattern for getting data from PHP into JS the better.

@westonruter

westonruter Jul 27, 2017

Member

Instead of moving more values to the global wpEditorL10n should we not opt to move more values into the object being passed into createEditorInstance? I think we should eliminate wp_localize_script() and explicitly pass what is being exported as wpEditorL10n into the editor here as well. There's going to be a need to export more data from PHP into the editor, such as the server's date formatting in #2013. So the more that we can establish a single pattern for getting data from PHP into JS the better.

This comment has been minimized.

@youknowriad

youknowriad Jul 27, 2017

Contributor

I think there's a global reflection to have here the more we create JS UI. (not only Gutenberg).
A proposal could be:

  • if you need date and you enqueue wp-date you automatically get a wpDateSettings global variable.
  • if you need i18n and you enqueue wp-i18n you automatically get a wpI18nSettings global.
  • if you need themes support and you enqueue wp-themes you get wpThemeSettings global.

I don't if this proposal is good but we need to find a consistent way of retrieving these settings. An automatically injected global variable or an endpoint (the first one is more "reactive" and performant I guess)

But at the same time, I'd like us to avoid the globals in the Editor Code itself (to make it reusable), so a settings variable in createEditorInstance makes sense to me and could be computed using these globals.

@youknowriad

youknowriad Jul 27, 2017

Contributor

I think there's a global reflection to have here the more we create JS UI. (not only Gutenberg).
A proposal could be:

  • if you need date and you enqueue wp-date you automatically get a wpDateSettings global variable.
  • if you need i18n and you enqueue wp-i18n you automatically get a wpI18nSettings global.
  • if you need themes support and you enqueue wp-themes you get wpThemeSettings global.

I don't if this proposal is good but we need to find a consistent way of retrieving these settings. An automatically injected global variable or an endpoint (the first one is more "reactive" and performant I guess)

But at the same time, I'd like us to avoid the globals in the Editor Code itself (to make it reusable), so a settings variable in createEditorInstance makes sense to me and could be computed using these globals.

This comment has been minimized.

@westonruter

westonruter Jul 28, 2017

Member

@youknowriad:

But at the same time, I'd like us to avoid the globals in the Editor Code itself (to make it reusable), so a settings variable in createEditorInstance makes sense to me and could be computed using these globals.

Yes! I was thinking something similar. So perhaps each individual script could export to a global, but then we could use jQuery.extend() to merge them together just-in-time. For example:

wp_add_inline_script( 'wp-editor', 
    sprintf(
        'wp.api.init().done( function() { wp.editor.createEditorInstance( "editor", %s ); } );',
        sprintf( 'jQuery.extend( {}, 
            { post: window._wpGutenbergPost },
            { date: wpDateSettings },
            { i18n: wpI18nSettings },
            { themes: wpThemeSettings },
            { editor: %s } )', 
            wp_json_encode( $editor_settings ) 
        )
    ) 
);

If wp-editor depends on wp-date, wp-i18n, wp-themes, any other scripts to which the corresponding globals are attached, then all of these globals will be guaranteed to have already been output to the browser and we'd be able to refer to them. We'd then eliminate the reliance on global variables.

@westonruter

westonruter Jul 28, 2017

Member

@youknowriad:

But at the same time, I'd like us to avoid the globals in the Editor Code itself (to make it reusable), so a settings variable in createEditorInstance makes sense to me and could be computed using these globals.

Yes! I was thinking something similar. So perhaps each individual script could export to a global, but then we could use jQuery.extend() to merge them together just-in-time. For example:

wp_add_inline_script( 'wp-editor', 
    sprintf(
        'wp.api.init().done( function() { wp.editor.createEditorInstance( "editor", %s ); } );',
        sprintf( 'jQuery.extend( {}, 
            { post: window._wpGutenbergPost },
            { date: wpDateSettings },
            { i18n: wpI18nSettings },
            { themes: wpThemeSettings },
            { editor: %s } )', 
            wp_json_encode( $editor_settings ) 
        )
    ) 
);

If wp-editor depends on wp-date, wp-i18n, wp-themes, any other scripts to which the corresponding globals are attached, then all of these globals will be guaranteed to have already been output to the browser and we'd be able to refer to them. We'd then eliminate the reliance on global variables.

Show outdated Hide outdated editor/index.js
@@ -20,6 +20,10 @@ import './assets/stylesheets/main.scss';
import Layout from './layout';
import { createReduxStore } from './state';
const defaultSettings = {

This comment has been minimized.

@aduth

aduth Jul 26, 2017

Member

Can we use SCREAMING_SNAKE_CASE for constants? Also a DocBlock comment would be a nice addition (though it's admittedly fairly self-explanatory, for consistency's / habit's / self-enforcement's sake).

@aduth

aduth Jul 26, 2017

Member

Can we use SCREAMING_SNAKE_CASE for constants? Also a DocBlock comment would be a nice addition (though it's admittedly fairly self-explanatory, for consistency's / habit's / self-enforcement's sake).

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 26, 2017

Member

after_setup_theme occurs fairly early in the page lifecycle, certainly prior to admin_enqueue_scripts in which we bootstrap the editor.

See also: https://codex.wordpress.org/Plugin_API/Action_Reference

In my testing, adding and removing add_theme_support( 'wide-images' ); does have an effect on whether those alignment options are present.... i.e. "wfm" 😆

Member

aduth commented Jul 26, 2017

after_setup_theme occurs fairly early in the page lifecycle, certainly prior to admin_enqueue_scripts in which we bootstrap the editor.

See also: https://codex.wordpress.org/Plugin_API/Action_Reference

In my testing, adding and removing add_theme_support( 'wide-images' ); does have an effect on whether those alignment options are present.... i.e. "wfm" 😆

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jul 26, 2017

Contributor

Let's do something that could include further items, like colors, width, etc, via add_theme_support( 'gutenberg', array() ) or similar.

Contributor

mtias commented Jul 26, 2017

Let's do something that could include further items, like colors, width, etc, via add_theme_support( 'gutenberg', array() ) or similar.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jul 27, 2017

Member

Let's do something that could include further items, like colors, width, etc, via add_theme_support( 'gutenberg', array() ) or similar.

There are limitations in the theme supports API here, specifically when making multiple calls to add_theme_support to add features. Consider, for example, a parent theme and a child theme each adding support for theme features separate. For example:

// Parent:
add_theme_support( 'gutenberg', array(
	'wide-images' => true,
) );

// Child
add_theme_support( 'gutenberg', array(
	'colors' => true,
) );

// Result:
get_theme_support( 'gutenberg' ) == array( 'colors' => true ); // FAIL

So if this were done, then if child themes ever added theme support, they'd have to always do this:

add_theme_support( 'gutenberg', array_merge(
	(array) get_theme_support( 'gutenberg' ), 
	array(
		'colors' => true,
	) 
) );

Which isn't great.

Member

westonruter commented Jul 27, 2017

Let's do something that could include further items, like colors, width, etc, via add_theme_support( 'gutenberg', array() ) or similar.

There are limitations in the theme supports API here, specifically when making multiple calls to add_theme_support to add features. Consider, for example, a parent theme and a child theme each adding support for theme features separate. For example:

// Parent:
add_theme_support( 'gutenberg', array(
	'wide-images' => true,
) );

// Child
add_theme_support( 'gutenberg', array(
	'colors' => true,
) );

// Result:
get_theme_support( 'gutenberg' ) == array( 'colors' => true ); // FAIL

So if this were done, then if child themes ever added theme support, they'd have to always do this:

add_theme_support( 'gutenberg', array_merge(
	(array) get_theme_support( 'gutenberg' ), 
	array(
		'colors' => true,
	) 
) );

Which isn't great.

@youknowriad youknowriad referenced this pull request Jul 27, 2017

Merged

Add "More" block. #1440

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 27, 2017

Contributor

@westonruter has some good points here, should we stick with an individual key for each theme support?

Contributor

youknowriad commented Jul 27, 2017

@westonruter has some good points here, should we stick with an individual key for each theme support?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jul 27, 2017

Contributor

@davidakennedy @obenland what are your thoughts?

Contributor

mtias commented Jul 27, 2017

@davidakennedy @obenland what are your thoughts?

@obenland

This comment has been minimized.

Show comment
Hide comment
@obenland

obenland Jul 27, 2017

Member

While it's true that "adding" theme support in a child theme doesn't really add to but overwrites a parent's theme support, I still think that Gutenberg-related support clauses should be bundled in a common namespace.

The amount of features themes can add support for are already getting unwieldy, let not add to that. I don't see why it couldn't work like custom-header or custom-background for example.

Member

obenland commented Jul 27, 2017

While it's true that "adding" theme support in a child theme doesn't really add to but overwrites a parent's theme support, I still think that Gutenberg-related support clauses should be bundled in a common namespace.

The amount of features themes can add support for are already getting unwieldy, let not add to that. I don't see why it couldn't work like custom-header or custom-background for example.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 28, 2017

Contributor

Ok the majority wins ;) updated to use this syntax:

add_theme_support( 'gutenberg', array(
	'wide-images' => true,
) );
Contributor

youknowriad commented Jul 28, 2017

Ok the majority wins ;) updated to use this syntax:

add_theme_support( 'gutenberg', array(
	'wide-images' => true,
) );
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jul 28, 2017

Codecov Report

Merging #2021 into master will increase coverage by 0.08%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2021      +/-   ##
==========================================
+ Coverage   20.25%   20.34%   +0.08%     
==========================================
  Files         135      135              
  Lines        4227     4233       +6     
  Branches      718      721       +3     
==========================================
+ Hits          856      861       +5     
+ Misses       2842     2841       -1     
- Partials      529      531       +2
Impacted Files Coverage Δ
blocks/library/image/index.js 15.62% <ø> (ø) ⬆️
blocks/library/cover-image/index.js 30.43% <ø> (ø) ⬆️
blocks/library/table/index.js 36.36% <ø> (ø) ⬆️
blocks/library/gallery/index.js 25.71% <ø> (ø) ⬆️
blocks/library/pullquote/index.js 33.33% <ø> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/index.js 0% <0%> (ø) ⬆️
blocks/library/latest-posts/index.js 10% <0%> (ø) ⬆️
blocks/library/embed/index.js 45.97% <0%> (ø) ⬆️
editor/selectors.js 96.33% <100%> (+0.03%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f459f...77fde94. Read the comment docs.

codecov bot commented Jul 28, 2017

Codecov Report

Merging #2021 into master will increase coverage by 0.08%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2021      +/-   ##
==========================================
+ Coverage   20.25%   20.34%   +0.08%     
==========================================
  Files         135      135              
  Lines        4227     4233       +6     
  Branches      718      721       +3     
==========================================
+ Hits          856      861       +5     
+ Misses       2842     2841       -1     
- Partials      529      531       +2
Impacted Files Coverage Δ
blocks/library/image/index.js 15.62% <ø> (ø) ⬆️
blocks/library/cover-image/index.js 30.43% <ø> (ø) ⬆️
blocks/library/table/index.js 36.36% <ø> (ø) ⬆️
blocks/library/gallery/index.js 25.71% <ø> (ø) ⬆️
blocks/library/pullquote/index.js 33.33% <ø> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/index.js 0% <0%> (ø) ⬆️
blocks/library/latest-posts/index.js 10% <0%> (ø) ⬆️
blocks/library/embed/index.js 45.97% <0%> (ø) ⬆️
editor/selectors.js 96.33% <100%> (+0.03%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f459f...77fde94. Read the comment docs.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jul 28, 2017

Contributor

Since this will remove some of the most prominent toolbar options, let's wait until next week so we can have a basic theme to go along with it.

Contributor

mtias commented Jul 28, 2017

Since this will remove some of the most prominent toolbar options, let's wait until next week so we can have a basic theme to go along with it.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 31, 2017

Contributor

pinging @karmatosed because I believe you're working on a Gutenberg Theme?

I'm willing to merge this but I'm waiting for a theme implementing it :)

Contributor

youknowriad commented Jul 31, 2017

pinging @karmatosed because I believe you're working on a Gutenberg Theme?

I'm willing to merge this but I'm waiting for a theme implementing it :)

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Jul 31, 2017

Member

@youknowriad just added it here: WordPress/gutenberg-starter-theme#5. Thanks for ping, I think I got the syntax right? Does it still use the same CSS class?

Member

karmatosed commented Jul 31, 2017

@youknowriad just added it here: WordPress/gutenberg-starter-theme#5. Thanks for ping, I think I got the syntax right? Does it still use the same CSS class?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 31, 2017

Contributor

👍 cool, let's merge this one then. The classes are unchanged.

Contributor

youknowriad commented Jul 31, 2017

👍 cool, let's merge this one then. The classes are unchanged.

@youknowriad youknowriad merged commit dbc8680 into master Jul 31, 2017

3 checks passed

codecov/project 20.34% (+0.08%) compared to 39f459f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/wide-images-theme-support branch Jul 31, 2017

### Wide Images:
Some blocks such as the image block have the possibility to define a "wide" or "full" alignment by adding the corresponding classname to the block's wrapper ( `alignwide` or `alignfull` ). A theme can opt-in for this feature by calling:

This comment has been minimized.

@aduth

aduth Aug 3, 2017

Member

When should they call this? ?after_setup_theme?

@aduth

aduth Aug 3, 2017

Member

When should they call this? ?after_setup_theme?

This comment has been minimized.

@westonruter

westonruter Aug 3, 2017

Member

Yes. This is when the core-bundled themes call add_theme_support()

@westonruter

westonruter Aug 3, 2017

Member

Yes. This is when the core-bundled themes call add_theme_support()

This comment has been minimized.

@aduth

aduth Aug 3, 2017

Member

Right, we should document as such.

@aduth

aduth Aug 3, 2017

Member

Right, we should document as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment