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

[Gutenberg] Add AMP Carousel for Gallery and AMP Lightbox features for Gallery and Image #1121

Merged
merged 33 commits into from
May 30, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
52943ed
Start adding carousel toggle for Gallery.
miina May 4, 2018
fd662d7
Merge #1008.
miina May 5, 2018
54f9555
Add amp-carousel class.
miina May 5, 2018
ed1f9de
Filter saving the post for when AMP Plugin gets disabled. Add tests.
miina May 5, 2018
0263878
Add lightbox logic for Image block.
miina May 7, 2018
40486fe
Add lightbox for gallery block.
miina May 8, 2018
96bd515
Fix CSS.
miina May 8, 2018
553c4da
Merge develop.
miina May 8, 2018
22060ec
Add block-related JS with blocks filter instead of admin filter.
miina May 8, 2018
5b63476
Fix issue with assigning props directly.
miina May 9, 2018
2e102ea
Add lightbox to gallery shortcode block.
miina May 9, 2018
c3aad13
Merge remote-tracking branch 'origin/develop' into add/amp-carousel-t…
miina May 16, 2018
dd924ec
Merge #1026
miina May 16, 2018
ca133a4
Adjust logic to using data- attributes.
miina May 16, 2018
22597fa
Fixes. Merge #1008
miina May 21, 2018
b70af66
Update method comments. Remove unused dynamicBlocks data.
miina May 21, 2018
438567d
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 21, 2018
7ee5fd3
Merge branch 'develop' into add/amp-carousel-toggle
westonruter May 23, 2018
58430e4
Merge develop & resolve conflicts.
miina May 25, 2018
55e8fff
Resolve conflicts.
miina May 25, 2018
f658c43
Merge remote-tracking branch 'origin/develop' into add/amp-carousel-t…
miina May 28, 2018
6900e2b
Fixes.
miina May 28, 2018
a69dd55
Merge develop & resolve conflicts.
miina May 30, 2018
ff2cdd2
Merge develop.
miina May 30, 2018
d5169ef
Fixes after merge.
miina May 30, 2018
13267f6
Fix displaying AMP carousel.
miina May 30, 2018
134335e
Fix checking for shortcode.
miina May 30, 2018
26c491e
Fix jscs.
miina May 30, 2018
46c3a50
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter May 30, 2018
201654f
Fix AMP carousel for gallery block.
westonruter May 30, 2018
f9f3a05
Pass through all attributes from img to amp-img
westonruter May 30, 2018
cc53173
Force carousels to be used when AMP theme support is absent (for back…
westonruter May 30, 2018
dbe281c
Remove apparently unneessary object-fit from amp-image-lightbox; remo…
westonruter May 30, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions assets/js/amp-editor-blocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ var ampEditorBlocks = ( function() {
* @return {*} Settings.
*/
component.addAMPAttributes = function addAMPAttributes( settings, name ) {
// Gallery settings for shortcode.
if ( 'core/shortcode' === name ) {
// AMP Carousel settings.
if ( 'core/shortcode' === name || 'core/gallery' === name ) {
if ( ! settings.attributes ) {
settings.attributes = {};
}
Expand Down Expand Up @@ -163,6 +163,8 @@ var ampEditorBlocks = ( function() {
}, props ) )
];
}
} else if ( 'core/gallery' === name ) {
inspectorControls = component.setUpGalleryInpsectorControls( props );
} else if ( -1 !== component.data.mediaBlocks.indexOf( name ) || 0 === name.indexOf( 'core-embed/' ) ) {
inspectorControls = component.setUpInspectorControls( props );
}
Expand Down Expand Up @@ -267,6 +269,38 @@ var ampEditorBlocks = ( function() {
);
};

/**
* Set up inspector controls for Gallery block.
* Adds ampCarousel attribute for displaying the output as amp-carousel.
*
* @param {Object} props Props.
* @return {*} Inspector controls.
*/
component.setUpGalleryInpsectorControls = function setUpGalleryInpsectorControls( props ) {
var ampCarousel = props.attributes.ampCarousel,
isSelected = props.isSelected,
el = wp.element.createElement,
InspectorControls = wp.blocks.InspectorControls,
ToggleControl = wp.components.ToggleControl,
PanelBody = wp.components.PanelBody,
toggleControl;

toggleControl = el( ToggleControl, {
label: 'Display as AMP carousel',
checked: ampCarousel,
onChange: function() {
props.setAttributes( { ampCarousel: ! ampCarousel } );
}
} );
return isSelected && (
el( InspectorControls, { key: 'inspector' },
el( PanelBody, { title: 'AMP Settings' },
toggleControl
)
)
);
};

/**
* Set up inspector controls for shortcode block.
* Adds ampCarousel attribute in case of gallery shortcode.
Expand Down Expand Up @@ -347,13 +381,16 @@ var ampEditorBlocks = ( function() {
);
}

// In case AMP Layout, add layout to classname.
// In case AMP attributes, add info to classname.
if ( component.hasAmpLayoutSet( attributes || '' ) ) {
ampClassName += ' amp-layout-' + attributes.ampLayout;
}
if ( component.hasAmpNoLoadingSet( attributes || '' ) ) {
ampClassName += ' amp-noloading';
}
if ( component.hasAmpCarouselSet( attributes || '' ) ) {
ampClassName += ' amp-carousel';
}

if ( '' !== ampClassName && attributes.className !== ampClassName ) {
props.className = ampClassName.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @miina,
In my browser, this didn't assign the new props.className value, causing the lightbox to not appear on the front-end.

But with this change (borrowing from Weston's solution here), it did:

props = _.extend( {}, props, { className: ampClassName.trim() } );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @kienstra, thanks for finding this, makes sense that the props.className can't be directly set this way.

Expand All @@ -365,6 +402,16 @@ var ampEditorBlocks = ( function() {
return element;
};

/**
* Check if AMP Carousel is set.
*
* @param {Object} attributes Attributes.
* @return {boolean} If is set.
*/
component.hasAmpCarouselSet = function hasAmpCarouselSet( attributes ) {
return attributes.ampCarousel && false !== attributes.ampCarousel;
};

/**
* Check if AMP NoLoading is set.
*
Expand Down
10 changes: 8 additions & 2 deletions includes/admin/class-amp-editor-blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,13 @@ public static function filter_rest_pre_insert_post( $prepared_post ) {

// Get the class of the figure.
preg_match( "'<figure class=\"(.*?)\"'si", $block['innerHTML'], $match );
if ( ! $match ) {

// Lets check for gallery block class.
if ( empty( $match ) ) {
preg_match( "'<ul class=\"(.*?)\"'si", $block['innerHTML'], $match );
}

if ( ! empty( $match ) ) {
continue;
}
$class_names = explode( ' ', $match[1] );
Expand All @@ -58,7 +64,7 @@ public static function filter_rest_pre_insert_post( $prepared_post ) {
$class_attr[] = $block['attrs']['className'];
}

// Take everything except for the wp-block-{block_type} class.
// Take everything with amp-*.
foreach ( $class_names as $class_name ) {
if ( false !== strpos( $class_name, 'amp-' ) ) {
$class_attr[] = $class_name;
Expand Down
3 changes: 2 additions & 1 deletion includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -484,11 +484,12 @@ function amp_get_content_sanitizers( $post = null ) {
'AMP_Comments_Sanitizer' => array(),
'AMP_Video_Sanitizer' => array(),
'AMP_Audio_Sanitizer' => array(),
'AMP_Block_Sanitizer' => array(),
'AMP_Playbuzz_Sanitizer' => array(),
'AMP_Iframe_Sanitizer' => array(
'add_placeholder' => true,
),
'AMP_Gallery_Block_Sanitizer' => array(),
'AMP_Block_Sanitizer' => array(), // Note: Block sanitizer must come after embed / media sanitizers since it's logic is using the already sanitized content.
'AMP_Style_Sanitizer' => array(),
'AMP_Tag_And_Attribute_Sanitizer' => array(), // Note: This whitelist sanitizer must come at the end to clean up any remaining issues the other sanitizers didn't catch.
),
Expand Down
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class AMP_Autoloader {
'AMP_Base_Sanitizer' => 'includes/sanitizers/class-amp-base-sanitizer',
'AMP_Blacklist_Sanitizer' => 'includes/sanitizers/class-amp-blacklist-sanitizer',
'AMP_Block_Sanitizer' => 'includes/sanitizers/class-amp-block-sanitizer',
'AMP_Gallery_Block_Sanitizer' => 'includes/sanitizers/class-amp-gallery-block-sanitizer',
'AMP_Iframe_Sanitizer' => 'includes/sanitizers/class-amp-iframe-sanitizer',
'AMP_Img_Sanitizer' => 'includes/sanitizers/class-amp-img-sanitizer',
'AMP_Comments_Sanitizer' => 'includes/sanitizers/class-amp-comments-sanitizer',
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-block-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public function sanitize() {
continue;
}

// We are looking for <figure> elements with layout attribute only.
// We are looking for <figure> elements with AMP classes only.
if ( false === strpos( $attributes['class'], 'amp-layout-' ) && false === strpos( $attributes['class'], 'amp-noloading' ) ) {
continue;
}
Expand Down
91 changes: 91 additions & 0 deletions includes/sanitizers/class-amp-gallery-block-sanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php
/**
* Class AMP_Gallery_Block_Sanitizer.
*
* @package AMP
*/

/**
* Class AMP_Gallery_Block_Sanitizer
*
* Modifies gallery block to match the block's AMP-specific configuration.
*/
class AMP_Gallery_Block_Sanitizer extends AMP_Base_Sanitizer {

/**
* Value used for width of amp-carousel.
*
* @since 1.0
*
* @const int
*/
const FALLBACK_WIDTH = 600;

/**
* Value used for height of amp-carousel.
*
* @since 1.0
*
* @const int
*/
const FALLBACK_HEIGHT = 480;

/**
* Tag.
*
* @var string Ul tag to identify wrapper around gallery block.
* @since 1.0
*/
public static $tag = 'ul';

/**
* Sanitize the gallery block contained by <ul> element where necessary.
*
* @since 0.2
*/
public function sanitize() {
$nodes = $this->dom->getElementsByTagName( self::$tag );
$num_nodes = $nodes->length;
if ( 0 === $num_nodes ) {
return;
}

for ( $i = $num_nodes - 1; $i >= 0; $i-- ) {
$node = $nodes->item( $i );

// We're looking for <ul> elements that at least one child.
if ( 0 === count( $node->childNodes ) ) {
continue;
}

$attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node );

// We are only looking for <ul> elements which have amp-carousel as class.
if ( ! isset( $attributes['class'] ) || false === strpos( $attributes['class'], 'amp-carousel' ) ) {
continue;
}

$images = $node->getElementsByTagName( 'a' );
$num_images = $images->length;

if ( 0 === $num_images ) {
continue;
}

$attributes = array(
'width' => self::FALLBACK_WIDTH,
'height' => self::FALLBACK_HEIGHT,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what could be a better alternative for setting width and height. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Could the AMP_Gallery_Block_Sanitizer get added to the sanitizers list so that it is forced to be run after the AMP_Img_Sanitizer? Then you would be able to use the width and height of the images inside of the gallery to obtain the width and height for the overall gallery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added AMP_Gallery_Block_Sanitize::get_carousel_height() which gets the heights of the images and sets the gallery height according to the smallest image. However, saw that basically all the images had the default height and width set (400px) within AMP_Image_Sanitizer::determine_dimensions(), so also added AMP_Base_Sanitizer::set_attachment_width_and_height_by_src() which gets the height and width from the image itself and is used in AMP_Image_Sanitizer::determine_dimensions() before the logic that adds the fallback width and height. Does this make sense, or does it interfere with the logic that sets the fallback width and height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking that this might also change the looks for default gallery in an unexpected way.

'type' => 'slides',
'layout' => 'responsive',
);
$amp_carousel = AMP_DOM_Utils::create_node( $this->dom, 'amp-carousel', $attributes );

for ( $j = $num_images - 1; $j >= 0; $j-- ) {
$amp_carousel->appendChild( $images->item( $j ) );
}

$node->parentNode->replaceChild( $amp_carousel, $node );
$this->did_convert_elements = true;
}
}
}
57 changes: 57 additions & 0 deletions tests/test-class-amp-gallery-block-sanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php
/**
* Class AMP_Gallery_Block_Sanitizer_Test.
*
* @package AMP
*/

/**
* Class AMP_Gallery_Block_Sanitizer_Test
*/
class AMP_Gallery_Block_Sanitizer_Test extends WP_UnitTestCase {

/**
* Get data.
*
* @return array
*/
public function get_data() {
return array(
'no_ul' => array(
'<p>Lorem Ipsum Demet Delorit.</p>',
'<p>Lorem Ipsum Demet Delorit.</p>',
),

'no_a' => array(
'<ul class="amp-carousel"><amp-img></amp-img></ul>',
'<ul class="amp-carousel"><amp-img></amp-img></ul>',
),

'no_amp_carousel' => array(
'<ul><a><amp-img></amp-img></a></ul>',
'<ul><a><amp-img></amp-img></a></ul>',
),

'data_amp_with_gallery' => array(
'<ul class="amp-carousel"><li class="blocks-gallery-item"><figure><a href="http://example.com"><amp-img src="http://example.com/img.png" width="600" height="400"></amp-img></a></figure></li></ul>',
'<amp-carousel width="600" height="480" type="slides" layout="responsive"><a href="http://example.com"><amp-img src="http://example.com/img.png" width="600" height="400"></amp-img></a></amp-carousel>',
),
);
}

/**
* Test sanitizer.
*
* @dataProvider get_data
* @param string $source Source.
* @param string $expected Expected.
*/
public function test_sanitizer( $source, $expected ) {
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Gallery_Block_Sanitizer( $dom );
$sanitizer->sanitize();
$content = AMP_DOM_Utils::get_content_from_dom( $dom );
$content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content );
$this->assertEquals( $expected, $content );
}
}