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

<picture> support with presets #421

Closed
alexey-pelykh opened this issue Oct 6, 2017 · 32 comments
Closed

<picture> support with presets #421

alexey-pelykh opened this issue Oct 6, 2017 · 32 comments

Comments

@alexey-pelykh
Copy link
Contributor

@envygeeks would you be interested in PR for adding picture tag support in form of

{% picture path/to/img.png alt:'some text' preset:layout1 %}

that will produce something like

<picture>
  <source media=“(min-width: 420px)” srcset=“{% asset_path path/to/img.png ...options-from-preset... %}, {% asset_path path/to/img.png ...options-for-retina... %} 2x”>
  <img src="{% asset_path path/to/img.png %}" alt="some text">
</picture>

according to the preset specified?

@envygeeks
Copy link
Owner

@alexey-pelykh I would wait till Monday (well realistically Tuesday) to do this (or I can do this for you) as Jekyll-Assets is currently under going a full rewrite and cleanup for 3.0 in the sprockes-4.x branch and any new features are being put on hold for the rewrite.

@envygeeks
Copy link
Owner

The reason I say this is because today I've entirely rewritten our tag system, it's 1000x simpler, uses encapsulated and extensible tags (that even users can start to use to extend Jekyll assets) and more, so doing this effort right now will just force more work to be done for both parties.

@alexey-pelykh
Copy link
Contributor Author

Gotcha! No rush then 👍

@envygeeks envygeeks modified the milestones: v3.0.0, v3.1.0 Oct 8, 2017
@envygeeks
Copy link
Owner

envygeeks commented Oct 12, 2017

{% asset img.svg srcset="magick:double media=\"(min-width:600px)\""
                 srcset="magick:resize=2400x2400 media=\"(min-width:1200px)\""
                 srcset="magick:half media=\"(min-width:400px)\"" %}

@envygeeks
Copy link
Owner

envygeeks commented Oct 12, 2017

Notes:

  • There is a bug in Liquid::Tag::Parser where array's are built wrong.
[30] pry(main)> Liquid::Tag::Parser.new('img.svg srcset=1 srcset=2 srcset=3')
=> #<Liquid::Tag::Parser:0x0000000003fd3330
 @args={:argv1=>"img.svg", :srcset=>[[1, 2, 3], 3]},
 @defaults={},
 @escaped_sep="\\=",
 @escaped_sep_regexp=/\\=/,
 @raw="img.svg srcset=1 srcset=2 srcset=3",
 @rsep="=",
 @sep="=",
 @sep_regexp=/\b(?<!\\)=/>
  • We need to escape quoted @ and quoted ! inside of Proxies.
  • Jekyll-Assets tag needs to be adjusted so that it accepts multiple assets and loops on them.

@envygeeks
Copy link
Owner

This should be going out this morning or afternoon. The concept is completed.

@nhoizey
Copy link
Contributor

nhoizey commented Oct 13, 2017

I have a few comments about this example:

{% asset img.svg srcset="magick:double media=\"(min-width:600px)\""
                 srcset="magick:resize=2400x2400 media=\"(min-width:1200px)\""
                 srcset="magick:half media=\"(min-width:400px)\"" %}

First, img.svg looks like an SVG, which means it doesn't need anything to be responsive… ;-)

Then, judging just from this, it looks like you mixed srcset and <picture>'s <source>, but it's difficult to know without the generated HTML.

@envygeeks
Copy link
Owner

envygeeks commented Oct 13, 2017

You do realize <source> uses srcset when it's inside of <picture>, right?

@envygeeks
Copy link
Owner

This is fixed in 3.x.

@envygeeks envygeeks modified the milestones: v3.1.0, v3.0.0 Oct 13, 2017
@nhoizey
Copy link
Contributor

nhoizey commented Oct 13, 2017

@envygeeks yes, I know <source> uses srcset, of course. I agree my question was not well thought, sorry.

Now I've read this commit, I still have some questions (don't take them as criticism, it's not what I want, maybe my english is not good enough):

  • You seem to support srcset in <source> but not sizes, which means it works only for images that take the full width of the viewport. Is it planed for a future release?
  • Do you plan to also support the simpler srcset/sizes attributes in <img>? As I mentioned in this comment two years ago, <picture> should be avoided most of the time.

Thanks for you answers, and really sorry if I annoy you.

@envygeeks
Copy link
Owner

No, I do not plan to support <img srcset> (unless somebody is willing to sponsor the feature. This rewrite has already consumed quite a bit of my time as it stands. Far more time than the donations I've received to update it.) There are also no plans to implement sizes, or pixel density on <source> either.

As far as that article claiming that <picture> is bad; that persons examples are either tailored towards making it seem like <picture> is bad, or there is a misunderstanding of how <picture> and <source> work as there is one <source> per picture. Rather than grouping all his sources, as well <source> supports the same srcset spec as <img> (As far as Chrome, Edge, Firefox 33+, and AFAIW even Safari.) (Note: I just skimmed the article .)

In the future there might be plans, but I don't know.

@alexey-pelykh
Copy link
Contributor Author

I'll plan to add support for sizes and density on <source> sometime soon then. Anyhow we'll need that for our website

@nhoizey
Copy link
Contributor

nhoizey commented Oct 13, 2017

@envygeeks the main issue with <picture> comes from the media attribute in <source>, which set contraints that we often don't have, leading to incorrect uses.

<img srcset> is easier to use, enough most of the time, and less risky.

I obviously understand your point about the time (and money) it took you to work on this. I'm really thankful, using this plugin for a while in my Jekyll site.

@envygeeks
Copy link
Owner

As I tell everybody, it's not about the money even though it does cost a lot of time (and thus money) and I've started to encourage people to donate, even $10 helps (that's an entire pitcher of beer made here locally where I live,) it's not about the size of the donation, it's about the act. Thanks to Github I've recently discovered that some fairly large and popular projects from fairly rich companies use quite a few of my repositories (projects) without starring them, or even bothering to try and help support them. Which often leaves me as the sole maintainer because they don't get discovered by the masses. So I've started encouraging people to help me help them.

@envygeeks envygeeks reopened this Oct 13, 2017
@grigs
Copy link

grigs commented Oct 16, 2017

As far as that article claiming that is bad; that persons examples are either tailored towards making it seem like is bad, or there is a misunderstanding of how and work as there is one per picture. Rather than grouping all his sources, as well supports the same srcset spec as (As far as Chrome, Edge, Firefox 33+, and AFAIW even Safari.) (Note: I just skimmed the article .)

I'm that person. I also helped write the spec for picture and srcset. I know what I'm talking about in that article. :)

None of that really makes a lick of difference if you don't have time to implement changes.

But the points in the article that @nhoizey cited are still valid. The vast majority of responsive image usage—our estimates in the RICG were 90%—are better suited for srcset and sizes than the picture element.

Anyways, good luck with your rewrite. Sounds like you've got a lot on your plate. :)

@nhoizey
Copy link
Contributor

nhoizey commented Oct 16, 2017

Thanks @grigs for stopping by and giving more information in favor of srcset!

@envygeeks I'm quite busy right now, but I will try ASAP to understand your code and work on a PR to support srcset/sizes in <img>.

@envygeeks
Copy link
Owner

envygeeks commented Oct 16, 2017

Hey @grigs @nhoizey

They are both to be implemented, <img srcset> and the changes to <source srcset>. I think I added them to the list of TODO's before I make my final release. Including the density and w bits. They've been partially implemented since Friday in a stash and should go out into master on within a few days with some major changes to support SourceMaps v3 properly (hopefully... I still don't quite understand SourceMaps... I need to reread the spec.)

After having thought about it in our discussion a bit I concluded that everyone was probably right, and it's best I probably implement it now when I can adjust everything around it to support these types of things, rather than hacking it in later creating more complexity for myself.

@nhoizey
Copy link
Contributor

nhoizey commented Oct 16, 2017

@envygeeks thanks a lot for your work. Again, feel free to ask for help if something is not clear in the specification, or for tests.

@envygeeks
Copy link
Owner

The specs are pretty clear (I think, I've been wrong about them before,) the only thing that confused me was one <source> per <picture>. It's still not quite clear if that's a recommended use-case, or if I was delusional and I read the examples wrong (quite likely as I just skimmed... hopefully I already mentioned that.) I don't know that I would support that.

I will start encouraging people to start using the alpha this week once I finish my last round of super refactors (cleanups that happen after I take a few days after writing something, go back and read it and decide if I didn't know what I was doing and either rewrite it, or consider it production ready.)

@grigs
Copy link

grigs commented Oct 16, 2017

Cool. Thanks @envygeeks. You're in good hands with @nhoizey who has been following responsive images pretty closely, but I'm also available if you have questions. We're always keen to help people with responsive images.

@envygeeks
Copy link
Owner

I do have one question regarding all this, does <img> need to come first in <picture>, should we recommend max-width or min-width and on that query have to be ordered a specific way?

@nhoizey
Copy link
Contributor

nhoizey commented Nov 6, 2017

@envygeeks I suppose your question only relates to <picture>/<source>, not <img srcset>:

<img> doesn't need to come first in <picture>, but the order of various <source>s is important, because the first one that matches (Media Query and/or content type) is used to "update" the src of the <img>.

Regarding min-width or max-width, I'm more inclined to use min-width because it helps define breakpoints in a Mobile First way, like in my CSS. The "weird" thing is that these Media Queries then have to be written from the larger to the smaller, the last one (the smallest) being the <img> without any need for an additional <source>.

Here's a simple exemple with arbitrary sizes, where the image goes from landscape to square and then portrait while the viewport gets smaller, is always full viewport width, and there are just 2 image per srcset:

<picture>
  <source media="(min-width: 1000px)" srcset="landscape-1000.jpg 1000w, landscape-2000.jpg 2000w">
  <source media="(min-width: 600px)" srcset="square-600.jpg 600w, square-1200.jpg 1200w.jpg">
  <img src="fallback.jpg" srcset="portrait-300.jpg 300w, portrait-600.jpg 600w">
</picture>

With no Art Direction, and an image always in landscape ratio (for example), it would be much easier:

<img src="fallback.jpg" srcset="landscape-300.jpg 300w, landscape-600.jpg 600w, landscape-900.jpg 900w, landscape-1200.jpg 1200w, landscape-1500.jpg 1500w">

Please correct me @grigs if I'm wrong… ;-)

@grigs
Copy link

grigs commented Nov 6, 2017

That's mostly correct, but any time you use width descriptors, you must use sizes as well even if all you do is sizes="100vw".

@envygeeks
Copy link
Owner

Where does sizes need to be placed? On <img>, <picture>, or <source>, and does it only apply to <picture> or does it apply to <img srcset> too.

Sorry for the all the questions, I'm working out our implementation this week and somebody filed a bunch of tickets on our current one and it's examples. So I'm trying to do it right the first time and I've only briefly read the spec.

@nhoizey
Copy link
Contributor

nhoizey commented Nov 6, 2017

@grigs oh yes, you're right. I'm the first telling this to others, and this time I forgot… :-D

@envygeeks sizes needs to be placed on <source> and <img> every time there's a w descriptor in its srcset.

Updated examples:

<picture>
  <source media="(min-width: 1000px)" srcset="landscape-1000.jpg 1000w, landscape-2000.jpg 2000w" sizes="100vw">
  <source media="(min-width: 600px)" srcset="square-600.jpg 600w, square-1200.jpg 1200w.jpg" sizes="100vw">
  <img src="fallback.jpg" srcset="portrait-300.jpg 300w, portrait-600.jpg 600w" sizes="100vw">
</picture>

And

<img
  src="fallback.jpg"
  srcset="landscape-300.jpg 300w, landscape-600.jpg 600w, landscape-900.jpg 900w, landscape-1200.jpg 1200w, landscape-1500.jpg 1500w"
  sizes="100vw">

If the image is not always full width, sizes has to be adapted.

Example without Art Direction but the image going from full width on small screen to two columns on intermediate size screens and three columns on larger screens:

<img
  src="fallback.jpg"
  srcset="landscape-300.jpg 300w, landscape-600.jpg 600w, landscape-900.jpg 900w, landscape-1200.jpg 1200w, landscape-1500.jpg 1500w"
  sizes="(min-width: 1000px) 33vw, (min-width: 600px) 50vw, 100vw">

33vw and 50vw are not really accurate because there will often be some space between the columns (the gutters), but I think it's enough to understand how it works.

In my jekyll-cloudinary plugin, I let the user define the full sizes attribute for each preset.

HTH

@envygeeks
Copy link
Owner

Jekyll Assets doesn't stop people from setting size manually, we can default it though.

@nhoizey
Copy link
Contributor

nhoizey commented Nov 6, 2017

The thing is that I wonder how Jekyll Assets would be able to guess the right sizes value, and it can have a great impact on the right image selection.

@envygeeks
Copy link
Owner

There is an assumption that this software is going to try and guess, it won't. You'll either supply it or we'll set it to "100vw" and let you deal with the fallout. We can't/won't do everything for you. We won't even verify that it's right on your behalf because we are not your frontend, or an HTML/CSS processor. What happens after you don't set it and we let your mistake slip through is between you and the browser.

@nhoizey
Copy link
Contributor

nhoizey commented Nov 6, 2017

Ok, so we do totally agree, I thought you had something else than 100vw in mind. ;-)

@envygeeks
Copy link
Owner

The final push is happening this weekend, this software is production ready, and is waiting for the last few pieces of this ticket to happen. My sites already moved over to this software, and I've worked out a few production bugs through it. It'll probably go out early next week.

@envygeeks
Copy link
Owner

It just dawned on me as I was working on this to release this morning, that we have always supported sizes, media, and any other attribute, known or not. We just don't have them defaulted in the HTML default system. The way our HTML builder is designed is that it parses the arguments, we decide if it's internal, snatch it up if it is, block it's output if it's internal, and then anything else gets passed back to your HTML output as an attribute unless it's an array, or hash, or negative boolean, if it's positive we output as an attribute without a value, and since srcset= creates a loopable hash, when our internal stuff loops, it takes that argument hash (which is unique to each loop) and passes it along. You can even have unique attributes per, if you really want.

{% asset img.png @pic
    srcset='magick:resize=800 sizes="(min-width:800px), 800px, 100vw" id=1'
    srcset='magick:resize=600 sizes="(min-width:600px), 600px, 100vw" id=2'
    srcset='magick:resize=400 sizes="(min-width:400px), 400px, 100vw" id=3'
      %}

And makes it

<picture>
  <source src=/assets/img-hash.png sizes="(min-width:800px), 800px, 100vw" id=1>
  <source src=/assets/img-hash.png sizes="(min-width:600px), 600px, 100vw" id=2>
  <source src=/assets/img-hash.png sizes="(min-width:800px), 400px, 100vw" id=3>
  <img src=/assets/img-hash.png>
</picture>

You can read more about my parser here: https://github.com/envygeeks/liquid-tag-parser


So really the only thing I need to do at this point, is detect the size and default to a cheap default, if your image is 800px I'll set sizes=(min-width: 800px), 800px, 100vw, and if you send your own, I won't bother to validate, correct, or alter it, because we aren't an HTML validator, and we don't need the value, but in the future we might use some part of it, maybe in 3.1 or 3.0.1 if somebody files a ticket, I'll parse it and search for a 800px and kick in an automatic magick:resize for you.

@stanmots
Copy link

stanmots commented Feb 8, 2018

I see the issue is closed, but after reading through the all topics related to presets for responsive images I'm still confused. Is this feature supported in the latest versions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants