-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
Comments
@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 |
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. |
Gotcha! No rush then 👍 |
{% 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)\"" %} |
Notes:
[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(?<!\\)=/>
|
This should be going out this morning or afternoon. The concept is completed. |
I have a few comments about this example:
First, Then, judging just from this, it looks like you mixed |
You do realize |
This is fixed in 3.x. |
@envygeeks yes, I know 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):
Thanks for you answers, and really sorry if I annoy you. |
No, I do not plan to support As far as that article claiming that In the future there might be plans, but I don't know. |
I'll plan to add support for sizes and density on |
@envygeeks the main issue with
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. |
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. |
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. :) |
Thanks @grigs for stopping by and giving more information in favor of @envygeeks I'm quite busy right now, but I will try ASAP to understand your code and work on a PR to support |
They are both to be implemented, 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. |
@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. |
The specs are pretty clear (I think, I've been wrong about them before,) the only thing that confused me was one 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.) |
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. |
I do have one question regarding all this, does |
@envygeeks I suppose your question only relates to
Regarding 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 <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… ;-) |
That's mostly correct, but any time you use width descriptors, you must use |
Where does 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. |
@grigs oh yes, you're right. I'm the first telling this to others, and this time I forgot… :-D @envygeeks 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, 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">
In my jekyll-cloudinary plugin, I let the user define the full HTH |
Jekyll Assets doesn't stop people from setting size manually, we can default it though. |
The thing is that I wonder how Jekyll Assets would be able to guess the right |
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. |
Ok, so we do totally agree, I thought you had something else than |
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. |
It just dawned on me as I was working on this to release this morning, that we have always supported {% 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 |
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? |
@envygeeks would you be interested in PR for adding
picture
tag support in form ofthat will produce something like
according to the preset specified?
The text was updated successfully, but these errors were encountered: