search

bbc / Imager.js

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

[READY] README demo for defining availableWidths as a function does not work #66

Closed BPScott closed 10 years ago

BPScott commented 10 years ago

The readme suggests that I can pass a function to availableWidths, however the example provided does not appear to work as expected.

Codepen Demo

As an aside:

What is the use-case for defining availableWidths as a function? I know it allows people to customize the function that runs when replaceImagesBasedOnScreenDimensions is called, but surely that's core functionality to Imager? Anybody that wants to overwrite it would also have to reimplement determineAppropriateResolution themselves otherwise they would end up hammering their image service on every resize. To me this feels like we're giving people the option to reimplement core internals at a too deep level without an obvious reason why they would need to.

thom4parisot commented 10 years ago

This started here:

I think it might actually be worth making a new image every 8 pixels, from what I can tell by the RespImages community chatter JPEG has some magic formula where images that are factors of 8x8 compress much better than other dimensions.

So instead of having a long array, the idea was to calculate dynamically the width of an image. That's basically what is tested here:

https://github.com/BBC-News/Imager.js/blob/935089d0a829a425cef06b34ef332d0933744b57/test/unit/core.js#L126-133

The idea is that Imager still works the same way, but the width picker logic can be overridden for special needs. It has to just return a value. This is the only responsability of that function, simple.

Integralist commented 10 years ago

@oncletom heya, just checking if this ready for me to review (looks like it)?

thom4parisot commented 10 years ago

@Integralist yep, ready :-) it was an "almost" working feature, previously badly implemented.

BPScott commented 10 years ago

It looks like this got merged into the browsers-compatibility branch, not master. Should this have gone to master as browser-compatability was already merged?

thom4parisot commented 10 years ago

@BPScott I'll rebase as soon as I end up the current meeting :-)

thom4parisot commented 10 years ago

@BPScott okay merged in master :-)