Skip to content

Load Photon resized images in gallery widget#3680

Merged
jeherve merged 2 commits intoAutomattic:masterfrom
jubstuff:fix/resize_image_gallery_widget
Jun 21, 2016
Merged

Load Photon resized images in gallery widget#3680
jeherve merged 2 commits intoAutomattic:masterfrom
jubstuff:fix/resize_image_gallery_widget

Conversation

@jubstuff
Copy link
Copy Markdown
Contributor

Fixes #1186 .

Changes proposed in this Pull Request:

  • As it already happens for Tiled Galleries, the Gallery widget now loads images for galleries and slideshows from Photon, resizing them accordingly.

@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Apr 18, 2016
@jeherve jeherve modified the milestones: 4.0.1, 4.0.2, 4.0.3 Apr 18, 2016

$this->img_src = add_query_arg( array( 'w' => $this->image->width, 'h' => $this->image->height, 'crop' => true ), $this->orig_file );

$this->img_src = jetpack_photon_url( $this->orig_file, array( 'resize' => sprintf( '%s,%s', $this->image->width, $this->image->height ) ) );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're using sprintf here, then those dimensions should probably be cast to decimal/numbers (%d), rather than strings (%s).

@jeherve jeherve modified the milestones: 4.1, 4.0.3 Apr 26, 2016
@mdawaffe
Copy link
Copy Markdown
Member

These changes make sense to me. @jeherve, can you recall any time something has gone wrong or someone has complained when we've photonizing image URLs?

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Apr 27, 2016

can you recall any time something has gone wrong or someone has complained when we've photonizing image URLs?

That shouldn't be a problem. We're photonizing image URLs in multiple other areas of the plugin (like Related Posts thumbnails for example), and it's usually not an issue as long as we have a good reason to do so.
I think performance is a good reason :)

@mdawaffe mdawaffe added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 20, 2016
@mdawaffe
Copy link
Copy Markdown
Member

This seems ready to me. @samhotchkiss?

foreach ( $instance['attachments'] as $attachment ) {
$attachment_image_src = wp_get_attachment_image_src( $attachment->ID, 'full' );
$attachment_image_src = $attachment_image_src[0]; // [url, width, height]
$attachment_image_src = jetpack_photon_url( $attachment_image_src[0], array( 'w' => $this->_instance_width ) ); // [url, width, height]
Copy link
Copy Markdown
Member

@jeherve jeherve Jun 15, 2016

Choose a reason for hiding this comment

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

Should we have $this->_instance_width going through the gallery_widget_content_width filter, like it's done for the Gallery widget?

cc @mdawaffe

@jeherve jeherve added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jun 15, 2016
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Jun 21, 2016

This seems to work well in my tests. Merging.

@jeherve jeherve merged commit 58d7e55 into Automattic:master Jun 21, 2016
@jeherve jeherve removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 21, 2016
jeherve added a commit that referenced this pull request Jun 21, 2016
jeherve added a commit that referenced this pull request Jun 22, 2016
georgestephanis added a commit that referenced this pull request Apr 4, 2017
…size resulted in bad things.

Switching back to the original params, albeit passing it through `jetpack_photon_url()` now as the newer Jetpack code did.

Context: #3680

See: https://[private link]

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

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Focus] Performance [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants