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

[RNMobile] Shortcode block support #19534

Merged
merged 14 commits into from
Jan 15, 2020
Merged

[RNMobile] Shortcode block support #19534

merged 14 commits into from
Jan 15, 2020

Conversation

chipsnyder
Copy link
Contributor

@chipsnyder chipsnyder commented Jan 9, 2020

Description

Adds React Native support for the shortcode block and modifies the shortcode PHP registration so that attributes are exposed to both the javascript and PHP registration.

Implements: wordpress-mobile/gutenberg-mobile#690

How has this been tested?

Add New Shortcode

  • Open WP iOS / WP Android
  • Go to Site Pages / Blog Posts
  • Add a new page/post or open an existing page/post
  • Add a new block by selecting the (+) sign in the bottom left
  • Select Shortcode
  • Type Shortcode
  • Save
iOS Android
iOS - New Block Android - New Block

Edit existing Shortcode

  • Open WP iOS / WP Android
  • Go to Site Pages / Blog Posts
  • Open an existing page/post that has an existing shortcode
  • Select Shortcode
  • Edit Shortcode
  • Save
iOS Android
iOS - Edit Block Android - Edit Block

Screenshots

Placeholder

iOS

Default Dark mode
Placeholder-iOS iOS Dark - Placeholder
iPad Default iPad Dark mode
iPad - Placeholder iPad Dark - Placeholder

Android

Default
Android - Placeholder

Static

iOS

Default Dark mode
Static-iOS iOS Dark - Static
iPad Default iPad Dark mode
iPad - Static iPad Dark - Static

Android

Default
Android - Static

Selected

iOS

Default Dark mode
Selected-iOS iOS Dark - Selected
iPad Default iPad Dark mode
iPad - Selected iPad Dark - Selected

Android

Default
Android - Selected

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@chipsnyder chipsnyder closed this Jan 9, 2020
@chipsnyder chipsnyder reopened this Jan 9, 2020
@@ -21,15 +21,13 @@ function render_block_core_shortcode( $attributes, $content ) {
* Registers the `core/shortcode` block on server.
*/
function register_block_core_shortcode() {
$path = gutenberg_dir_path() . 'packages/block-library/src/shortcode/block.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

@gziolo I don't think this would work when this makes it into core, but I'm not sure what's the right way to load this in PHP. Any tips? I see you did a similar thing in #14238, but it never shipped in core, so I'm not sure if it's a solved problem

Copy link
Member

Choose a reason for hiding this comment

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

It's quite a task to coordinate both Gutenberg and WordPress core :)

PHP files without any change are copied to this location in WordPress core:

https://github.com/WordPress/wordpress-develop/tree/master/src/wp-includes/blocks

block.json files aren't copied over in both Gutenberg and WordPress core and with the current folder structure, I don't think it would be possible.

All PHP are whitelisted in WP core in here:
https://github.com/WordPress/wordpress-develop/blob/14376be6e195ce9cc7f4bf237651504777b504fb/tools/webpack/packages.js#L97-L109

I think it wouldn't be that hard to change it to something else and include block.json files there. I guess the simplest approach would be to put PHP file and its JSON file in the same folder to make them easy to reference.

In Gutenberg, PHP files for blocks are copied to the build folder with this logic:

gutenberg/webpack.config.js

Lines 117 to 151 in 3caee37

new CopyWebpackPlugin( [
{
from: './packages/block-library/src/**/index.php',
test: new RegExp( `([\\w-]+)${ escapeRegExp( sep ) }index\\.php$` ),
to: 'build/block-library/blocks/[1].php',
transform( content ) {
content = content.toString();
// Within content, search for any function definitions. For
// each, replace every other reference to it in the file.
return content
.match( /^function [^\(]+/gm )
.reduce( ( result, functionName ) => {
// Trim leading "function " prefix from match.
functionName = functionName.slice( 9 );
// Prepend the Gutenberg prefix, substituting any
// other core prefix (e.g. "wp_").
return result.replace(
new RegExp( functionName, 'g' ),
( match ) => 'gutenberg_' + match.replace( /^wp_/, '' )
);
}, content )
// The core blocks override procedure takes place in
// the init action default priority to ensure that core
// blocks would have been registered already. Since the
// blocks implementations occur at the default priority
// and due to WordPress hooks behavior not considering
// mutations to the same priority during another's
// callback, the Gutenberg build blocks are modified
// to occur at a later priority.
.replace( /(add_action\(\s*'init',\s*'gutenberg_register_block_[^']+'(?!,))/, '$1, 20' );
},
},
] ),

@aduth can provide more details for the PHP changes applied in Gutenberg if you need it for anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite a task to coordinate both Gutenberg and WordPress core :)

😄


Thanks for the guidance @gziolo, I'll get to work on including the block.json files in those directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gziolo

I went ahead and made those adjustments
For Gutenberg the change is here:
https://github.com/WordPress/gutenberg/pull/19534/files#diff-11e9f7f953edc64ba14b0cc350ae7b9dR152-R158

Then in WordPress core, I made the changes here:
https://github.com/chipsnyder/wordpress-develop/blob/rnmobile/shortcode-support/tools/webpack/packages.js#L155-L163

and here:
https://github.com/chipsnyder/wordpress-develop/blob/rnmobile/shortcode-support/tools/webpack/packages.js#L244

I haven't opened up a PR for the changes to WordPress core yet because I wanted to check to see if this was in line with what you were thinking. I also wasn't sure about the process for that on updating the block-library package then committing the change and so forth.

Copy link
Member

@gziolo gziolo Jan 14, 2020

Choose a reason for hiding this comment

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

Yes, this is more or less what's necessary to get started. There is this question whether the PHP file and block.json file should be put in their own folder:

-- build/block-library/blocks
   -- shortcode
     -- index.php
     -- block.json

Copy link
Contributor

Choose a reason for hiding this comment

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

If block.json is renamed to shortcode.json then we'll have to replace the filename to read in the php file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then we'll have to replace the filename to read in the php file as well.

Is this ( https://github.com/WordPress/gutenberg/pull/19534/files#diff-0272f9f4a31a060e383e4d3cf5d5c7feR24 ) the change you're thinking of?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I missed that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I missed that one

No problem 🙂

I opened a PR for the WordPress-develop changes WordPress/wordpress-develop#131
@gziolo or @koke Do either of you know the process for updating the block-library version there?
Should it be part of that PR or should I open a new one after this merges?

Copy link
Member

Choose a reason for hiding this comment

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

@wordpress/block-library is updated from npm. The changes applied to Gutenberg are released with the plugin release every two weeks and then published to npm.

@chipsnyder chipsnyder marked this pull request as ready for review January 10, 2020 15:00
@koke koke added [Feature] Shortcodes Related to shortcode functionality Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Jan 13, 2020
@chipsnyder chipsnyder changed the base branch from master to rnmobile/master January 13, 2020 21:19
@chipsnyder chipsnyder changed the base branch from rnmobile/master to master January 13, 2020 21:19
@chipsnyder chipsnyder changed the base branch from master to rnmobile/master January 14, 2020 13:39
@chipsnyder chipsnyder changed the base branch from rnmobile/master to master January 14, 2020 13:39
Copy link
Contributor

@koke koke left a comment

Choose a reason for hiding this comment

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

I tested this and it works fine now, but I'm worried that this could get confusing. The JSON file is named block.json, and the JS file imports block.json, but the PHP imports shortcode.json.

I'm going to approve this one, but we might want to consider an alternative

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Shortcodes Related to shortcode functionality Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants