bbc / Imager.js

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

Add an option to build image URL with one single {width*pixel_ratio} placeholder instead of separated {width} and {pixel_ratio} #84

Closed nhoizey closed 9 years ago

nhoizey commented 10 years ago

Often, the same image could be served for a width of 200px and a pixel_ratio of 2 or a width of 400px and a pixel_ratio of 1.

Cases when this doesn't apply:

thom4parisot commented 10 years ago

Well if you really need to do that, that's more an exception than a common case.

You can use the new width interpolator to target the specific elements and multiply the width by 2, and enabling only specific availablePixelRatios.

That will have the same effect without adding any single line of code.

If that computation needs to be dynamic (per image and not per size), a slight change should do the trick (by making transform.width configurable))

nhoizey commented 10 years ago

I've seen this behavior in some projects, not as exceptional as it may seem… ;-)

Would it be possible to have the pixel_ratio as second parameter of the width interpolator? I don't want to multiply by 2 if the pixel_ratio is 1.3.

    new Imager({
        availableWidths: [200, 260, 320, 600],
        widthInterpolator: function(width, ratio) {
          return width * ratio;
        }
    });
thom4parisot commented 10 years ago

Well sadly you can't, as the interpolator is computed once, in the Constructor.

This is a way to say 200 is replaced by 200, or replaced by '_d' or '200x15' for example.

Making the transforms configurable is the way to go IMO, it does not make the project more complicated, it addresses your problem and let you a total control over it.

Good proposal in any case. Something that feeds the "responsive image" problem… which might not be solved by pure HTML spec ;-)

nhoizey commented 10 years ago

Pixel ratio doesn't change after the Constructor is run, so why couldn't it be used in the interpolator?

I'll give a look at the other option anyway.

thom4parisot commented 10 years ago

@nhoizey pixelRatio is available as a second argument :-)

Let me know if does the trick or not!

nhoizey commented 10 years ago

On iPhone 5, pixelRatio seems to be undefined…

I tested with my iPhone plugged to my Mac, with Safari remote debug.

thom4parisot commented 10 years ago

@nhoizey is window.devicePixelRatio returning a value at least? I wonder if the WebView is removing that information…

nhoizey commented 10 years ago

@oncletom it returns "2"

I'm using Safari iOS, not a webview.

You can test it on http://esviji.com/

The code I use:

new Imager({
    availableWidths: [160, 320, 480, 640],
    widthInterpolator: function(width, pixelRatio) {
        var r = width;
        if (undefined !== pixelRatio) {
            r *= pixelRatio;
        }
        console.log('window.devicePixelRatio = ' + window.devicePixelRatio);
        console.log(width, pixelRatio, r);
        return r;
    }
});

The result in the console: capture decran 2014-04-15 a 10 40 11

thom4parisot commented 10 years ago

Oh okay, the pixelRatio there :-) It might not be passed correctly from a previous call.

nhoizey commented 10 years ago

;-)

thom4parisot commented 10 years ago

@nhoizey oh yes it is definitely missing here for some reason.

I'll need to add it again and add a unit test to prevent that regression.

nhoizey commented 10 years ago

@oncletom that was quick! ;-)

thom4parisot commented 10 years ago

@nhoizey fixed in the latest commit pushed to the branch. Actually, I thought this feature was already merged!

Needs some proper tests before being merged though.

nhoizey commented 10 years ago

Aaaaand, it seems to work!

But the width="0" issue doesn't seem to be fixed in this branch… ;-)

thom4parisot commented 10 years ago

It should be working now, I have rebased from master :+1:

nhoizey commented 10 years ago

@oncletom I'll wait for he Travis CI build to pass… :-p

nhoizey commented 10 years ago

And it works perfectly!