-
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
A proposal for the sizes attribute #34
Comments
@joemcgill thank you for the incredibly detailed and well though out post. I agree completely with this idea as the default option. @Wilto what are your thoughts? @joemcgill would there be any reason for a theme developer to want to override this proposed functionality, and if so, how would they do that? |
Thanks Tim, Giving developers a way to hook into and override the default functionality is something we would definitely need to consider. For content images (i.e. images inserted directly into posts through the editor) we could separate |
Sounds smart to me re: At one point I guessed that something like Thoughts:
|
@joemcgill But it seems there is still a slight misunderstanding regarding what changed about sizes: The browser itself will always go to |
@chriscoyier both good points. In terms of overriding/removing, I imagine we would be adding the I would also use different settings for featured images than I would use for content images, so we'd need separate filters or execution paths for those different use cases. Even so, I think the default logic could be the same for both. For example, featured images are often displayed in themes using a template tag like this:
We could use the value passed to |
I'd lean towards not having a UI live as part of this plugin. It's not a bad idea—it could be a useful thing to prototype/implement as a separate plugin, and could be presented as a dashboard or integrated throughout a site. However, many of the less-technical WordPress users I know already get flummoxed at the number of options and panels available within the UI. Better to let the customization be an opt-in, and to try to provide sane defaults and utilities for theme developers to use. |
@joemcgill, we could also probably set part of the I like your idea of splitting out srcset and sizes into two functions. I'm almost done with a first pass at how to incorporate the |
Good point. Maybe it would be useful to use |
For anyone wondering, the goal for this plugin is to enable responsive images without the need for a UI. That being said, a plugin separate from this one (using this plugin as a dependency), that introduces a UI in order to make managing responsive images easier, would be pretty interesting |
ok @joemcgill, @tevko, I made a pull request (#35) for my update. I added an
I created a couple of filter functions to give devs a way to hook into the template tag This gives us both a hook to modify the output, and useful settings to specify how to modify the output. |
Man, this is some great discussion—this is exactly how I was hoping things would play out once @tevko got the first iteration out the door. We gotta start with the idea that any sensible default we come up with here is only going to be “technically correct” the same way a broken clock is “technically correct” twice a day:
So long as we make it easy for the theme authors, we can say “hey, use valid markup; write |
Hi everyone! It looks like we've all been grinding away at some of the same issues. I was alerted to this project via Reddit, briefly corresponded with Tim, and figure I may as well see if I have anything to contribute to the discussion. Over the last several months I have implemented support for For an example of how this system of hooks interfaces with actual images in a theme have a look at this module. It's rather complicated but well-commented (a consequence of working through this without much of a reference and wanting to document everything for the benefit of others). I use a number of non-standard image layout conventions on my blog: full-width images, group images, ad hoc galleries of post thumbnails, etc. All of these are flex-width and, as such, required a fair amount of work to come up with something close to pixel-perfect values for the Anyhow, I just wanted to share this stuff to provide an example of a full and (I hope) working implementation of responsive images in a WP plugin/theme combination. I've been working on this stuff for my own use and don't have any specific plans to develop it into something for the main WP plugin repository so perhaps it will be of some help here. |
Not quite, the behavior is unchanged. It's just invalid markup to omit |
@zcorpan My mistake, thanks for clarifying. At any rate, the main point is that we need a default way of handling default values for the |
I really feel a little bit sorry about bringing the validity discussion up. For me this was just an unsubstantial formal argument. My main point is: Adding srcset with the width descriptor automatically without having sizes is mostly a bad thing. In case validity is the only thing that is a concern: It has to be said, that the spec didn't change that much. Omitting the The fact, that you have problems with this easy fix is that you didn't wanted this behavior before. You only quietened your concerns with the excuse: "We are not doing something explicitly wrong, we are just doing it implicitly wrong." And I must say, that all this discussion about this problem shows how huge and great this small spec change actually is, it forces developers to think about useful values inside the |
@joemcgill as discussed earlier, I'd like to move forward with the default sizes attribute. We also need to provide a responsible hook so that developers can customize the sizes attribute as needed. That hook would need to be done via filtering Another issue is concerning featured images which won't be found in |
@tevko, filtering In my pull request in #35, I added filters that worked in the scope of the Basically what happens is, those two functions start off by getting the srcset string, but then apply any filters that have been hooked in. That will probably be an easier way of hooking it in rather than |
After some thought, I have a proposal for how we might be able to incorporate the
sizes
attribute as it was meant to function in relation to the w descriptor insrcset
. At present, the plugin does not set anysizes
attribute on images because, by doing so, we would be giving the browser information about the intended layout width of the image relative to the page—information we do not have, and information which only the theme author (or someone implementing a theme) would know for certain.However, as has been expressed in #15, #21, #29, and elsewhere, not setting a
sizes
attribute currently tells browsers (or PictureFill) that the image will be displayed at 100% of the viewport, which will often mislead to the browser into choosing an image from the source set list that is much larger than what is actually needed. Particularly when the image is actually being displayed at a much smaller layout width than the full viewport width (e.g., tablets, laptop/desktop screens, etc.).For content images, we may not know the actual width of the image in a layout, but we might be able to assume that the maximum width of the image is the width of the original image selected (i.e., the one referenced by the
src
attribute) and build asizes
list accordingly. For example, if the original image that was chosen is 600px wide, we might be able to safely tell the browser that the image should never be larger than 600px wide. Or to put it in terms of asizes
attribute list, it would look like this:sizes="(min-width: 600px) 600px, 100vw"
To extend this example, let's say we upload a 3000px wide image and WordPress makes the standard soft crops: medium 300px and large 1024px. Now let's assume we've chosen to embed the medium size image in our post. Currently, the plugin would embed that image with the following markup (omitting a
sizes
attribute):If I were viewing this image on a non-retina (1x pixel density) tablet with a viewport width of 1024px, the browser would assume that the image should be 100% of the screen and choose to load the large, 1024px image, when in fact, it should have chosen the 300px version. On a larger retina screen, we very well might get served the 3000px image when the 1024px version would be much more than adequate.
By adding a
sizes
attribute such that the first source size in the source size list matches(min-width: {{chosen-size}}) {{chosen-size}}, 100vw
the markup from above would now be:In our above examples, the tablet would choose the 300px wide image and the retina screen would choose the 1024px wide image, a considerable bandwidth savings in both cases.
Given that the RICG has as recently as 10 days ago changed the proposed spec such that a
srcset
list using w descriptors without a correspondingsizes
attribute would not assume a source size of 100vw but instead do nothing, as @aFarkas rightly pointed out in #29 (comment), we may end up needing to deal with thesizes
attribute in a more explicit way no matter what.I think this may be a sensible approach, but I'd love to get feedback before trudging down this path in case there be dragons of which I'm unaware. Thanks!
The text was updated successfully, but these errors were encountered: