Responsive image attributes #1819

maxxwv · 2018-10-16T21:56:40Z

Issue

I didn't see an easy way to get the srcset and sizes attributes for a Timber\Image object that was as simple as post.thumbnail.src.

Solution

It seemed like a good idea to add srcset and sizes as public methods, allowing post.thumbnail.srcset and post.thumbnail.sizes. Unfortunately, .sizes didn't work but img_sizes did.

Impact

As far as I can tell, none.

Usage Changes

Should be no impact.

Considerations

For whatever reason, creating a size() method in the Image object wasn't working. It didn't throw an error, just that post.thumbnail.sizes didn't ever call the method. It'd be a more intuitive call than img_sizes, which would be nice.

Testing

There's really nothing to fail, it's just a method to get the data that's more in keeping with the rest of the project.

codecov · 2018-10-16T22:19:26Z

Codecov Report

Merging #1819 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1819      +/-   ##
============================================
+ Coverage     94.74%   94.98%   +0.24%     
- Complexity     1534     1535       +1     
============================================
  Files            48       48              
  Lines          3632     3629       -3     
============================================
+ Hits           3441     3447       +6     
+ Misses          191      182       -9

Impacted Files	Coverage Δ	Complexity Δ
lib/Image.php	`90.62% <100%> (+5.34%)`	`73 <4> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2a6705...3676282. Read the comment docs.

jarednova · 2018-10-18T02:30:31Z

lib/Image.php

+	public function img_sizes( $size = "full" ) {
+		$img_sizes = wp_get_attachment_image_sizes($this->ID, $size);
+		$img_sizes = apply_filters('timber/image/img_sizes', $img_sizes, $this->ID);
+		$img_sizes = apply_filters('timber_image_img_sizes', $img_sizes, $this->ID);


We're deprecating this filter style, so you can remove

Suggested change

$img_sizes = apply_filters('timber_image_img_sizes', $img_sizes, $this->ID);

jarednova · 2018-10-18T02:32:04Z

Thank you @maxxwv! This looks like a good implementation of the WP methods. Before we merge, I want to make sure we have test coverage (even though it's a pretty simple thing, I want to make sure the functionality is always checked). Is that something you'd be up for trying?

gchtr

This seems like such a simple solution 🎉. Great stuff! And you also included examples, which is always great! Thanks for that!

I add some comments about the filters, because this is really my only concern right now.

gchtr · 2018-10-18T15:10:55Z

lib/Image.php

+	public function srcset( $size = "full" ) {
+		$srcset = wp_get_attachment_image_srcset($this->ID, $size);
+		$srcset = apply_filters('timber/image/srcset', $srcset, $this->ID);
+		$srcset = apply_filters('timber_image_srcset', $srcset, $this->ID);


Suggested change

$srcset = apply_filters('timber_image_srcset', $srcset, $this->ID);

Old filter style, see Jared’s review.

gchtr · 2018-10-18T15:13:18Z

lib/Image.php

+	 */
+	public function srcset( $size = "full" ) {
+		$srcset = wp_get_attachment_image_srcset($this->ID, $size);
+		$srcset = apply_filters('timber/image/srcset', $srcset, $this->ID);


I’m not sure if we should add this filter, because the wp_get_attachment_image_srcset() function already provides 3 filters to filter the resulting srcset. What would be the benefit of having this against using the filters in wp_get_attachment_image_srcset()?

gchtr · 2018-10-18T15:18:53Z

lib/Image.php

+	 */
+	public function img_sizes( $size = "full" ) {
+		$img_sizes = wp_get_attachment_image_sizes($this->ID, $size);
+		$img_sizes = apply_filters('timber/image/img_sizes', $img_sizes, $this->ID);


Also here, I’m not sure if we need this filter at all, because wp_get_attachment_image_sizes() calls wp_calculate_image_sizes() in the last line, which in turn has a wp_calculate_image_sizes filter in the last line. So the timber/image/img_sizes filter would be called immediately after wp_calculate_image_sizes. What would be the benefit of this?

maxxwv · 2018-10-18T15:48:37Z

Thanks for the comments!

@jarednova - Sure, I'll take a stab at some basic tests, though honestly it's been a while since I've done unit testing so I can't guarantee much there. I'll look at the test-timber-image.php file and maybe the resize tests for examples and inspiration, unless there's a more appropriate place.

@gchtr - I'm fine with pulling out those filters. I just put them in to match the src() interface more closely, to be honest. I'll pull them out in the next revision.

I was also thinking it could be handy to have a method that runs both these methods and returns a single string of the attributes, as neither is really all that useful on their own. Thoughts or opinions?

…ponsive data.

…sive_image * 'master' of https://github.com/timber/timber: Fix for older php versions Added simple current page item test Add comments and apply coding standards Added depth arg from TimberMenu Added filters for plugins to hook into TimberMenu

…nto responsive_image * 'responsive_image' of https://github.com/maxxwv/timber:

coveralls · 2018-10-19T21:05:35Z

Coverage decreased (-0.05%) to 93.98% when pulling 49a0fe9 on maxxwv:responsive_image into 604b3c1 on timber:master.

coveralls · 2018-10-19T21:05:36Z

Coverage increased (+0.1%) to 94.294% when pulling 3676282 on maxxwv:responsive_image into a2a6705 on timber:master.

maxxwv · 2018-10-27T22:48:36Z

@gchtr @jarednova - not sure what to do about the coverage failure, but everything else should be good to go. Let me know if there's anything else you need from me.

also some bonus `@codeCoverageIgnore`s on some deprecated functions to help juice code coverage scores

jarednova · 2018-11-10T02:23:47Z

Everything's looking good @maxxwv. I changed the spacing to match WP style (tabs > spaces). The coverage failure was b/c of the is_image sanity checks. I'm fine to leave as-is w/o writing a bunch of tests to cover the non-results — merged!

Thanks again for the code, tests and examples for the docs!

drzraf · 2019-02-05T13:16:13Z

The phpdoc example provided uses sizes="{{ post.thumbnail.sizes }}" instead of sizes="{{ post.thumbnail.img_sizes }}"

timber#1819 (comment)

maxxwv added 4 commits October 16, 2018 12:50

Add responsive image info getters

cdf3475

Add filters

bd0270b

Fix typos in documentation

57610fe

Another typo

3714c52

jarednova added the 1.x label Oct 18, 2018

jarednova reviewed Oct 18, 2018

View reviewed changes

jarednova added the needs-unit-test label Oct 18, 2018

gchtr reviewed Oct 18, 2018

View reviewed changes

maxxwv and others added 8 commits October 19, 2018 16:39

Remove redundant apply_filter() calls.

bbf6c2a

Make sure the attachment is an image before attempting to get the res…

154798e

…ponsive data.

Add attachment_metadata to post_meta table for test image.

76882a4

Add srcset test

03326ec

Add iimg_sizes test.

12a3201

Merge branch 'master' into responsive_image

7c04dab

Merge branch 'responsive_image' of https://github.com/maxxwv/timber i…

49a0fe9

…nto responsive_image * 'responsive_image' of https://github.com/maxxwv/timber:

jarednova added 3 commits November 9, 2018 20:01

Merge branch 'master' into responsive_image

945458e

Change spacing to match WP/Timber style

4b6cc1e

also some bonus `@codeCoverageIgnore`s on some deprecated functions to help juice code coverage scores

Merge branch 'master' into responsive_image

3676282

jarednova merged commit 83cf568 into timber:master Nov 10, 2018

maxxwv deleted the responsive_image branch December 2, 2018 05:02

maxxwv added a commit to maxxwv/timber that referenced this pull request Feb 16, 2019

Update documentation.

9dd3915

timber#1819 (comment)

maxxwv mentioned this pull request Feb 16, 2019

Update documentation. #1923

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Responsive image attributes #1819

Responsive image attributes #1819

maxxwv commented Oct 16, 2018

codecov bot commented Oct 16, 2018 •

edited

jarednova Oct 18, 2018

jarednova commented Oct 18, 2018

gchtr left a comment

gchtr Oct 18, 2018

gchtr Oct 18, 2018

gchtr Oct 18, 2018

gchtr Oct 18, 2018

maxxwv commented Oct 18, 2018

coveralls commented Oct 19, 2018

coveralls commented Oct 19, 2018 •

edited

maxxwv commented Oct 27, 2018

jarednova commented Nov 10, 2018

drzraf commented Feb 5, 2019

Responsive image attributes #1819

Responsive image attributes #1819

Conversation

maxxwv commented Oct 16, 2018

Issue

Solution

Impact

Usage Changes

Considerations

Testing

codecov bot commented Oct 16, 2018 • edited

Codecov Report

jarednova Oct 18, 2018

Choose a reason for hiding this comment

jarednova commented Oct 18, 2018

gchtr left a comment

Choose a reason for hiding this comment

gchtr Oct 18, 2018

Choose a reason for hiding this comment

gchtr Oct 18, 2018

Choose a reason for hiding this comment

gchtr Oct 18, 2018

Choose a reason for hiding this comment

gchtr Oct 18, 2018

Choose a reason for hiding this comment

maxxwv commented Oct 18, 2018

coveralls commented Oct 19, 2018

coveralls commented Oct 19, 2018 • edited

maxxwv commented Oct 27, 2018

jarednova commented Nov 10, 2018

drzraf commented Feb 5, 2019

codecov bot commented Oct 16, 2018 •

edited

coveralls commented Oct 19, 2018 •

edited