Apps: Add Full Site Editing Plugin and Theme #32355

Copons · 2019-04-17T12:11:46Z

Changes proposed in this Pull Request

Add a new full-site-editing app containing full-site-editing-plugin and blank-theme.
For demonstration purposes, add the Page Content preview block to the plugin.

Testing instructions

From the Calypso root, try building the plugin with npx lerna run build --scope='@automattic/full-site-editing'.
Move the full-site-editing-plugin folder (or symlink it) into /wp-content/plugins of any WordPress install.
Open its dashboard, and enable the Full Site Editing plugin.
Open the block editor, and insert the "Page Content Preview" block.
Pick a page (you need to have pages first!), and check that it gets previewed inside the block.

Note: on WPCOM we need a file in the plugins root to load it.
It's enough to just use it to require the actual plugin php file, e.g.

require_once( 'full-site-editing-plugin/full-site-editing-plugin.php' );

matticbot · 2019-04-17T12:11:48Z

Test live: https://calypso.live/?branch=add/full-site-editing-plugin-app

matticbot · 2019-04-17T12:21:01Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copons · 2019-04-17T15:31:48Z

Pinging @sirreal, @simison, and @mmtr for some 👀 in case I've overlooked something!

dmsnell · 2019-04-17T15:38:31Z

apps/full-site-editing/full-site-editing-plugin/blocks/page-content/index.js

+	withSelect( select => ( {
+		pages: select( 'core' ).getEntityRecords( 'postType', 'page', {
+			per_page: -1,
+		} ),


core already makes this mistake, but this line loads in every page on a site. if you load this in the FieldGuide it will download over 40 MB of data before returning pages due to the middleware which follows paged results

I know, I planned to replace this whole thing with a modified version of URLInput (inspired by your plugin), but decided to avoid doing so now, and improve on it in a follow up PR if/when the dev env gets approved.

dmsnell · 2019-04-17T15:39:37Z

apps/full-site-editing/full-site-editing-plugin/blocks/page-content/index.js

+	const selectOptions = [
+		{ label: '', value: undefined },
+		...map( pages, page => ( {
+			label: `[${ page.id }] ${ page.title.rendered }`,


why are we showing the page id?

Because it's useful to me in testing 😛

dmsnell · 2019-04-17T15:42:35Z

apps/full-site-editing/full-site-editing-plugin/blocks/page-content/index.js

+						dangerouslySetInnerHTML={ {
+							__html: get( selectedPage, 'content.rendered' ),
+						} }
+					/>


there's a RawHTML component we should be able to use here. you can see where I've used it in my prototype.

Copons · 2019-04-17T15:43:31Z

@dmsnell appreciate the comments, but let me stop you right there: that plugin is not final, but just to have something to test!
This PR is first and foremost to have a dev environment to work in. :)

dmsnell · 2019-04-17T15:44:46Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+	function __construct() {
+		$this->register_script_and_style();
+
+		add_action( 'init', [ $this, 'register_blocks' ], 100 );


I love this, but if we're going to send this to Jetpack then we need to make sure we don't use the array shorthand syntax

add_action( 'init', array( $this, 'register_blocks' ), 100 );

Dang!
I was sure JP was on PHP 7 nowadays, but thinking about it, I guess I got confused with WPCOM.

Jetpack is soon able to bump PHP requirement and PHP 5.4 might already support this? Are you targeting Jetpack 7.3 or 7.4 release? (@kraftbj and @jeherve prolly know more about PHP support on those)

tl;dr: ff you are aiming to launch in Jetpack 7.3 (or even 7.4), it's best to stick to PHP 5.2 compatibility.

Jetpack aims to follow the same requirements as WordPress core, and supports Core n-1. For example, right now Core is 5.1, so Jetpack's minimum WP core supported version is 5.0.

WordPress 5.2, scheduled to be released in a couple of weeks, will up the PHP requirements to PHP 5.6.

As a result, we'll be able to start using PHP 5.6 syntax as soon as WordPress 5.3 ships, since at that point Jetpack will support WordPress 5.2.

Core is theoretically moving to PHP 5.6 this month and Jetpack follows suit so hopefully this will be moot in the not-too-distant future

We are not targeting any particular release at this stage, this is mainly for internal development and milestones. Also at this point it's not clear if and how we'll incorporate this into Jetpack.

dmsnell · 2019-04-17T15:45:20Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+		wp_register_script(
+			'wpcom-full-site-editing-script',
+			plugins_url( 'dist/full-site-editing-plugin.js', __FILE__ ),
+			[],


array( 'wp-block-editor', 'wp-blocks', 'wp-components', 'wp-compose', 'wp-data', 'wp-element', 'wp-i18n', )

dmsnell · 2019-04-17T15:45:29Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+		wp_register_style(
+			'wpcom-full-site-editing-style',
+			plugins_url( 'dist/full-site-editing-plugin.css', __FILE__ ),
+			[],


dmsnell · 2019-04-17T15:47:51Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+		register_block_type( 'wpcom/page-content', [
+			'editor_script' => 'wpcom-full-site-editing-script',
+			'editor_style' => 'wpcom-full-site-editing-style',
+			'render_callback' => [ $this, 'render_page_content_block' ],


array( $this, 'render_page_content_block )

dmsnell · 2019-04-17T15:51:53Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+		}
+		$post = get_post( $attributes[ 'pageId' ] );
+		setup_postdata( $post );
+		$the_content = get_the_content();


I'm pretty sure we don't want get_the_content() here

first of all it doesn't run the_content filter, so we at least would favor the_content() instead I think
second of all, can we confirm that it runs do_blocks() on the content? I think it should but just in case we want to confirm by making sure something like a void block gets rendered as it should

do_blocks is run within the the_content filter but the_content() prints the content so we'd either need to do some output buffering or just something like:

$the_content = apply_filters( 'the_content', $post->post_content );

dmsnell · 2019-04-17T15:57:41Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+	}
+}
+
+new WPCOM_Full_Site_Editing();


while it doesn't have to end up as a singleton, we want to ensure that we don't accidentally invoke register_block_type multiple times.

class WPCOM_Full_Site_Editing { static $has_already_initialized = false; function __construct() { if ( self::$has_already_initialized ) { return; } self::$has_already_initialized = true; … } … }

the more common method around these parts is to create a singleton class.

sirreal · 2019-04-17T15:58:49Z

Pinging @sirreal […] for some 👀 in case I've overlooked something!

Thanks for the ping. I'll be away and won't be able to get to this for at least a week.

Copons · 2019-04-17T16:14:16Z

@dmsnell I've fixed all your suggestions except:

How we fetch pages (#). I've removed the -1, so that now it only fetches 10 pages, and it doesn't kill big sites. The idea is to eventually replace it with an autosuggested input field like URLInput (but with some changes. Though, it's out of this PR scope, so I'll leave it for a follow up if/when this gets approved and merged.
How the Page Content preview block renders (#). the callback wasn't meant to be there (my bad, I've copypasted more than needed from my experiment). In fact, the block isn't supposed to render anything, but just preview a page content in the editor.

simison · 2019-04-17T20:22:47Z

apps/full-site-editing/package.json

+		"dev:theme": "npx lerna run theme",
+		"build:theme": "NODE_ENV=production npx lerna run theme"
+	},
+	"devDependencies": {


Note this comment about devDependencies:

https://github.com/Automattic/wp-calypso/pull/32294/files#r276148646

It's mentioned in passing also in https://github.com/Automattic/wp-calypso/blob/master/docs/monorepo.md

It can cause trouble like this: #32383

simison · 2019-04-17T20:37:07Z

apps/full-site-editing/full-site-editing-plugin/blocks/page-content/index.js

+import { compose, withState } from '@wordpress/compose';
+import { withSelect } from '@wordpress/data';
+import { Fragment, RawHTML } from '@wordpress/element';
+import { __ } from '@wordpress/i18n';


Note that these translation functions won't work out of the box on wpcom without some extra setup; I'd get in touch with Team global about it.

I know, and I was wondering how to handle the i18n.
Would it make sense to use i18n-calypso instead?

I'd assume it depends where these scripts are loaded (Jetpack/wpcom/Calypso) and how that works with files that are loaded from CDN URLs like widgets.wp.com, if you use those. i18n gets complicated really fast. :-)

@yoavf is working on making wp.i18n work for Jetpack blocks on wpcom — you might be able to piggyback on that work. At any rate, I'd check with Team global.

I don't think you should worry about it for this PR tho. It's just something I recommend to start figuring out early on.

simison · 2019-04-17T20:39:10Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+				'wp-compose',
+				'wp-data',
+				'wp-element',
+				'wp-i18n',


Calypso-build tool should produce a list of these dependencies a s *.deps.json file that you can read and use here without needing to manually define these.

mmtr

Nice work @Copons!

Left some comments regarding the building process, but didn't test manually.

mmtr · 2019-04-18T03:01:05Z

apps/full-site-editing/README.md

+There are 4 scripts that perform roughly the same tasks, but on different folders and environments:
+
+```
+npm run dev:plugin


Note that you can also run these scripts from the Calypso root dir using lerna and the scope param: npx lerna run dev:plugin --scope='@automattic/full-site-editing'.

Actually, this will be the recommended way included on the monorepo docs @simison is currently working on #32137: https://github.com/Automattic/wp-calypso/blob/42496d0976a6ea54fd4204daaedbdd12615141c4/apps/README.md

I'm not sure if there are any negative consiquences running npm commands (especially npm install in /apps/** subfolders instead of running them from the root using scope.

I didn't add running npm scripts from subfolders to docs just because I didn't consider someone would want to do that. :-) I don't really want to recommend the way or the other without understanding Lerna better.

Maybe it'll skip some pre* commands and dependencies don't get hoisted properly between this folder and root node_modules? Can that lead to inconsistent versions depending what you run? Since you use ^ range in semver, it's prolly not a concern. cc @blowery who knows this stuff better.

All that said, in general it definitely is useful to stay consistent with how we use the monorepo.

mmtr · 2019-04-18T03:05:17Z

apps/full-site-editing/package-lock.json

@@ -0,0 +1,2572 @@
+{


I don't think this file is needed

It's automatically generated when installing dependencies via CLI.
Even though it messes with the LOC diff, it's useful in case of package versions conflicts and whatnot.

You can run npm i --no-package-lock to not generate lockfiles. https://docs.npmjs.com/cli/install

If it's useful, you should commit it to the repo and use npm ci. :-)

It's automatically generated when installing dependencies via CLI.

Did this happen when installing dependencies from the Calypso root or within the package folder? If it's the later, I was thinking that it is not necessary to install them because the package is actually not used in Calypso and we're not going to use those dependencies anywhere.

But giving a second thought, maybe the build generation will fail if it cannot find the dependencies that are not externalized. So maybe we really need to install the dependencies?

mmtr · 2019-04-18T03:10:45Z

apps/full-site-editing/package.json

+	"homepage": "https://github.com/Automattic/wp-calypso",
+	"scripts": {
+		"plugin": "webpack --folder='plugin' --env.WP='true'",
+		"dev:plugin": "npx lerna run plugin",


Does npm run plugin produce a different result? We don't use npm lerna run on the existing o2-blocks and wpcom-block-editor apps, but maybe you found some issues because you ran the commands inside the full-site-editing dir?

Right, I'm almost sure it gave me some issues, but I've done a lot of changes on these scripts and tbh they might have been caused by something else.
I'll try converting this with the whole npx lerna run from the root, and see what happens.

simison · 2019-04-18T08:37:10Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+		wp_register_script(
+			'wpcom-full-site-editing-script',
+			plugins_url( 'dist/full-site-editing-plugin.js', __FILE__ ),
+			array(


You could add wp-polyfill dependency here since the build doesn't take care of that for you (unlike you might expect in Calypso). Pretty much every other of your dependencies already relies on it implicitly but it's always nice to be explicit. :-)

It won't really matter with editor side script but if you ever add view side script, it becomes pretty important.

simison · 2019-04-18T08:56:44Z

apps/full-site-editing/babel.config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	extends: require.resolve( '@automattic/calypso-build/babel.config.js' ),
+	presets: [ require( '@wordpress/babel-preset-default' ) ],


Nit; @wordpress/default will work because of how Babel resolves these names.

simison · 2019-04-18T09:03:47Z

apps/full-site-editing/babel.config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	extends: require.resolve( '@automattic/calypso-build/babel.config.js' ),
+	presets: [ require( '@wordpress/babel-preset-default' ) ],


You might want to consider including only relevant bits shipped in Calypso-build for this purpose:

presets: [ require( '@automattic/calypso-build/babel/wordpress-element' ) ],

The @wordpress/default is little more extensive.

Copons · 2019-04-18T11:35:22Z

All right, I think I have addressed everything!

Most important change is that this (both the plugin and the theme at the same time) is now runnable via these scripts from the Calypso root:

npx lerna run dev --scope='@automattic/full-site-editing'
npx lerna run build --scope='@automattic/full-site-editing'

simison · 2019-04-18T12:59:26Z

I'm curious what's your plan with .php files workflow-wise?

vindl · 2019-04-18T13:25:14Z

apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php

+		}
+		self::$initialized = true;
+
+		$this->register_script_and_style();


Suggested change

$this->register_script_and_style();

add_action( 'enqueue_block_editor_assets', array( $this, 'register_script_and_style' ) );

Otherwise there is a PHP notice about running wp_register_script too early. We'll probably want to have a separate method down the road for non-editor stuff that hooks into wp_enqueue_scripts for example, but this works for now.

I think enqueue_block_editor_assets is supposed to be used for scripts that extend the editor itself, not blocks.

Blocks just need a wp_register_script (which doesn't enqueue the script), and define as dependency in the editor_script option of register_block_type.
When the block type is created, the script gets automatically enqueued (if still needed).

Although, I should change my error reporting and check that notice. 🤔

vindl · 2019-04-18T13:43:10Z

apps/full-site-editing/full-site-editing-plugin/blocks/page-content/index.js

+} );
+
+registerBlockType( 'wpcom/page-content', {
+	title: __( 'Page Content Preview' ),


I'm thinking whether this should be Post Content Preview? 🤔 In the context of wp_template CPT, we'd probably want to allow previewing arbitrary post types when viewing the whole template in the editor. So maybe "post" would be more generic term, or just "Content Preview"? (perfectly fine to leave it scoped down to just pages in this PR)

I'll keep it like this for now, but I 100% agree that, especially in this context, we should stop mentioning "posts", and just go as generic as possible.

"Content Preview" could sound a bit confusing (content of what?), but "Page (Content) Preview" is unfortunate as well (why just pages?).

vindl · 2019-04-18T13:50:15Z

Great work @Copons! ✨

I gave this a spin and it works fine in my local WP setup. However, I wasn't able to get it to show on WP.com - no noticeable errors in the console and logs, and I couldn't track down the root cause so far.

Given that this will be our primary target for deployment (Simple sites) I think we should try to figure this out before merging.

Copons · 2019-04-18T14:50:08Z

@vindl Good catch, I should have run the plugin on my sandbox too!

As it turned out, on my local plugin I'm using the Gutenberg plugin on a version more recent than Core (although not the most recent).
That version moved some components from @wordpress/editor to the new @wordpress/block-editor package.
BlockControls was one of those components.

In the plugin I imported it from @wordpress/block-editor, adding it as dependency for the block script.
Though, since that package doesn't exist in Core (and on WPCOM) yet, on WPCOM the block missed a dependency and wasn't loaded altogether.

In 0b90bd9 I've replaced @wordpress/block-editor with @wordpress/editor, and it now works both on WPCOM and on my local install (Gutenberg 5.5).

Copons · 2019-04-18T17:28:11Z

I'm curious what's your plan with .php files workflow-wise?

@simison Do you mean regarding linting, etc.?
I don't have a good answer for you, I'm not familiar with how that works with PHP.

If you talk about how to move these files in their final location, current plan would be to make Circle create artifacts (in a follow up), and then send them to the WPCOM's plugins and themes folders with a script (like #32163).

vindl

@Copons I think this is a great starting point! I'm in favor of landing this as is, so we can start working in parallel on some of the improvements. I'll start writing down issues for some of the major areas to follow-up on (i18n, CircleCI artifacts & sandbox dev flow etc)

mmtr

Thanks for the changes @Copons. And good job again!

Copons · 2019-04-19T10:52:28Z

Thank y'all for the reviews! 🙇

Thanks for the changes @Copons. And good job again!

Thanks @mmtr, but I've basically copied everything off your work on wpcom-block-editor 😄

simison · 2019-04-19T11:05:06Z

apps/full-site-editing/README.md

+Compiles both the theme and the plugin, and watches for changes.
+
+- `npx lerna run build --scope='@automattic/full-site-editing'`<br>
+Compiles and minifies for production both the theme and the plugin.


Doesn't matter much but useful to know — you can do the same by intending the 2nd line, instead of adding br:

- `npx lerna run build --scope='@automattic/full-site-editing'` Compiles and minifies for production both the theme and the plugin.

Copons added 2 commits April 17, 2019 12:18

Initial commit

0870f08

Add the Page Content preview block

56dde5d

Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Gutenberg Working towards full integration with Gutenberg [Goal] Full Site Editing labels Apr 17, 2019

Copons requested a review from a team April 17, 2019 12:11

Copons self-assigned this Apr 17, 2019

Make a distinction between external and wordpress deps

cd506d3

dmsnell reviewed Apr 17, 2019

View reviewed changes

The customary post-dmsnell-review fixes

39c243b

simison reviewed Apr 17, 2019

View reviewed changes

mmtr reviewed Apr 18, 2019

View reviewed changes

simison reviewed Apr 18, 2019

View reviewed changes

Copons added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 18, 2019

Copons added 3 commits April 18, 2019 12:07

Simplifies the build scripts

608975d

Replace devDeps

e8960a6

Improve deps and rtl handling

1564864

Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 18, 2019

vindl reviewed Apr 18, 2019

View reviewed changes

Fix Guten packages version incompatibility

0b90bd9

vindl approved these changes Apr 18, 2019

View reviewed changes

vindl added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 18, 2019

mmtr approved these changes Apr 18, 2019

View reviewed changes

Replace WPCOM namespace with A8C

b2a3cb0

Copons merged commit 6aa997f into master Apr 19, 2019

Copons deleted the add/full-site-editing-plugin-app branch April 19, 2019 11:02

matticbot removed the [Status] Ready to Merge label Apr 19, 2019

simison reviewed Apr 19, 2019

View reviewed changes

simison mentioned this pull request Apr 19, 2019

Calypso Build Package: Add calypso-build bin shorthand #32295

Merged

2 tasks

vindl mentioned this pull request Apr 20, 2019

Full Site Editing: change script loading order #32448

Merged

Copons added a commit that referenced this pull request Apr 25, 2019

Revert workflow to #32355 and update Circle job

b96bdb4

Copons mentioned this pull request Apr 25, 2019

Full Site Editing: Fix PHP files missing in development workflow #32571

Merged

	$this->register_script_and_style();
	add_action( 'enqueue_block_editor_assets', array( $this, 'register_script_and_style' ) );

Apps: Add Full Site Editing Plugin and Theme #32355

Apps: Add Full Site Editing Plugin and Theme #32355

Conversation

Copons commented Apr 17, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Apr 17, 2019

matticbot commented Apr 17, 2019

Copons commented Apr 17, 2019

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

Copons commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

jeherve Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Apr 17, 2019 • edited Loading

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

sirreal commented Apr 17, 2019

Copons commented Apr 17, 2019

simison Apr 17, 2019 • edited Loading

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

mmtr left a comment

Choose a reason for hiding this comment

mmtr Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

simison Apr 18, 2019 • edited Loading

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

mmtr Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copons commented Apr 18, 2019

simison commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindl Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindl commented Apr 18, 2019

Copons commented Apr 18, 2019

Copons commented Apr 18, 2019 • edited Loading

vindl left a comment • edited Loading

Choose a reason for hiding this comment

mmtr left a comment

Choose a reason for hiding this comment

Copons commented Apr 19, 2019

simison Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

Copons commented Apr 17, 2019 •

edited

Loading

simison Apr 17, 2019 •

edited

Loading

jeherve Apr 17, 2019 •

edited

Loading

dmsnell Apr 17, 2019 •

edited

Loading

simison Apr 17, 2019 •

edited

Loading

mmtr Apr 18, 2019 •

edited

Loading

simison Apr 18, 2019 •

edited

Loading

mmtr Apr 18, 2019 •

edited

Loading

simison Apr 18, 2019 •

edited

Loading

vindl Apr 18, 2019 •

edited

Loading

Copons commented Apr 18, 2019 •

edited

Loading

vindl left a comment •

edited

Loading

simison Apr 19, 2019 •

edited

Loading