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.

Sign up for GitHub

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

Jump to bottom

[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

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

Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion assets/js/amp-editor-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ var ampEditorBlocks = ( function() { // eslint-disable-line no-unused-vars
ampAttributes[ 'data-amp-lightbox' ] = attributes.ampLightbox;
}
if ( attributes.ampCarousel ) {
ampAttributes[ 'data-amp-carousel' ] = attributes.ampLightbox;
ampAttributes[ 'data-amp-carousel' ] = attributes.ampCarousel;
}

return _.extend( ampAttributes, props );
Expand Down
6 changes: 0 additions & 6 deletions includes/admin/class-amp-editor-blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,6 @@ public function enqueue_block_editor_assets() {
AMP__VERSION
);

wp_add_inline_script(
'amp-editor-blocks-build',
'wp.i18n.setLocaleData( ' . wp_json_encode( gutenberg_get_jed_locale_data( 'amp' ) ) . ', "amp" );',
'before'
);

// Scripts.
wp_enqueue_script(
'amp-editor-blocks-build',
Expand Down
71 changes: 37 additions & 34 deletions includes/sanitizers/class-amp-gallery-block-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,35 +78,30 @@ public function sanitize() {
continue;
}

$num_images = 0;
$images = null;

// If it's not AMP lightbox, look for links first.
if ( ! $is_amp_lightbox ) {
$images = $node->getElementsByTagName( 'a' );
$num_images = $images->length;
$images = $node->getElementsByTagName( 'a' );
}

if ( 0 === $num_images ) {

// If not linking to anything then look for <amp-img>.
$images = $node->getElementsByTagName( 'amp-img' );
$num_images = $images->length;
// If not linking to anything then look for <amp-img>.
if ( ! $images || 0 === $images->length ) {
$images = $node->getElementsByTagName( 'amp-img' );
}

if ( 0 === $num_images ) {
// Skip if no images found.
if ( ! $images || 0 === $images->length ) {
continue;
}

$carousel_height = $this->get_carousel_height( $node );
$carousel_attributes = array(
'height' => $carousel_height,
$amp_carousel = AMP_DOM_Utils::create_node( $this->dom, 'amp-carousel', array(
'height' => $this->get_carousel_height( $node ),
'type' => 'slides',
'layout' => 'fixed-height',
);
$amp_carousel = AMP_DOM_Utils::create_node( $this->dom, 'amp-carousel', $carousel_attributes );

for ( $j = $num_images - 1; $j >= 0; $j-- ) {
$amp_carousel->appendChild( $images->item( $j ) );
) );
foreach ( $images as $image ) {
$amp_carousel->appendChild( $image );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter for ( $j = $num_images - 1; $j >= 0; $j-- ) { was used here since when looping directly through the nodes (foreach ( $images as $image ) {) it's not working as expected, for example when adding a gallery with 5 images locally and using AMP Carousel it will just display the first three in the gallery. That's happening when testing locally now as well, is it working as expected for you?

Copy link
Member

Choose a reason for hiding this comment

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

@miina you're absolutely right. I broke it.

Copy link
Member

Choose a reason for hiding this comment

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

PR to fix opened in #1187.

}

$node->parentNode->replaceChild( $amp_carousel, $node );
Expand All @@ -117,39 +112,47 @@ public function sanitize() {
/**
* Get carousel height by containing images.
*
* @param DOMNode $node Node <ul>.
* @return int
* @param DOMElement $element The UL element.
* @return int Height.
*/
protected function get_carousel_height( $node ) {
$images = $node->getElementsByTagName( 'amp-img' );
protected function get_carousel_height( $element ) {
$images = $element->getElementsByTagName( 'amp-img' );
$num_images = $images->length;
$height = false;
$max_height = 0;
$max_width = 0;
if ( 0 === $num_images ) {
return self::FALLBACK_HEIGHT;
}
for ( $i = $num_images - 1; $i >= 0; $i-- ) {
$image = $images->item( $i );
foreach ( $images as $image ) {
/**
* Image.
*
* @var DOMElement $image
*/
$image_height = $image->getAttribute( 'height' );
if ( ! $image_height || ! is_numeric( $image_height ) ) {
continue;
if ( is_numeric( $image_height ) ) {
$max_height = max( $max_height, $image_height );
}
if ( ! $height ) {
$height = $image_height;
} elseif ( $height > $image_height ) {
$height = $image_height;
$image_width = $image->getAttribute( 'height' );
if ( is_numeric( $image_width ) ) {
$max_width = max( $max_width, $image_width );
}
}

return false === $height ? self::FALLBACK_HEIGHT : $height;
if ( ! empty( $this->args['content_max_width'] ) && $max_height > 0 && $max_width > $this->args['content_max_width'] ) {
$max_height = ( $max_width * $this->args['content_max_width'] ) / $max_height;
}

return ! $max_height ? self::FALLBACK_HEIGHT : $max_height;
}

/**
* Set lightbox related attributes to <amp-img> within gallery.
*
* @param DOMNode $node <ul> node.
* @param DOMElement $element The UL element.
*/
protected function add_lightbox_attributes_to_image_nodes( $node ) {
$images = $node->getElementsByTagName( 'amp-img' );
protected function add_lightbox_attributes_to_image_nodes( $element ) {
$images = $element->getElementsByTagName( 'amp-img' );
$num_images = $images->length;
if ( 0 === $num_images ) {
return;
Expand Down
4 changes: 3 additions & 1 deletion tests/test-class-amp-gallery-block-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ public function get_data() {
*/
public function test_sanitizer( $source, $expected ) {
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Gallery_Block_Sanitizer( $dom );
$sanitizer = new AMP_Gallery_Block_Sanitizer( $dom, array(
'content_max_width' => 600,
) );
$sanitizer->sanitize();
$content = AMP_DOM_Utils::get_content_from_dom( $dom );
$content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content );
Expand Down