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

Tiled Gallery block: Add server render srcset #11397

Merged
merged 7 commits into from
Apr 29, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Feb 22, 2019

Related to https://github.com/Automattic/wp-calypso/issues/30118

Add srcset to Tiled Gallery blocks. This adds a srcset which will provide improved image sizes using photon.

Compainion to #12061

It provides a number of alternatives in the srcset, between a calculated minimum and maximum, with a step.

Current values:

Min Max Step
600px 2000px 300px

There were enough changes that it felt appropriate to rework the simple block registration as part of a class that serves to namespace some functions.

Questions

Are the minimum and maximum values appropriate? The maximum 2000px was found to be around the Photon threshold on the image size in bytes.

Is the step appropriate? Should we have more or less alternatives?

Testing

  • Add a tiled gallery
  • Publish and view the post on the frontend
  • Verify that the original img[src] is preserved and correct (should be the same as the src in the code editor)
  • Verify that the img[srcset] looks good
  • Verify that the srcset results in different image sizes loading at different viewport widths. Note that different browsers may exhibit slightly different behavior.
    • Start with a narrow viewport
    • Watch img requests in your network tab. Filter on /(i[0-2]\.wp\.com)|(files\.wordpress\.com)/ to see only photon requests.
    • Scale the viewport up and watch larger images load.
  • Test with lazy images enabled and disabled.

It's very important to test all the different layouts!

Also verify the same behavior in D24729-code for Simple sites.

@sirreal sirreal added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Feb 22, 2019
@sirreal sirreal added this to the 7.1 milestone Feb 22, 2019
@sirreal sirreal self-assigned this Feb 22, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D24729-code before merging this PR. Thank you!

@sirreal sirreal changed the title Initial pass at ssr srcset Tiled Gallery block: Add server render srcset Feb 22, 2019
@sirreal sirreal force-pushed the add/tiled-gallery-block-ssr-srcset branch from 2aa4317 to edff3f9 Compare February 25, 2019 10:35
@sirreal sirreal marked this pull request as ready for review February 25, 2019 13:11
@sirreal sirreal requested a review from a team as a code owner February 25, 2019 13:11
@sirreal sirreal added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Feb 25, 2019
@sirreal sirreal requested a review from a team February 25, 2019 13:11
@sirreal

This comment has been minimized.

@matticbot
Copy link
Contributor

sirreal, Your synced wpcom patch D24729-code has been updated.

@simison
Copy link
Member

simison commented Feb 25, 2019

Noting that having "lazy images" feature enabled also fiddles with these attributes, so it's important to verify this works with that module on or off.

@simison

This comment has been minimized.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I tested this in Jetpack, and I would have a few questions, and a concern:

Currently on master, if I create a squared gallery it ends up loading 800px-wide images in a space where only 692px can fit:

image

It's not perfect, but that works.

With this PR, however, it ends up loading the 600px version of the image, thus upscaling it a little, which is definitely not ideal:

image

extensions/blocks/tiled-gallery/tiled-gallery.php Outdated Show resolved Hide resolved
extensions/blocks/tiled-gallery/tiled-gallery.php Outdated Show resolved Hide resolved
extensions/blocks/tiled-gallery/tiled-gallery.php Outdated Show resolved Hide resolved
@sirreal
Copy link
Member Author

sirreal commented Feb 25, 2019

but when I disable lazy loading images, I don't get square layout working

I've been unable to reproduce this on my Jetpack site.

simison
simison previously approved these changes Feb 25, 2019
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

but when I disable lazy loading images, I don't get square layout working

So this was due be building blocks bundle with Automattic/wp-calypso#30724 — with the latest master build from Calypso all works great. 👍

I've tested with and without Lazy loading images, with and without carousel features as well.

@kraftbj kraftbj added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Apr 29, 2019
@matticbot
Copy link
Contributor

sirreal, Your synced wpcom patch D24729-code has been updated.

@simison
Copy link
Member

simison commented Apr 29, 2019

Rebased + addressed PHP linter complaining about filename.

@kraftbj good for another look! :-)

@simison simison added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 29, 2019
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

There's an issue that exists in master per @simison preventing images from getting the ssl arg, so approving this to merge and will fix that up in an upcoming PR.

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 29, 2019
@simison simison merged commit 228097d into master Apr 29, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 29, 2019
@simison simison deleted the add/tiled-gallery-block-ssr-srcset branch April 29, 2019 19:38
@simison
Copy link
Member

simison commented Apr 29, 2019

D24729-code still needs to be deployed but since this touches only one infrequently updated file and it's night in Europe, I'm leaving it for tomorrow for less risky wpcom deploy.

Tracking editor dropping ssl=1 in #12199

@sirreal
Copy link
Member Author

sirreal commented Apr 30, 2019

D24729-code (this diff) requires D27606-code

kraftbj added a commit that referenced this pull request Apr 30, 2019
@sirreal
Copy link
Member Author

sirreal commented May 9, 2019

r191530-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] Normal Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants