-
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
Add support for the sizes attribute. #51
Conversation
|
||
foreach( $args['sizes'] as $size ) { | ||
// Use 100vm as the size value unless something else is specified. | ||
$size_value = ( $size['size_value'] ) ? $size['size_value'] : '100vm'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be vw no? vm units don't seem to have great support http://caniuse.com/#search=vm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got to stop coding upside down.
One thing to consider is whether We could abstract the attribute strings into their own functions—
Thoughts? |
FWIW, over in Slack, @pento just recommended using data-attributes as well. https://wordpress.slack.com/archives/feature-respimg/p1423993863000283 |
I've made a change to this PR that separates the creation of the |
that's why i would completely leave it in the theme developer's hands. they know their switches and sizes and can easily add the sizes attribute by filtering the_content. they can put whatever value they use to set the image witdh - rem, em, vw... - in their own filter. no need to write to the user database and everything is fine.
you assume that the editor image width is the highest needed width... this way it would be impossible to add bigger images for higher resolutions in the layout. |
Hi @side777. Generally, I am leaving the sizes attribute in theme developers hands, I'm just setting a default |
that's good to know!
so, why not leave the database part out and only add sizes by searching the content for srcset? |
Not a bad idea. By setting a data-sizes attribute, we leave room for content creators to modify the sizes attribute on an image specific basis (by editing the HTML in the editor). Should we retain that ability? |
a check for existing sizes attribute (and srcset btw!) would solve this... now that i think about that - should the srcset be added via the_content too? i think about a scenario where the image dimensions get changed by user or theme... |
This would require running extra database queries to build the srcset arrays at runtime, which is probably a bad idea. In reality, the only way these should change is if someone regenerates all of their thumbnails. Otherwise, changing image sizes only affect images uploaded after the change. |
Now that #56 has landed, I'm going to write tests for this before we merge it. |
4e2288e
to
06d01a0
Compare
06d01a0
to
22831d2
Compare
I've refreshed this patch and have added tests. In the process, I found a potential issue with the way the tests for srcset were written which could cause them to fail under certain conditions and have supplied a patch for that issue in #58. There's no need to merge that first, but it would be nice to get them both in around the same time. For simplicity sake, I've squashed this change down to a single commit, but have left the change history in a separate branch in my fork of the repo in case you're interested (https://github.com/joemcgill/wp-tevko-responsive-images/compare/feature/sizes-wip). I'll leave that branch open until this has been merged so I can go back to it if needed. |
Add support for the sizes attribute.
Here's an initial candidate for how we can handle default support for the
sizes
attribute in this plugin.Background
As discussed in #34, we need a way to set a smart default value for the
sizes
attribute when we are including a source set list using w descriptors. Otherwise, we run the risk of fooling the browser into downloading larger resources than necessary by assuming the image layout width is equal to 100% of the viewport width (among other issues). For the purpose of this pull request, I'm using the following pattern as the default forsizes
:In other words, when the viewport is less than or equal to the width of the main image, assume the image takes up the full screen width. If the viewport is greater than the width of the image, just use the width of the image itself as the layout width.
The case for
data-sizes
For this implementation, I've opted to include the sizes list as a
data-sizes
attribute instead of asizes
attribute directly. Here are my reasons:First, It's important to remember that this plugin works by adding markup directly to the image HTML element before it's inserted into the editor, so we need to be careful about what we're saving to user's databases. Since
srcset
lists are unlikely to change over time and the attribute is now part of the HTML spec, we're probably safe there. On the other hand, thesizes
attribute is completely dependent on the site's layout, which is theme territory and something we should be very careful about saving into users' databases.Also, as of this writing, the version of TinyMCE that is bundled in WordPress core does not include support for the
sizes
attribute on image elements, so even if we did addsizes
to images, TinyMCE would just strip them right off. We can extend TinyMCE, and I've included a filter which does just that, but it currently breaks the wp.media view so I have it commented out for now. 👎I've added new filters to
the_content
andpost_thumbnail_html
that simply looks for images withdata-sizes
attributes and change them tosizes
attributes instead.While I'll be the first to admit that this approach isn't perfect, I see several benefits this strategy. Primarily, it allows us to deliver a default sizes attribute out of the box, which is a nice win. Developers will have the flexibility to alter the filters to their own liking in order to improve the accuracy of the sizes attributes without writing a bunch of code, which is pretty nice. Maybe most importantly, saving data attributes to the post data instead of the
sizes
attribute buys us some flexibility while we work out better ways of handling this in the future.