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

Conversation

miina
Copy link
Contributor

@miina miina commented May 5, 2018

Based on #1026.

Does the following:

  • Adds AMP Carousel toggle to Gallery block which allows displaying the gallery as carousel instead of default WP gallery.
  • Adds "Add lightbox effect" toggle to Gallery and Image core blocks.
    screen shot 2018-05-08 at 4 03 09 pm


$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.

@miina miina changed the title [Gutenberg] Add AMP Carousel toggle for Gallery block [Gutenberg] Add AMP Carousel for Gallery and AMP Lightbox features for Gallery and Image May 8, 2018
@miina
Copy link
Contributor Author

miina commented May 8, 2018

Note: Also added amp-image-lightbox implementation for Image and Gallery blocks within this PR since it was quite related.

}

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.

@@ -211,6 +242,15 @@ var ampEditorBlocks = ( function() {
ampLayout = attributes.ampLayout;

if ( 'core/shortcode' === name ) {
// Lets remove amp-carousel from edit view.
if ( component.hasGalleryShortcodeCarouselAttribute( attributes.text || '' ) ) {
props.setAttributes( { text: component.removeAmpCarouselFromShortcodeAtts( attributes.text ) } );
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be moved to an onChange handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, removing the attributes from the saved shortcode text on loading the block isn't related to any other attribute changing, the objective here was to not display the saved attributes within the shortcode already when the block is initially loaded, e.g. display just [gallery] instead of [gallery amp-lightbox=true]. Moving this part to onChange handler would cause the AMP attributes show up when first loading the block and remove these once onChange triggers.

Should we just display the attributes (in case they're not default) within the shortcode instead of trying to hide them? Thoughts?

}
// Lets remove amp-lightbox from edit view.
if ( component.hasGalleryShortcodeLightboxAttribute( attributes.text || '' ) ) {
props.setAttributes( { text: component.removeAmpLightboxFromShortcodeAtts( attributes.text ) } );
Copy link
Member

Choose a reason for hiding this comment

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

Per above, this will need to be moved to an onChange handler.

@@ -3,3 +3,11 @@
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
object-fit: contain;
}

#amp-image-lightbox-1 img {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be targeting img elements in selectors. I should think that only targeting amp-img should be done since img should really be the domain of the amp-img to control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like amp-image-lightbox component is empty by default and clicking on image which is targeting this lightbox will insert the img element into the amp-image-lightbox, the amp-image-lightbox component doesn't include an additional amp-img component within, targeting amp-img doesn't seem to be possible in this case.
For example after clicking on an an image the lightbox HTML looks like this:
screen shot 2018-05-21 at 10 34 06 pm
Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use AMP-generated class instead of img within 6900e2b.

onChange: function( value ) {
props.setAttributes( { ampLayout: value } );
if ( 'core/image' === props.name ) {
component.setImageBlockLayoutAttributes( props, value );
Copy link
Member

@westonruter westonruter May 21, 2018

Choose a reason for hiding this comment

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

Per above, this will need to be moved to an onChange handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is already in onChange handler.

$lightbox_tag = AMP_HTML_Utils::build_tag(
'amp-image-lightbox',
array(
'id' => AMP_Base_Sanitizer::AMP_IMAGE_LIGHTBOX_ID,
Copy link
Member

Choose a reason for hiding this comment

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

Why the static 'amp-image-lightbox-1'? Should this not have some auto-increment number instead that is appended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used 'amp-image-lightbox-1' originally since thought that maybe there will be more than one, however, looks like using one should be enough, maybe we could just leave it as 'amp-image-lightbox'. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to amp-image-lightbox.

@@ -384,6 +396,12 @@ public function filter_data_amp_attributes( $attributes, $amp_data ) {
if ( isset( $amp_data['noloading'] ) ) {
$attributes['data-amp-noloading'] = '';
}
if ( isset( $amp_data['lightbox'] ) ) {
$attributes['data-amp-lightbox'] = '';
$attributes['on'] = 'tap:' . self::AMP_IMAGE_LIGHTBOX_ID;
Copy link
Member

Choose a reason for hiding this comment

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

Per above, I think this needs to auto-increment a number to append to amp-image-lightbox-.

* Let gallery have be sized according to the max height.
* Constrain height of AMP gallery carousel by ratio with max width
* Remove duplicate data for jed_locale_data.
@westonruter
Copy link
Member

For some reason I get this warning when I click the Carousel or Lightbox toggles the first time:

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

@westonruter
Copy link
Member

At some point it would probably be a good idea to hide the gallery column count when slideshow is enabled:

image

}

.entry__content .wp-block-gallery[class*="columns-"] figure {
Copy link
Member

Choose a reason for hiding this comment

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

entry__content is not a standard class name.


#amp-image-lightbox div div > :first-child {
object-fit: contain;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be having any effect. I don't think it is needed?


.entry__content .wp-block-gallery[class*="columns-"] figure {
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

I think this may just be needed in the ampnews theme. It doesn't seem necessary in Twenty Fifteen. It's probably just that the ampnews theme lacks all of the required styles.

@westonruter westonruter added this to the v1.0 milestone May 30, 2018
@westonruter westonruter merged commit a15be99 into develop May 30, 2018
@westonruter westonruter deleted the add/amp-carousel-toggle branch May 30, 2018 23:37
$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.

@kienstra
Copy link
Contributor

kienstra commented Jun 5, 2018

Request For Testing

Hi @csossi,
Could you please test this?

  1. Go to https://native-ampconfdemo.pantheonsite.io/
  2. Create a new post, using Gutenberg (the default)
  3. Add a "Gallery Block," and add images to the gallery
  4. Publish the post
  5. Toggle the "Display as AMP carousel" and "Add lightbox effect" options that Miina showed in her comment.
  6. Go to the front-end of the post
  7. Expected: The gallery displays as a carousel if selected
  8. Expected: On selecting "Add lightbox effect," clicking an image expands it to fill the display

@csossi
Copy link

csossi commented Jun 7, 2018

@kienstra - Works fine on front-end, but noted that a message appeared in the admin on publishing post. Not sure if relevant here, but wanted you to see it:
image
The link here takes me to:
image

@csossi csossi assigned kienstra and unassigned kienstra and csossi Jun 7, 2018
@kienstra
Copy link
Contributor

kienstra commented Jun 7, 2018

Thanks, Should Be Fine

Hi @csossi,
Thanks for letting me know about that. The validation error should be fine, as it's not directly related to the post content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants