[Gutenberg] 1008: Extend core blocks to add AMP-specific functionality. #1026
Conversation
|
Oh, I wasn't aware of the |
|
Another AMP attribute that could be good to add support for is |
|
For |
| wp_enqueue_script( | ||
| 'amp-editor-blocks', | ||
| amp_get_asset_url( 'js/amp-editor-blocks.js' ), | ||
| array( 'underscore', 'wp-hooks' ), |
There was a problem hiding this comment.
I think this should include amp-runtime
assets/js/amp-editor-blocks.js
Outdated
| ); | ||
| return [ | ||
| inspectorControls, | ||
| el( BlockEdit, _.assign( { key: 'original' }, props ) ), |
There was a problem hiding this comment.
The following is what I have in mind, though it is pretty broken in my testing:
diff --git a/assets/js/amp-editor-blocks.js b/assets/js/amp-editor-blocks.js
index c4d826a..c1ad182 100644
--- a/assets/js/amp-editor-blocks.js
+++ b/assets/js/amp-editor-blocks.js
@@ -82,9 +82,14 @@ var ampEditorBlocks = ( function() {
} )
)
);
+
+ const original = el( BlockEdit, _.assign( { key: 'original' }, props ) );
+
return [
inspectorControls,
- el( BlockEdit, _.assign( { key: 'original' }, props ) ),
+ wp.element.createElement( 'amp-layout',
+ { layout: attributes.ampLayout, width: 1, height: 1, children: original }
+ ),
];
};
};
|
I've also raised this in the amp-wp Slack channel:
https://amphtml.slack.com/archives/C4J8QHGBZ/p1521501878000295 |
|
Thanks for looking into the PR and for the comments, @westonruter, very helpful thoughts! On Are you thinking that maybe we should add Thoughts? |
|
In case of AMP elemenets and |
… attributs instead of amp-layout (commented out ATM).
Great points. One idea is that there could be an In other words, given a As I noted in #937 (comment) I'm not totally familiar with the intricacies of the AMP layout system, so take my input with a grain of salt.
This is partially implemented in #937 but it is not yet merged. You could copy the relevant code and put it into this PR. In particular, that PR is specifically looking at adding |
|
Note on TBH, I'd not worry about the |
|
@westonruter Picked up the PR today to continue work here, was looking at Also, started looking into adding Layout attribute to embeds and was wondering what would be a good approach -- the embed methods don't have access to the attributes saved within Gutenberg nor their parent HTML element (which has the |
I'm looking at a video that I uploaded in my install and I'm seeing there is a {"filesize":24004120,
"mime_type":"video\/mp4",
"length":9,
"length_formatted":"0:09",
"width":1920,
"height":1080,
"fileformat":"mp4",
"dataformat":"quicktime",
"audio":{"dataformat":"mp4","codec":"ISO\/IEC 14496-3 AAC","sample_rate":48000,"channels":1,"bits_per_sample":16,"lossless":false,"channelmode":"mono"},
"created_timestamp":1512335233}So at least for this video there is indeed
So the problem is that a Twitter block, could look like this: <!-- wp:core-embed/twitter {"url":"https://twitter.com/westonruter/status/988577526741061632","type":"rich","providerNameSlug":"twitter"} -->
<figure class="wp-block-embed-twitter wp-block-embed is-type-rich is-provider-twitter">
https://twitter.com/westonruter/status/988577526741061632
</figure>
<!-- /wp:core-embed/twitter -->And get output as this: <figure class="wp-block-embed-twitter wp-block-embed is-type-rich is-provider-twitter">
<amp-twitter data-tweetid="988577526741061632" layout="responsive" width="600" height="480"></amp-twitter>
</figure>And if Here's an idea: what if you go ahead and add a <figure data-amp-layout="fixed" class="wp-block-embed-twitter wp-block-embed is-type-rich is-provider-twitter">
<amp-twitter data-tweetid="988577526741061632" layout="responsive" width="600" height="480"></amp-twitter>
</figure>This could get post-processed by a sanitizer into: <figure class="wp-block-embed-twitter wp-block-embed is-type-rich is-provider-twitter">
<amp-twitter data-tweetid="988577526741061632" layout="fixed" width="600" height="480"></amp-twitter>
</figure>The question then in my mind is whether that “inheritance override” behavior should be limited to |
| * | ||
| * Modifies elements created as blocks to match the blocks' AMP-specific configuration. | ||
| */ | ||
| class AMP_Block_Sanitizer extends AMP_Base_Sanitizer { |
There was a problem hiding this comment.
@westonruter That's still WIP but if you happen to have time, could you check if this approach for embeds seems to make sense?
There was a problem hiding this comment.
I think you're on the right track!
assets/js/amp-editor-blocks.js
Outdated
| } else { | ||
| width = 600; | ||
| } | ||
| // @todo Should we try to add width and height according to the layout? |
There was a problem hiding this comment.
@westonruter Another issue with AMP Layout is rendering in Gutenberg -- in case of embeds the content of the block in the editor is replaced once the content (e.g. from Youtube) has been loaded and the added attributes would get removed. The point being that currently looks like for trying to match the layout behavior in Gutenberg editor, possibly the whole edit method of the core block should be replaced. Or perhaps the width and height could be modified later, not sure. Also, since the block within Gutenberg is quite separated from the outer divs of the content, then likely it would still not show a matching result. Thoughts on showing the layout changes in Gutenberg?
| public function override_gallery( $html, $attributes ) { | ||
| public function maybe_override_gallery( $html, $attributes ) { | ||
| if ( isset( $attributes['amp-carousel'] ) && false === filter_var( $attributes['amp-carousel'], FILTER_VALIDATE_BOOLEAN ) ) { | ||
| return $html; |
There was a problem hiding this comment.
@westonruter Added editor support for making displaying gallery shortcode as amp-carousel optional by adding a shortcode attribute amp-carousel. The attribute is hidden from the user in editor view, and the Inspector Control is displayed only if [gallery is found in the shortcode text, seems to be working as expected.
Do you think this approach makes sense? Thoughts?
(Logic added within fea3434)
There was a problem hiding this comment.
Yes, I think that makes sense and it seems to work great.
At first I was thinking that carousel option should also be made available to the Gallery block in addition to the gallery shortcode, but maybe that's something that will be coming in Gutenberg as a separate block type in its own right.
There was a problem hiding this comment.
Do you think that just using amp-carousel attribute within post content when using Classic Editor for gallery shortcode could be a fix for #1065, too?
There was a problem hiding this comment.
Wouldn't it be the case anyway here? It's just that at the moment the user would have to manually add the amp-carousel attribute to the [gallery] to achieve that with the changes here?
There was a problem hiding this comment.
Yes, it would, just thinking if this should be the official fix that closes #1065, too, or if an alternative approach would be needed for that.
miina
changed the title
|
@kienstra Thank you for testing. Guess the PR should be ready for review. |
|
@miina I tried adding
Additionally, the window froze, apparently due to an infinite loop until finally an error was thrown:
It seems to be specifically a problem when setting the layout to |
|
@miina also, as we chatted about today, if I add a
So I think we need to save out the |
|
@westonruter: One additional note, this appears actually on Can you also see this error or is it just me? |
* Use _.extend() consistently instead of _.assign() * Use "Default" instead "None" for non-set layout. * Just return in each loop instead of returning true. * Use wp.editor.InspectorControls instead of deprecated wp.blocks.InspectorControls.
assets/js/amp-editor-blocks.js
Outdated
|
|
||
| _.each( component.data.ampLayoutOptions, function( option ) { | ||
| // Exclude options from layout that are not supported. | ||
| if ( 'core/image' === blockName || 'core/video' === blockName || 'core-embed/twitter' === blockName ) { |
There was a problem hiding this comment.
It may be easier to read if the logic in this each loop were broken up into isAvailable( blockName ) functions that are defined with each of the ampLayoutOptions.
There was a problem hiding this comment.
Changed within dbc1c7c, used notAvailable param for ampLayoutOptions, let me know if isAvailable param seems to make more sense.
assets/js/amp-editor-blocks.js
Outdated
| data: { | ||
| dynamicBlocks: [], | ||
| ampLayoutOptions: [ | ||
| { value: 'nodisplay', label: 'No Display' }, |
assets/js/amp-editor-blocks.js
Outdated
| ampAttributes[ 'data-amp-noloading' ] = attributes.ampNoLoading; | ||
| } | ||
|
|
||
| return _.assign( ampAttributes, props ); |
There was a problem hiding this comment.
I think we should consistently use _.extend instead of _.assign. The former is from Underscore and the latter is from lodash. Only underscore is marked as a dependency for this script. Interesting that the post edit screen loads both Underscore and lodash, with the latter doing noConflict.
assets/js/amp-editor-blocks.js
Outdated
| isSelected = props.isSelected, | ||
| name = props.name, | ||
| el = wp.element.createElement, | ||
| InspectorControls = wp.blocks.InspectorControls, |
There was a problem hiding this comment.
wp.blocks.InspectorControls is deprecated and will be removed from Gutenberg in 3.1. Please use wp.editor.InspectorControls instead.
assets/js/amp-editor-blocks.js
Outdated
|
|
||
| if ( attributes.ampLayout ) { | ||
| if ( 'core/image' === name ) { | ||
| component.setImageBlockLayoutAttributes( props, attributes.ampLayout, inspectorControls ); |
There was a problem hiding this comment.
Calling this here results in a warning:
Warning: Cannot update during an existing state transition (such as within
renderor another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved tocomponentWillMount.
There was a problem hiding this comment.
I think this should be done in some onChange callback somehow.
There was a problem hiding this comment.
Changed within dbc1c7c.
assets/js/amp-editor-blocks.js
Outdated
| // Exclude options from layout that are not supported. | ||
| if ( 'core/image' === blockName || 'core/video' === blockName || 'core-embed/twitter' === blockName ) { | ||
| if ( 'container' === option.value ) { | ||
| return true; |
There was a problem hiding this comment.
This might as well just return.
assets/js/amp-editor-blocks.js
Outdated
| */ | ||
| component.getLayoutOptions = function getLayoutOptions( blockName ) { | ||
| var layoutOptions = [ | ||
| { value: '', label: 'None' } |
There was a problem hiding this comment.
Probably “Default” is more clear than “None”, since the latter could be confused with “No Display”.
| public function override_gallery( $html, $attributes ) { | ||
| public function maybe_override_gallery( $html, $attributes ) { | ||
| if ( isset( $attributes['amp-carousel'] ) && false === filter_var( $attributes['amp-carousel'], FILTER_VALIDATE_BOOLEAN ) ) { | ||
| return $html; |
There was a problem hiding this comment.
Wouldn't it be the case anyway here? It's just that at the moment the user would have to manually add the amp-carousel attribute to the [gallery] to achieve that with the changes here?
| * @param array $amp_data Array of AMP attributes. | ||
| * @return array | ||
| */ | ||
| public function set_data_amp_attributes( $attributes, $amp_data ) { |
There was a problem hiding this comment.
I think instead of set_ this should be prefixed with filter_, since it's not actually setting any attributes on a node. That would align with filter_attributes method in the image sanitizer.
| * @param string $layout Layout. | ||
| * @return array New attributes. | ||
| */ | ||
| public function set_attachment_layout_attributes( $node, $new_attributes, $layout ) { |
There was a problem hiding this comment.
Same as above:
I think instead of
set_this should be prefixed withfilter_, since it's not actually setting any attributes on a node. That would align withfilter_attributesmethod in the image sanitizer.
But that gets tricky here because here there are side-effects on the parent node. So it's not so clear what is the best to do.
There was a problem hiding this comment.
Changed this to filter_ within dbc1c7c since even with setting the parent node attributes it seems to be more consistent. One option could be to separate setting the parent node attributes from the filtering method into a separate method, would that be a more clear / better alternative? Thoughts?
assets/js/amp-editor-blocks.js
Outdated
| } | ||
| } | ||
|
|
||
| // Return original. |
There was a problem hiding this comment.
Is this this the original or copied?
assets/js/amp-editor-blocks.js
Outdated
| break; | ||
|
|
||
| case 'nodisplay': | ||
| return [ |
There was a problem hiding this comment.
I don't see where this return value is ever used?
| * @return mixed Modified array. | ||
| */ | ||
| public function whitelist_layout_in_wp_kses_allowed_html( $context ) { | ||
| foreach ( $context as $tag ) { |
There was a problem hiding this comment.
I don't think this is working. At least it needs &$tag to ensure the array modifications get set into $context.
There was a problem hiding this comment.
That was not working, indeed. Changed within dbc1c7c.
| * @param array $context Array of contexts. | ||
| * @return mixed Modified array. | ||
| */ | ||
| public function whitelist_layout_in_wp_kses_allowed_html( $context ) { |
There was a problem hiding this comment.
I think that $context should be $tags.
| public function set_attachment_layout_attributes( $node, $new_attributes, $layout ) { | ||
|
|
||
| // If either height or width is missing, try to get these from original file. | ||
| if ( empty( $new_attributes['width'] ) || empty( $new_attributes['height'] ) ) { |
There was a problem hiding this comment.
Shouldn't the width and height discovery be delegated to the AMP_Img_Sanitizer and AMP_Video_Sanitizer respectively? The image sanitizer is already using advanced logic to fetch image dimensions. The video sanitizer could be augmented with this logic instead, as I believe it is only needed for video?
There was a problem hiding this comment.
Moved within dbc1c7c, missed the fetching image dimensions logic within AMP_Img_Sanitizer when creating this, thanks for noting that!
|
@westonruter On editor freeze with Gallery block -- just to confirm, do I understand correctly that the editor freeze is with inserting the default Gallery block and not the gallery shortcode into Shortcode block? Asking since couldn't reproduce the issue at this moment (using Gutenberg 2.9.1 on WP 4.9.6 on VVV), in theory this PR shouldn't influence the Gallery block. Does the freeze occur when clicking the block or already when inserting the block without any additional action? |
| el( ToggleControl, { | ||
| label: __( 'AMP loading indicator disabled' ), | ||
| checked: ampNoLoading, | ||
| onChange: function() { |
There was a problem hiding this comment.
For some reason, the first time I check the checkbox I get a warning:
Warning: A component is changing an uncontrolled input of type checkbox to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components
Maybe it's a bug in Gutenberg. It doesn't seem to cause a problem.
|
I just pulled down the latest from Gutenberg and I'm not able to reproduce the editor freezing when adding a Gallery block. |
…onents cannot yet be used in the React context
Fixes #1008.
Note that this PR is not a complete solution. It doesn't fully implement AMP Layout, also it doesn't include overridingrender_callbacks of dynamic blocks. This is partly implementing AMP Layout of all the core blocks (except for the front-end view of dynamic blocks which could be likely done in a similar way to #1010). The logic is not tested on all the blocks and was mainly usingparagraphandcodeblocks as examples during development, and some others briefly, it's possible that some of the blocks will need separate logic.What does the PR do?
ampLayoutattribute to all the core blocks which are converted to AMP elements leveraging the filterblocks.registerBlockType;editof each core block with filterblocks.BlockEdit. AddsAmp Layoutselect control to Inspector Controls (settings under Block tab when clicking on a block) which currently allows choosing between options that the specific element supports;Extendssaveof each (non-dynamic) core block with filterblocks.getSaveElement. Wraps the saved block in<amp-layout layout=[ampLayout value] width=1 height=1></ampLayout>tags.noloadingsupport for relevant blocks;amp-carouseloptional for gallery shortcode;Todo
noloadingsupport for image, video, and other embeds;