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 srcset to images in the editor #12061

Merged
merged 2 commits into from
May 14, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 16, 2019

Independent companion to #11397
Implementation from Automattic/wp-calypso#30989

Add srcset to images in the Tiled Gallery block in the editor. This allows the browser to pick smaller assets when possible improving several aspects of performance.

Testing

  • No changes to the saved output. No block invalidation should occur. Try loading posts with older Tiled Galleries and they should work fine.
  • Add a Tiled Gallery block with a lot of large images.
  • Disable cache and reload the editor with a very small viewport.
  • Watch the network tab, initially smaller assets are requested.
  • Scale up the viewport.
  • Depending on the browser (different browsers handle srcset differently) you will likely see more requests fired for larger assets as the viewport width increases.

For a block with a lot of large images, the editor should be more responsive.

Changelog

  • Add srcset to Tiled Gallery block in the editor for an improved editing experience.

@sirreal sirreal added [Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Focus] Performance [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Apr 16, 2019
@sirreal sirreal self-assigned this Apr 16, 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 D26982-code before merging this PR. Thank you!

@sirreal sirreal force-pushed the update/tiled-gallery-block-editor-srcset branch from 9817f92 to dab6e2b Compare April 24, 2019 16:16
@matticbot
Copy link
Contributor

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

@sirreal sirreal added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] In Progress labels Apr 24, 2019
@sirreal sirreal marked this pull request as ready for review April 24, 2019 16:21
@sirreal sirreal requested a review from a team as a code owner April 24, 2019 16:21
@sirreal sirreal force-pushed the update/tiled-gallery-block-editor-srcset branch from dab6e2b to d3a2ffa Compare April 24, 2019 16:21
@sirreal sirreal requested a review from a team April 24, 2019 16:21
@jetpackbot
Copy link

jetpackbot commented Apr 24, 2019

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 4bafbe2

simison
simison previously approved these changes Apr 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.

Looking fabulous! I like the move to utils.

Tested with all layouts on Chrome — scaling the browser window bigger looks amazing in the Networks tab and images load blazing fast now:

Screenshot 2019-04-25 at 18 31 13

Tested also by adding blocks with old code and then reloading the editor to confirm nothing breaks.

@matticbot
Copy link
Contributor

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

@sirreal
Copy link
Member Author

sirreal commented Apr 25, 2019

Rebased and applied suggestion. Re-review please 🙂

@sirreal sirreal requested a review from simison April 25, 2019 15:49
const maxWidth = Math.min( PHOTON_MAX_RESIZE, width, height );

srcSet = range( minWidth, maxWidth, step )
.map( srcsetWidth => {
Copy link
Member

@simison simison Apr 25, 2019

Choose a reason for hiding this comment

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

BTW you're using a mixture of srcset and srcSet in variable names. Not a biggie, just thought you ought to know. 😜

Copy link
Member Author

Choose a reason for hiding this comment

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

😱 Yeah… I think I was mostly using srcset, but React wants srcSet so… 🤷‍♂️

simison
simison previously approved these changes Apr 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.

Still fabulous. 👍

@sirreal
Copy link
Member Author

sirreal commented Apr 25, 2019

FYI @MichaelArestad - we'd talked about this block's performance in the editor, I suspect you'll find this PR is a solid improvement.

@MichaelArestad
Copy link
Contributor

Wonderful!

@sirreal
Copy link
Member Author

sirreal commented Apr 29, 2019

Rebased to fix conflict

@matticbot
Copy link
Contributor

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

@simison simison requested a review from a team April 30, 2019 09:05
@simison simison added this to the 7.4 milestone Apr 30, 2019
@sirreal
Copy link
Member Author

sirreal commented May 9, 2019

I believe this is ready but needs @Automattic/jetpack-crew review.

@simison
Copy link
Member

simison commented May 14, 2019

@kraftbj any thoughts on getting this merged? Would be nice to give it some time to get tested in master before 7.4.

@kraftbj
Copy link
Contributor

kraftbj commented May 14, 2019

@sirreal @simison Apologies! Head was in the 7.3 game. Looking and testing it, it works for me, so merging.

@kraftbj kraftbj merged commit 4f6c119 into master May 14, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 14, 2019
@kraftbj kraftbj deleted the update/tiled-gallery-block-editor-srcset branch May 14, 2019 19:30
@simison
Copy link
Member

simison commented May 14, 2019 via email

@kraftbj
Copy link
Contributor

kraftbj commented May 14, 2019

Could y'all check out the wpcom merge. There were some test failures when actually trying to get it in so want to defer to y'all on if there's something bigger in play.

@simison
Copy link
Member

simison commented May 17, 2019

r191914-wpcom

jeherve added a commit that referenced this pull request May 17, 2019
jeherve added a commit that referenced this pull request May 17, 2019
jeherve added a commit that referenced this pull request May 23, 2019
jeherve added a commit that referenced this pull request May 23, 2019
jeherve added a commit that referenced this pull request May 27, 2019
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
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 [Focus] Performance 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.

7 participants