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
Collaborator

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,

This comment has been minimized.

@miina

miina May 5, 2018

Collaborator

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

This comment has been minimized.

@westonruter

westonruter May 7, 2018

Member

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?

This comment has been minimized.

@miina

miina May 8, 2018

Collaborator

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?

This comment has been minimized.

@miina

miina May 8, 2018

Collaborator

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

miina added some commits May 7, 2018

@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 added some commits May 8, 2018

@miina

This comment has been minimized.

Copy link
Collaborator

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();

This comment has been minimized.

@kienstra

kienstra May 8, 2018

Collaborator

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() } );

This comment has been minimized.

@miina

miina May 9, 2018

Collaborator

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

miina and others added some commits May 9, 2018

@@ -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 ) } );

This comment has been minimized.

@westonruter

westonruter May 21, 2018

Member

This will need to be moved to an onChange handler.

This comment has been minimized.

@miina

miina May 21, 2018

Collaborator

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 ) } );

This comment has been minimized.

@westonruter

westonruter May 21, 2018

Member

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 {

This comment has been minimized.

@westonruter

westonruter May 21, 2018

Member

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?

This comment has been minimized.

@miina

miina May 21, 2018

Collaborator

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?

This comment has been minimized.

@miina

miina May 28, 2018

Collaborator

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 );

This comment has been minimized.

@westonruter

westonruter May 21, 2018

Member

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

This comment has been minimized.

@miina

miina May 21, 2018

Collaborator

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,

This comment has been minimized.

@westonruter

westonruter May 21, 2018

Member

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

This comment has been minimized.

@miina

miina May 21, 2018

Collaborator

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?

This comment has been minimized.

@miina

miina May 28, 2018

Collaborator

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;

This comment has been minimized.

@westonruter

westonruter May 21, 2018

Member

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

Fix AMP carousel for gallery block.
* 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 westonruter force-pushed the add/amp-carousel-toggle branch from b2382d7 to 201654f May 30, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 30, 2018

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

This comment has been minimized.

Copy link
Member

westonruter commented May 30, 2018

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

image

westonruter added some commits May 30, 2018

}

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

This comment has been minimized.

@westonruter

westonruter May 30, 2018

Member

entry__content is not a standard class name.


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

This comment has been minimized.

@westonruter

westonruter May 30, 2018

Member

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%;

This comment has been minimized.

@westonruter

westonruter May 30, 2018

Member

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 force-pushed the add/amp-carousel-toggle branch from 041311c to dbe281c May 30, 2018

@westonruter westonruter added this to the v1.0 milestone May 30, 2018

@westonruter westonruter merged commit a15be99 into develop May 30, 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/amp-carousel-toggle branch May 30, 2018

@westonruter westonruter added this to Ready for QA in v1.0 May 31, 2018

$amp_carousel->appendChild( $images->item( $j ) );
) );
foreach ( $images as $image ) {
$amp_carousel->appendChild( $image );

This comment has been minimized.

@miina

miina May 31, 2018

Collaborator

@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?

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

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

This comment has been minimized.

@westonruter

westonruter May 31, 2018

Member

PR to fix opened in #1187.

@kienstra

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Jun 7, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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