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.

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

Reusable Blocks: Add reusable blocks effects #3377

Merged
merged 5 commits into from
Nov 21, 2017
Merged

Reusable Blocks: Add reusable blocks effects #3377

merged 5 commits into from
Nov 21, 2017

Conversation

noisysocks
Copy link
Member

Adds the effects necessary for supporting reusable blocks. There are 4:

  • FETCH_REUSABLE_BLOCKS: Loads reusable blocks from the API and inserts
    them into the store.
  • SAVE_REUSABLE_BLOCK: Persists a reusable block that's in the store to
    the API.
  • MAKE_BLOCK_STATIC: Transforms a reusable block on the page into a
    regular block.
  • MAKE_BLOCK_REUSABLE: Transforms a regular block on the page into a
    reusable block.

// Ensure that we've got jQuery.
if ( ! wp_script_is( 'jquery', 'done' ) ) {
wp_enqueue_script( 'jquery' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we just add a jquery dependency, something like this 255ef19

Copy link
Member Author

Choose a reason for hiding this comment

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

We can only pass dependencies into wp_enqueue_script, not wp_add_inline_script: https://developer.wordpress.org/reference/functions/wp_add_inline_script/

Copy link
Member

Choose a reason for hiding this comment

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

But wp_add_inline_script is tied to the original handle wp-editor, and we can define jQuery as a dependency for the enqueuing of wp-editor.

wp_add_inline_script(
'wp-editor',
(
'jQuery.when( wp.api.init(), wp.api.init( { versionString: \'gutenberg/v1/\' } ) ).done( function() {'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is jQuery.when the same as Promise.all. It would be great if we can avoid jQuery

Copy link
Member

Choose a reason for hiding this comment

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

I expect init may be returning an instance of jQuery.Deferred. Unsure if Promise.all is capable of handling these.

Copy link
Member Author

@noisysocks noisysocks Nov 20, 2017

Choose a reason for hiding this comment

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

Copy link
Member Author

@noisysocks noisysocks Nov 20, 2017

Choose a reason for hiding this comment

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

Ah, but we can't do this because IE 11 doesn't support promises: https://caniuse.com/#feat=promises

Copy link
Contributor

Choose a reason for hiding this comment

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

We use a promise polyfill :)

wp_add_inline_script(

Copy link
Member Author

Choose a reason for hiding this comment

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

Right you are! 😄 Changed in f5cd6cd.

'wp-editor',
(
'jQuery.when( wp.api.init(), wp.api.init( { versionString: \'gutenberg/v1/\' } ) ).done( function() {'
. 'wp.editor.createEditorInstance( \'editor\', window._wpGutenbergPost, ' . json_encode( $editor_settings ) . ' ); '
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should drop this line? Probably a merge conflict?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! Fixed in 5367b6e.

@mtias mtias added [Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript labels Nov 8, 2017
Adds the effects necessary for supporting reusable blocks. There are 4:

- FETCH_REUSABLE_BLOCKS: Loads reusable blocks from the API and inserts
  them into the store.
- SAVE_REUSABLE_BLOCK: Persists a reusable block that's in the store to
  the API.
- MAKE_BLOCK_STATIC: Transforms a reusable block on the page into a
  regular block.
- MAKE_BLOCK_REUSABLE: Transforms a regular block on the page into a
  reusable block.
@codecov
Copy link

codecov bot commented Nov 20, 2017

Codecov Report

Merging #3377 into master will increase coverage by 2.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3377      +/-   ##
==========================================
+ Coverage    34.9%   36.92%   +2.01%     
==========================================
  Files         263      267       +4     
  Lines        6727     6663      -64     
  Branches     1227     1203      -24     
==========================================
+ Hits         2348     2460     +112     
+ Misses       3694     3551     -143     
+ Partials      685      652      -33
Impacted Files Coverage Δ
editor/effects.js 60.16% <100%> (+14.82%) ⬆️
blocks/api/validation.js 91.46% <0%> (-3.7%) ⬇️
editor/components/inserter/menu.js 85.54% <0%> (-1.79%) ⬇️
editor/components/inserter/index.js 0% <0%> (ø) ⬆️
blocks/library/button/index.js 9.3% <0%> (ø) ⬆️
editor/components/block-settings-menu/index.js 0% <0%> (ø) ⬆️
editor/header/fixed-toolbar-toggle/index.js 0% <0%> (ø) ⬆️
blocks/block-alignment-toolbar/index.js 33.33% <0%> (ø) ⬆️
editor/utils/with-change-detection/index.js 100% <0%> (ø) ⬆️
editor/header/mode-switcher/index.js 0% <0%> (ø) ⬆️
... and 52 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 1709207...e156849. Read the comment docs.

This unnecessary line probably came as a result of a bad merge conflict
resolution.
@noisysocks noisysocks changed the title Add reusable blocks effects Reusable Blocks: Add reusable blocks effects Nov 20, 2017
`gutenberg_collect_meta_box_data` expected that
`window._wpGutenbergEditor` would always be instantiated, but this is
only true for when `wp.api.init()` returns quickly because of the API
schema being cached.

The fix is to use a Promise to store `window._wpGutenbergEditor`.

(Well, a jQuery.Deferred, since we have to support IE 11 which doesn't
support promises.)
Remove jQuery.Deferred and jQuery.when in our editor initialization in
lieu of Promise and Promise.all.
lib/register.php Outdated
@@ -236,7 +236,7 @@ function gutenberg_collect_meta_box_data() {
*/
wp_add_inline_script(
'wp-editor',
'window._wpGutenbergEditor.initializeMetaBoxes( ' . wp_json_encode( $meta_box_data ) . ' )'
'window._wpGutenbergEditor.then( function( editor ) { editor.initializeMetaBoxes( ' . wp_json_encode( $meta_box_data ) . ' ) } );'
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch here! Potential bug fixed. Maybe we should rename the variable to make it clear that it's a promise and not the editor instance. Something like _wpLoadGutenbergEditor

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 e156849

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Nice work

Make it clear that this variable is a promise by giving it a non-noun
name.
@noisysocks noisysocks merged commit 981c22a into WordPress:master Nov 21, 2017
@noisysocks noisysocks deleted the add/reusable-blocks-effects branch November 21, 2017 22:15
@aduth aduth added the [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) label Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants