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

Add Gutenberg 'Enable AMP' toggle #1275

Merged
merged 20 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@kienstra
Collaborator

kienstra commented Jul 21, 2018

This adds the 'Enable AMP' toggle to the Gutenberg editor.

  • This should have the initial enabled value, probably from AMP_Post_Meta_Box.
  • It could also have the notifications that the classic editor has, if needed.

Closes #1230

enable-amp

kienstra added some commits Jul 21, 2018

Begin Gutenberg 'Enabled AMP' toggle.
This still needs to have the initial toggled value
probably from AMP_Post_Meta_Box.
It could also have the notifications
that the classic editor has, if needed.
Refactor JS file to use a module patter.
Mainly taken from amp-block-validation.js.
Also, rename it to clarify that it's for Gutenberg.
Though don't use that name,
as the name probably won't remain after its Core merge.
Abstract logic to get enabled status into helper method.
This enables reusing it for the block toggle.
Also, add a PHPUnit test for this.
This is mainly copied from test_render_status().
Add assertions for inline script.
Include the variable ampBlockEditorToggle.
And defaultStatus.
Improve documentation in amp-block-editor-toggle.js.
Add documentation for module.data.
And other functions, like onAmpChange() and getEnabledStatus().
@westonruter

This comment has been minimized.

Member

westonruter commented Jul 24, 2018

It could also have the notifications that the classic editor has, if needed.

Meaning the notices shown when expanding the toggle to edit? Yes, these should be shown as well. In Gutenberg perhaps these notices would be shown unconditionally since there wouldn't be any toggle to expand. So this will require exporting $errors from PHP to JS as found here:

https://github.com/Automattic/amp-wp/blob/e3d036c2db60b215636a2b665414b0e599e9ad97/includes/admin/class-amp-post-meta-box.php#L187-L203

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 25, 2018

Notices

Hi @westonruter,

Meaning the notices shown when expanding the toggle to edit?

Yes, that's what I had in mind. Good point that they would have to be shown without the toggle to expand.

@@ -0,0 +1,121 @@
/**

This comment has been minimized.

@westonruter

westonruter Jul 25, 2018

Member

Ideally this file would be converted into a module similar to all of the scripts for blocks in https://github.com/Automattic/amp-wp/tree/develop/blocks

This needs to be done for https://github.com/Automattic/amp-wp/blob/develop/assets/js/amp-block-validation.js and https://github.com/Automattic/amp-wp/blob/develop/assets/js/amp-editor-blocks.js as well. This would allow for being able to use Babel to handle i18n and it would allow for the scripts to all be concatenated into a single build script.

This comment has been minimized.

@kienstra

kienstra Jul 31, 2018

Collaborator

Sure, Issue #1298 now tracks bundling this amp-block-editor-toggle.js module with amp-block-validation.js. We could also add converting amp-editor-blocks.js to a module.

kienstra added some commits Jul 28, 2018

Take first step to enable compiling amp-block-editor-togge.js
As Weston suggested,
this script should be a module,
and we could use Babel for internationalization.
All of these scripts might eventually be in a single script.
But this only affects 1 script now.
Begin to convert AMP block editor toggle to a module.
There will probably be styling issues to correct.
Eventually, we might be able to compile all of the block-related
JS scripts into one.
Improve documentation, simplify conditional.
The ternary conditional doesn't really need a ().
Also, improve documentation,
including changing 'Gutenberg' to 'block editor.'
Localize values for amp-block-editor-toggle.js
Before, this used wp_add_inline_script().
But that doesn't seem to work well with this module,
as it had used a variable in the global scope.
This simply adds a global variable, wpAmpEditor
And other AMP Gutenberg scripts could possibly use this.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 30, 2018

Hiding Toggle, Displaying Notice

The next step is to apply something like the Classic Editor UI. If AMP is disabled via errors, you shouldn't be able to select "Enabled."

errors-amp-disabled

@@ -4,7 +4,8 @@ const path = require( 'path' );
module.exports = {
entry: {
'./assets/js/amp-blocks-compiled': './blocks/index.js'
'./assets/js/amp-blocks-compiled': './blocks/index.js',
'./assets/js/amp-block-editor-toggle-compiled': './assets/src/amp-block-editor-toggle.js'

This comment has been minimized.

@westonruter

westonruter Jul 30, 2018

Member

I think these two two should be combined into a single entry point. In other words, ./blocks/index.js could just import the module for the block editor toggle, and then there wouldn't be a need for amp-block-editor-toggle-compiled.js

This comment has been minimized.

@kienstra

kienstra Jul 30, 2018

Collaborator

Good idea, I'll apply that.

This comment has been minimized.

@kienstra

kienstra Jul 31, 2018

Collaborator

Could this instead combine amp-block-editor-toggle-compiled.js with any future modules, like amp-block-validation.js (when that's converted to a module)?

It looks like assets/js/amp-blocks-compiled.js is only enqueued if amp_is_canonical().

This comment has been minimized.

@westonruter

westonruter Jul 31, 2018

Member

@kienstra Yes, amp-block-validation.js should also be converted to a module and included in this bundle. 100%

Good point on amp-blocks-compiled.js. Maybe that would make sense to be a separate bundle then. Either that, or there could be an arg passed for whether the blocks should be registered. But if not, then why include the file in the first place?

This comment has been minimized.

@westonruter

westonruter Jul 31, 2018

Member

So amp-block-validation.js and amp-block-editor-toggle-compiled.js should definitely be combined.

This comment has been minimized.

@westonruter

westonruter Jul 31, 2018

Member

Sounds good. 👍

This comment has been minimized.

@westonruter

westonruter Jul 31, 2018

Member

I know that @hellofromtonya is currently working on amp-block-validation.js so it would be good to not step on her toes 😄

This comment has been minimized.

@westonruter

westonruter Jul 31, 2018

Member

@kienstra One more thing for that issue you'll create: please add to it the need to generate source maps.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 31, 2018

Collaborator

@westonruter, @kienstra and I synced up in slack. He'll open the issue. Once we resolve the issues in amp-block-validation.js, then we can green light the task.

This comment has been minimized.

@kienstra

kienstra Jul 31, 2018

Collaborator

Issue #1298 now tracks bundling amp-block-editor-toggle.js with amp-block-validation.js. It also mentions that it's probably best to wait for #1293.

Move error messages into function, localize them into script
On Weston's suggestion,
in order to display notices in Gutenberg,
like they display in the Classic Editor.
@todo: handle <a> elements, as they are escaped.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 30, 2018

Current Notice Display

Here's an example of how the Notice looks so far when AMP can't be enabled:

amp-notice

@westonruter

This comment has been minimized.

Member

westonruter commented Jul 31, 2018

That looks good 👍

Output Notices with links in the block editor.
Move the message function into another helper,
get_raw_error_messages().
This has the text of the message and the href.
And it allows the block editor script
to construct the <a>.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 31, 2018

The Notice in the block editor now has the same links as the Classic Editor:

links-like-classic-editor

And here's the display in the unlikely case that there are 2 messages in a Notice:
two-messages-notice

kienstra added some commits Jul 31, 2018

Improve documentation for Notices
And more comments descrbing how text and <a>
is contructed in the Notice.
Use wildcards in .eslintignore and .jshintignore
This might not be the right syntax
But it'll prvent having to add a new line
for all of the compiled JS files.
Only call gutenberg_get_jed_locale_data() if it exists.
When Gutenberg is merged,
the gutenberg_ prefix might be removed from functions.
So only call this function if it exists.
Also, simplify call to
get_classic_editor_error_messages().
@@ -39,36 +39,7 @@
</fieldset>
<?php else : ?>
<div class="inline notice notice-warning notice-alt">
<p>
<?php
$error_messages = array();

This comment has been minimized.

@kienstra

kienstra Jul 31, 2018

Collaborator

This section is moved to get_raw_error_messages().

Simply pass value to function without storing in variable
$raw_messages is not needed.
Instead, simply pass it to the funtion.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Aug 1, 2018

Request For Code Review

Hi @westonruter,
Could you please review this PR?

Notice with a link:
notice-with-link

Notice without a link:
notice-without-a-link

You know this, but for this PR's record, please run this to compile the script:
npm run dev

@kienstra kienstra requested a review from westonruter Aug 1, 2018

@kienstra kienstra changed the title from [WIP] Add Gutenberg 'Enable AMP' toggle to Add Gutenberg 'Enable AMP' toggle Aug 1, 2018

* @param array $raw_error_messages The raw error messages, possibly with URLs.
* @return array $error_messages The error message(s), as an array of strings.
*/
public function get_classic_editor_error_messages( $raw_error_messages ) {

This comment has been minimized.

@westonruter

westonruter Aug 1, 2018

Member

There is duplication between PHP here and the JS in PluginPostStatusInfo. I appreciate the concern to not dangerouslySetInnerHTML but since the error messages are coming from a trusted source, I think it would be better to do so in this case. Even in Gutenberg, for example, there is a RawHTML component for this purpose. This component is publicly available already in Gutenberg via wp.element.RawHTML.

This comment has been minimized.

@kienstra

kienstra Aug 1, 2018

Collaborator

Thanks, I'm working on this now.

This comment has been minimized.

@kienstra

kienstra Aug 1, 2018

Collaborator

These commits simplify the error messages to use RawHTML like you suggested.

kienstra and others added some commits Aug 1, 2018

Simplify error messages to use RawHTML.
As Weston suggested,
it's possible to construct elements using RawHTML.
So this simplifies how the messages are output,
to just use a string.
Fix test_get_error_messages() to apply latest change.
The tested method had several changes,
so update this for them.
Fix error raised when post resource lacks meta
I noticed the error when adding a new Jetpack Testimonial post.

Also switch to from classic functions to arrow functions.
@westonruter

@kienstra You've written some beautiful JS here. Great work.

@westonruter westonruter added this to the v1.0 milestone Aug 1, 2018

@westonruter westonruter merged commit 27406eb into develop Aug 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/editor-toggle branch Aug 1, 2018

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Aug 1, 2018

Thanks, @westonruter!

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