bbc / Imager.js

Responsive images while we wait for srcset to finish cooking
Apache License 2.0
3.84k stars 224 forks source link

Responsive background images #108

Open thom4parisot opened 9 years ago

thom4parisot commented 9 years ago

Added a new option, cssBackground.

When cssBackground === true, Imager updates the element.style.backgroundImage. When cssBackground === false (or is not set), Imager work as usual (div -> img[blank gif] -> img[src])

closes #88

RickHolmes commented 9 years ago

I implemented your changes and discovered a bit of a weakness: sometimes you want to use background images, but sometimes you want s.

The site I'm dealing with uses thumbnail images which must all be the same size -- a perfect use case for background images. But I also needed to use full-size versions of these images and maintain their true aspect ratio. And it's much easier to just let the browser resize things than to do it with JavaScript.

So, my solution is to have another data attribute: data-background. This would be set as "yes" when you want to use a background-image, "no" or not used at all when you want to use an .

See http://kvdesign.net/testing for what I mean. Click on a project to see the modal with the thumbs.

thom4parisot commented 9 years ago

But I also needed to use full-size versions of these images and maintain their true aspect ratio.

Well in that case the CSS background-size: cover should do the job?

Regarding your point, this is true that this PR does not consider each image individually (cf. Imager constructor, this is decided once for all). This is mostly influenced by DOM replacement/update which would be unnecessary complex (to me, but I am not everybody so feel free to challenge this opinion).

Remember you can use several instances of Imager: one could be used to handle responsive images, another one could be used to handle background responsive images.

RickHolmes commented 9 years ago

I first tried using an Imager.update() method for the modals, passing new cssBackground: true option. But that gave a problem when there were still items to lazy load on the main page. (They could be used as background images, but that might cause accessibility or SEO problems.) So that was out.

Then I tried using a new instance of the Imager for each modal. Unfortunately, there are some images that are verticals. I would have to dynamically change the container size to fit the entire image in order to show them at their full size using background-size. Otherwise they would be cropped. And radically changing the container size would make the modal size jump around (even more) when using the links below the text to "navigate" between projects.

But there was also a problem with using a new Imager instance for each modal: for some reason I cannot seem to completely delete the previous modal's Imager, so I get some memory leakage. The old instances are still there, along with a detached DOM tree for each. I'm not sure whether the latter is coming from bound events or references. (I tried writing an Imager.destruct() method, but it didn't seem to work.)

So I settled on a single Imager instance, an update() method and using data-replacement-type="background" when I want to use a background image for the thumbs. This way the modal's full-size image is as big as possible and the thumbs are all the same size.

Rick

On 11/27/2014 6:06 AM, Thomas Parisot wrote:

But I also needed to use full-size versions of these images and
maintain their true aspect ratio.

Well in that case the CSS |background-size: cover| should do the job?

Regarding your point, this is true that this PR does not consider each image individually (cf. https://github.com/BBC-News/Imager.js/pull/108/files#diff-b33c92ac11fbf0d3ea67899762af99b7R109, decided once for all). This is mostly influenced by DOM replacement/update which would be unnecessary complex (to me, but I am not everybody so feel free to challenge this opinion).

Remember you can use several instances of Imager: one could be used to handle responsive images, another one could be used to handle background responsive images.

— Reply to this email directly or view it on GitHub https://github.com/BBC-News/Imager.js/pull/108#issuecomment-64776702.

nealoke commented 7 years ago

@oncletom why was this never released? :'( any chance it still will?

thom4parisot commented 7 years ago

@nealoke good question – I probably missed it at some point.

I am no longer a BBC employee so I do not have write permissions anymore. Happy to contribute again at some point if this library is still relevant in today's polyfilling world.

nealoke commented 7 years ago

@oncletom what other methods do you use now to solve the issues imager was created? I still see no way to effectively do this though :/ thanks!

kodikos commented 7 years ago

Hiya,

Not sure quite why I'm cc'd in this! But with it being in github and as long as its' public, you can fork, make changes and submit a merge request.

Jodi.

On 24 February 2017 at 15:36, Thomas Parisot notifications@github.com wrote:

@nealoke https://github.com/nealoke good question – I probably missed it at some point.

I am no longer a BBC employee so I do not have write permissions anymore. Happy to contribute again at some point if this library is still relevant in today's polyfilling world.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/BBC-News/Imager.js/pull/108#issuecomment-282321431, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOeYptz9lMUX9KffL16o8RhEvg6eFe_ks5rfvjsgaJpZM4CzIQF .