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

Photon: srcset to use custom thumbnail sizes #3040

Merged
merged 5 commits into from
Nov 19, 2015

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Nov 18, 2015

If there are existing custom thumbnails at the matching size, we should use those instead.

Fixes #2919
Enhances #2885 See #2804

@kraftbj kraftbj added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 18, 2015
@kraftbj kraftbj added this to the 3.8.1 milestone Nov 18, 2015
@kraftbj
Copy link
Contributor Author

kraftbj commented Nov 18, 2015

That said, I'm not sure if this approach is better/worse than something like 1ffcec2 or neither.

@kraftbj
Copy link
Contributor Author

kraftbj commented Nov 19, 2015

This is no good. You can have multiple custom thumbs of the same width. Assuming no Core changes, the other commit method is probably better. Let Core decide which image sizes to use and roll from there.

@kraftbj
Copy link
Contributor Author

kraftbj commented Nov 19, 2015

Core Slack chatter about if this approach seems sane to @joemcgill and asking about viability of a Core patch to make the dims easier to grab: https://wordpress.slack.com/archives/feature-respimg/p1447899460000167

@kraftbj
Copy link
Contributor Author

kraftbj commented Nov 19, 2015

Core ticket that would enhance the filter and would be beneficial to us: https://core.trac.wordpress.org/ticket/34736

@joemcgill
Copy link

If 34736 doesn't land before 4.4 I would recommend looking at an option similar to 5be08ef but matching the the $source['url'] against each $size['file'] from the meta array being passed to the filter. Alternately, you could use the $size_array to figure out the w/h ratio and use that to determine the correct height argument, since all the images in the srcset should have the same aspect ratio.

Not necessarily related, but you might be able to speed your filter up a bit by grabbing the full size image directly from $image_meta['file'], rather than runningstrip_image_dimensions_maybe()`.

@@ -579,17 +579,24 @@ public function filter_image_downsize( $image, $attachment_id, $size ) {
* @uses self::validate_image_url, jetpack_photon_url
* @return array An array of Photon image urls and widths.
*/
public function filter_srcset_array( $sources ) {
public function filter_srcset_array( $sources, $size_array, $image_src, $image_meta, $attachment_id ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these extra parameters here? I can't see any place where they are being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be removed from both lines. Added to make exploring options to fix faster, but in the end nothing helpful there for the final proposal. Thanks for catching it, can remove in a few hours if you don't get to it first.

@zinigor zinigor 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 Nov 19, 2015
@zinigor
Copy link
Member

zinigor commented Nov 19, 2015

Read the discussion, tested the changes, this looks good except one thing I commented about earlier.

…ntermediate size

We get the full-size image of the post thumbnail if the intermediate size is unreachable, or too small for the size set by the theme.  This fixes images distorting to fit the post-thumbnail container
dereksmart added a commit that referenced this pull request Nov 19, 2015
Photon: srcset to use custom thumbnail sizes
@dereksmart dereksmart merged commit 32eb952 into master Nov 19, 2015
@dereksmart dereksmart deleted the update/photon-image-srcset branch November 19, 2015 16:58
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants