-
Notifications
You must be signed in to change notification settings - Fork 53
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
Added args for sizes, aspect ratio, included/excluded sizes. #35
Conversation
…e, and included/excluded sizes
@jhned a lot of good thinking here. I've got some questions, mainly about the use of the This looks to me to be trying to solve the art direction use case instead of simple source switching based on the viewport size or pixel density of the screen. If that's the case, I believe that art direction should be handled by using the I like the idea of adding arguments that allow developers to be more selective about which image sizes get included/excluded. Would you be open to shortening the argument names to |
@joemcgill, that's true, I was trying to provide for an art direction use case with the If it's all the same to everybody else, I would prefer to keep it in, because Here's a beneficial use case: say you had a 16:9 featured image on desktop, but you want that featured image to be 3:2 on mobile. Technically that's art direction, since you're not just doing simple source switching. But if you didn't do that, the featured image would be about 50px tall on mobile. And of course, shortening the names of the arguments is peachy. (Do I do that before the merge, or do we do that after? If I do it, should I create a new pull request? I'm still relatively new to Github processes) I also updated a couple of function and variable names in the process, to keep the code clear. We may need to update them as well. |
@jhned An example: Someone uses your aspect_ratio option and only tests on a 1x device. He will like the fact, that on a small screen (less than 500px) another image is used. As soon as he tests this with his mobile device the result is totally different, than he has expected it. Although he only has 320-400 layout pixels the wrong image for his layout is used (because 320 - 400 is multiplied with 2). Additionally currently the srcset selection by browser is quite predictable. Chrome uses something like the nearest match for the required resolution, but don't expect that this will be always the case in the future. About picture support. This is really something, that can be indirectly already achieved by using the get srcset API. |
oh by the way in chrome 40+ is already some code, which make it less predictable. In case an image candidate is in cache and has enough resolution this images is chosen. |
Ok, I can take the aspect ratio argument out. @joemcgill, is there anything else besides that and the argument names I should change in order for the pull request to be accepted? I saw that @tevko changed |
I haven't tested the code, but in principle I would be cool with the PR once the aspect ratio arg is stripped and the others are renamed. @tevko will have to pull the trigger, so he can better speak to the function names. Thanks again! |
Ok, good deal. @tevko, do you have a preference on the functions? |
I was looking over this tonight after spending some time thinking about how to best deliver the For example, I'd like to see us separate the functionality that sets up the Even so, this is a useful proof of concept to help us get the ball moving in that direction. |
I'd agree with that. The more flexible the code is, the better. |
@jhned thank you for this, it is very helpful. I will close this for now as we regroup and plan for the next iteration of the plugin. More than likely some of this will still make it into the core plugin as we will definitely be using this as a reference |
Like @joemcgill suggested, I added support for
$args
to thetevkori_get_src_sizes
function. I also added a few more options:maintain_aspect_ratio
is used during the $default_sizes loop: if set to true, it executes the aspect ratio (or crop) check.included_sizes
allows a developer to specify which WordPress image sizes to include in thesrcset
string.excluded_sizes
allows a developer to specify which WordPress image sizes to exclude from thesrcset
string.In order to give a developer a way to hook into the srcset/sizes function, I added two filter functions:
tevkori_apply_content_src_filter
andtevkori_apply_post_thumbnail_src_filter
, whichtevkori_extend_image_tag
andtevkori_filter_post_thumbnail_html
use, respectively. They can probably be condensed, but it's late and I'm tired.The two filter functions first run the
tevkori_get_src_sizes
function, and then apply any filters added totevkori_get_content_src_sizes_filter
andtevkori_get_post_thumbnail_src_sizes_filter
.Here's an example of modifying content images:
And modifying post thumbnails:
Here's my full debug code that I used when testing: