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.

Sign up for GitHub

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

Jump to bottom

Add apos.images.srcset method #805

Merged
merged 10 commits into from Jun 11, 2018
Merged

Add apos.images.srcset method #805

merged 10 commits into from Jun 11, 2018

Conversation

fredrikekelund
Copy link
Contributor

Here's the apos.attachments.srcset method (with an accompanying test) discussed in #803. The srcset attribute is fairly simple, so there's not much to it. I changed my mind on the naming of the sizes attribute passed to apostrophe-images-widgets, though, so it's now sizesAttr. It seemed like this would cause less confusion with the existing size option.

One thing that I think warrants some discussion is the possible inclusion of the picturefill.js polyfill. The only browser that doesn't support srcset is IE, but picturefill.js really just works and weighs in at a pretty small 11.5 kB when minified. Maybe this is the kind of thing better left to individual developers (perhaps the possibility of including it could be mentioned in the docs), but I wanted to throw it out there.

I'll go ahead and submit a PR to the docs for this as well.

Btw, it looks like some trailing whitespace was removed automatically by my editor, but I didn't think that caused any harm.

@fredrikekelund
Added apos.attachments.srcset method
7dc74c7 
Short and sweet, it mostly serves as a convenience, but it can be
very convenient indeed.
@fredrikekelund
Render srcset and sizes attribute in apostrophe-images-widgets
20374a0 
This change also introduces the `sizesAttr` option that can be
passed to the apostrophe-images-widgets module to set the sizes
attribute of the resulting <img> element/-s. That attribute
defaults to '100vw'.
@fredrikekelund fredrikekelund mentioned this pull request Jan 26, 2017
Merged
@fredrikekelund
Copy link
Contributor Author

@boutell @austinstarin thoughts on this? I realize that while apos.attachments.src serves the same purpose for all types of attachments, this method is only applicable to images. Perhaps then it'd make more sense to move it to apostrophe-images?

@boutell
Copy link
Member

boutell commented Feb 20, 2017 via email

@fredrikekelund
Copy link
Contributor Author

Cool, I'll have a look at moving it!

@fredrikekelund
Move srcset method to apostrophe-images module
efc1da9 
It makes sense to place it there rather than in
apostrophe-attachments, since srcset attributes can't be calculated
for other types of attachments.
@fredrikekelund
Merge branch 'master' into add-attachments-srcset-method
84c6a6c
@fredrikekelund fredrikekelund changed the title Add apos.attachments.srcset method Add apos.images.srcset method Mar 3, 2017
@fredrikekelund
Copy link
Contributor Author

@boutell moved! It's now apos.images.srcset instead of apos.attachments.srcset. Let me know if you think we need any more changes here

@boutell
Copy link
Member

boutell commented Mar 3, 2017

Hi Fredrik,

We had a long discussion about this just now. We think it is a very, very cool idea but unfortunately there are some significant issues with it.

First, the code assumes that "sizes" should be set to 100vw. This works out pretty well on a phone, because generally any columns etc. have collapsed to one column in that setting. So the image that gets picked matches the device size and we're good.

But on a desktop it's bad news, because 100vw will be very large, even if this image widget is actually going into one of five columns and should therefore be very small. As a result the max image is picked every time, in exactly the situation where you wish it wouldn't be.

We could add a sizes option, requiring the developer to think about how big this thing is really going to be on the page at various breakpoints, and output srcsetonly if it is present. But then we're still making the assumption that sending a double-res image for retina screens is not worth the trouble. And that's not really an assumption we get to push on frontend developers.

Finally, and this is arguably our fault, although some shaping and styling tricks are very difficult any other way... we use CSS background images a lot. In that scenario the img element is pretty much there for SEO. And when that is happening the srcset doesn't have much use.

@austinstarin just showed me an example of a client project where he built his own figure element using srcset, et cetera, and the standard apos.attachments.url helper. It works well and doesn't seem to require any customization in the core.

So for now this is not something we're going to do. But we're open to an ongoing conversation about what we might be able to do to help frontend developers use srcset effectively without making unsafe assumptions about their use cases.

Thanks and please continue sending PRs!

@boutell boutell closed this Mar 3, 2017
@fredrikekelund
Copy link
Contributor Author

Thanks for the detailed feedback, feels good to engage in an open source project with such a positive attitude!

As for the points you bring up - I see the issue with the 100vw default value for sizes in apostrophe-images-widgets. I think we could come up with a better solution there by following WordPress' example. They use the following sizes attribute by default: (max-width: {{image-width}}px) 100vw, {{image-width}}px. I guess we would grab the image-width value from the size attribute passed in the Nunjucks template.

However, if you still feel that it's not worth including the apostrophe-images-widgets markup, then I won't fight for it. I think it would be neat to have, but I also believe people will be writing their own custom image modules for different scenarios, and can then decide for themselves whether or not to include those attributes.

But I do feel more strongly about including the apos.images.srcset method (and Nunjucks helper). srcset is a really important feature for a lot of modern responsive websites, and merging just that method is a small addition to acknowledge that and make life a little easier for developers.

So, what we could do is to drop the changes in apostrophe-images-widgets and just add the apos.images.srcset method. What do you say, @boutell? :)

@boutell
Copy link
Member

boutell commented Mar 5, 2017

Hmm, both suggestions make sense to me... is there a straightforward way to add an option for those who want to specify "I'll pay the extra freight to go 2x on retina"?

@fredrikekelund
Copy link
Contributor Author

The retina stuff is actually handled automatically by browsers! The srcset attribute has gone through one or two revisions since it was introduced, and the 2x syntax is no longer needed. And I don't really know of any way to turn off the retina consideration - I think browsers will try to pick the picture that will look best within some confines. For example, if your viewport is 1000px wide and you have two image options - one 800px and one 2000px, I believe most browsers will pick the smaller one, because 2000px is deemed unreasonably large to justify a download.

@boutell
Copy link
Member

boutell commented Mar 5, 2017 via email

@boutell
Copy link
Member

boutell commented Mar 8, 2017

Consensus here is that your revised suggestions make good sense! Do you want to take a crack at these changes? The dimensions for each named size are available in the attachments module.

@fredrikekelund
Copy link
Contributor Author

Great – happy to hear it! Absolutely, I'll have time to look into it in a day or two. Do you want to reopen the PR in the meantime? :)

@boutell boutell reopened this Mar 8, 2017
@fredrikekelund
Update default sizes attribute for <img>'s in apostrophe-images-widgets
60d99c8 
Follow the same pattern that WordPress does, ie.
(max-width: {{image-width}}px) 100vw, {{image-width}}px

This gives us sharp images, but also makes sure that users on desktop won't
have to download bigger images than with previous Apostrophe versions.

Also updated CSS after having tested the widget - background images are no
longer used on .apos-slideshow-item's. If we continue to use that, we won't
see any benefits from the srcset implementation.
@fredrikekelund
Preserve image cropping ratios for srcset variations as well
6e7f542
@fredrikekelund
Copy link
Contributor Author

There we are! I've updated the default value of the <img>'s sizes attribute in apostrophe-image-widgets.

I did some testing as well and concluded that we have to drop the current background-image solution on the .apos-slideshow-items for srcset to be effective, so I did that and updated the CSS accordingly. I did some manual testing besides the unit test I added for the new apos.images.srcset method, and things look to be working well both for single images and slideshows, but please do have an extra look at the diff before merging to make sure I don't break any of your assumptions in apostrophe-images-widgets.

@boutell
Copy link
Member

boutell commented Mar 10, 2017 via email

@fredrikekelund
Copy link
Contributor Author

Hmm well that's a problem then. I guess we could hide the new behavior behind a widget option (eg. srcset: true) but still default to the old behavior. Alternatively we could drop the updates to apostrophe-images-widgets and just add the apos.images.srcset method. The former option seems viable to me, I don't know how you feel about it?

@fredrikekelund
Copy link
Contributor Author

After having mulled this over a bit, I think that the added complexity we would introduce by adding a srcset: true widget option to apostrophe-images-widgets would probably give us more trouble than it's worth. My suggestion is that I reduce the changes to just the apos.images.srcset method and squash my commits here. Does that sound like a good way forward, @boutell?

@fredrikekelund
Copy link
Contributor Author

@stuartromanek @bgantick @austinstarin, I'm coming back to this now and am still thinking like my last comment. If P'unk Avenue relies on using background images over <img> elements, then the best way forward would probably be to drop the updated CSS and markup in apostrophe-images-widgets, and instead just add the apos.images.srcset and the apos.attachments.getImageSize methods.

I can't stress enough the value that this underlying functionality brings - for websites that demand ease of content editing, good performance for end users and high image quality - Apostrophe might actually have it all. The image variations that are already there just need to be exposed through some template functions (and those template functions need to be documented), and voíla!

@boutell
Copy link
Member

boutell commented Nov 8, 2017 via email

@stuartromanek
Copy link
Member

We should definitely be moving this direction in Apostrophe (and move away from background images as a default). Thanks for the hard work @fredrikekelund

@fredrikekelund can you take a crack at re-writing the first part of this document to go over the features? https://github.com/punkave/apostrophe-documentation/blob/master/tutorials/howtos/responsive-images.md

@fredrikekelund
Copy link
Contributor Author

@stuartromanek sure! Will see if I can look into it over the next couple of days. I assume you don't want me to make any further changes to apostrophe-images-widgets then?

@stuartromanek
Copy link
Member

Not at this time @fredrikekelund .. next week I want to round up with a few other Apostrophe devs to go over internally, will let you know how that shakes out.

@fredrikekelund
Copy link
Contributor Author

@stuartromanek @boutell I actually went ahead and reverted all of the changes to apostrophe-images-widgets - making it so the only changes here are the introduction of the apos.images.srcset and apos.attachments.getImageSize methods.

I think it's more valuable to merge those methods, showing clearly that Apostrophe supports srcset more or less out of the box, than it is to find a way forward going from a background-image to an <img> solution.

Happy to add documentation about those methods in the tutorial chapter that you mentioned @stuartromanek, if you like?

@fredrikekelund
Copy link
Contributor Author

To make sure we don't forget: we should close this PR once we merge this as well apostrophecms/apostrophe-documentation#45

@fredrikekelund fredrikekelund mentioned this pull request Jan 22, 2018
Open
@fredrikekelund
Copy link
Contributor Author

Bumping this PR again @stuartromanek and @boutell 😇

As a quick recap: we went around the whole background-image discussion by limiting the scope of this PR to simply adding a apos.images.srcset method.

apos.attachments.getImageSize was added in 60d99c8 to make it simple to implement the same default sizes attribute as WordPress does ((max-width: {{ imageSize.width }}px) 100vw, {{ imageSize.width }}px). I'm not 100% sure we need to keep that method, but it could be helpful, I suppose.

@boutell
Copy link
Member

boutell commented Jun 11, 2018

@stuartromanek given that this doesn't change the standard behavior of our widgets, do you see any objection to merging it? We could then provide a HOWTO on how to do srcset at project level.

@fredrikekelund it sounds like getImageSize has no remaining application here?

@stuartromanek
Copy link
Member

No objection here, I'd like a second look from some of the other Apostrophe front-end'ers who might be implementing this sort of behavior project-by-project and see if it satisfies their normal requirements.

@bgantick @abea @austinstarin

@fredrikekelund
Copy link
Contributor Author

@boutell I removed the apos.attachments.getImageSize template helper. If we realise at a later point that it would be helpful, we can bring it back – but right now it seemed mostly like a relic.

I also merged the latest master, so the branch should be mergeable now!

@stuartromanek stuartromanek self-requested a review June 11, 2018 18:47
@stuartromanek stuartromanek merged commit 43572ff into apostrophecms:master Jun 11, 2018
@stuartromanek
Copy link
Member

merged

@fredrikekelund
Copy link
Contributor Author

🙌

fredrikekelund added a commit to fredrikekelund/apostrophe-documentation that referenced this pull request Aug 17, 2018
@fredrikekelund
Explain how to make use of the apos.images.srcset helper
759f43d 
This PR was opened as a complement to apostrophecms/apostrophe#805.
But since the outcome of that PR was not to add a srcset feature
directly to apostrophe-images-widgets (but rather to introduce just
a template helper `apos.images.srcset`), this change adds a section
to the "Responsive images" how-to about how to write a simple widget
that renders an `<img>` tag with a srcset attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartromanek stuartromanek stuartromanek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fredrikekelund @boutell @stuartromanek