Load Photon resized images in gallery widget #3680
Conversation
jeherve
added
[Type] Enhancement
| @@ -21,8 +21,7 @@ public function __construct( $attachment_image, $needs_attachment_link, $graysca | |||
| $this->orig_file = wp_get_attachment_url( $this->image->ID ); | |||
| $this->link = $needs_attachment_link ? get_attachment_link( $this->image->ID ) : $this->orig_file; | |||
|
|
|||
| $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 ) ) ); | |||
There was a problem hiding this comment.
If you're using sprintf here, then those dimensions should probably be cast to decimal/numbers (%d), rather than strings (%s).
|
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? |
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. |
mdawaffe
added
[Status] Ready to Merge
|
This seems ready to me. @samhotchkiss? |
| @@ -253,7 +253,7 @@ public function slideshow_widget( $args, $instance ) { | |||
|
|
|||
| 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] | |||
There was a problem hiding this comment.
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
added
the
[Status] Needs Author Reply
|
This seems to work well in my tests. Merging. |
jeherve
removed
the
[Status] Ready to Merge
…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.
Fixes #1186 .
Changes proposed in this Pull Request:
grunt jshintbefore to commit your changes. It will allow you to detect errors in Javascript files.