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

Improve gallery sizing #2864

Merged
merged 2 commits into from Jul 25, 2019
Merged

Improve gallery sizing #2864

merged 2 commits into from Jul 25, 2019

Conversation

westonruter
Copy link
Member

This is intended to resolve issues reported with galleries in https://wordpress.org/support/topic/classic-block-galleries-amp-issues/

The appearance of galleries became unpredictable when images in landscape and portrait appear in the same gallery. The amp-carousel dimensions were being obtained from the max height of all images and the max width of all the images. Since the amp-carousel had a responsive layout, the width/height resulted in unexpected letterboxing above or to the side of the images. So this PR seeks to make the galleries more consistent by always opting for showing the carousel so that there will be no letterboxing above/below the image. Landscape images will fill the carousel, and portrait images will be constrained to fit inside the landscape box.

This PR also supplies missing srcset for images in shortcode galleries.

Testing

Create a gallery with two images, one portrait and one landscape:

Supply the attachment IDs in post_content such as the following (taking the place of 2715,2714):

<!-- wp:heading -->
<h2>Gallery Block</h2>
<!-- /wp:heading -->

<!-- wp:gallery {"ids":[2715,2714],"ampCarousel":true} -->
<ul class="wp-block-gallery columns-2 is-cropped" data-amp-carousel="true"><li class="blocks-gallery-item"><figure><img src="https://wordpressdev.lndo.site/content/uploads/2019/07/1367x2048-684x1024.png" alt="" data-id="2715" data-link="https://wordpressdev.lndo.site/?attachment_id=2715" class="wp-image-2715"/><figcaption>First slide</figcaption></figure></li><li class="blocks-gallery-item"><figure><img src="https://wordpressdev.lndo.site/content/uploads/2019/07/2048x1367-1024x684.png" alt="" data-id="2714" data-link="https://wordpressdev.lndo.site/?attachment_id=2714" class="wp-image-2714"/><figcaption>Second slide</figcaption></figure></li></ul>
<!-- /wp:gallery -->

<!-- wp:heading -->
<h2>Gallery Shortcode</h2>
<!-- /wp:heading -->

<!-- wp:shortcode {"ampCarousel":true} -->
[gallery ids="2715,2714"  link="none" size="large"]
<!-- /wp:shortcode -->

<!-- wp:heading -->
<h2>Gallery in Classic Block</h2>
<!-- /wp:heading -->

<p>[gallery link="none" size="large" ids="2715,2714"]</p>

Before

Before Portrait

before-portrait

Before Landscape

before-landscape

After

After Portrait

after-portrait

After Landscape

after-landscape


Build for testing: amp.zip (v1.2.1-beta1-20190722T201452Z-11d51844)

@westonruter westonruter added this to the v1.2.1 milestone Jul 22, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 22, 2019
@westonruter
Copy link
Member Author

@rsmith4321 Please test the amp.zip build on your site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants