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] `data-src` + pixel ratio variable #27

Closed thom4parisot closed 10 years ago

thom4parisot commented 11 years ago

The intention

Imager takes care only about widths, not about pixel ratio.

The proposal

Exposing a {pixel_ratio} variable, as it is proposed with {width} (cf. #21).

From:

<div class="delayed-image-load" data-src="Assets/Images/Generated-{pixel_ratio}x/A-{width}.jpg" data-width="1024"></div>

To:

<div class="delayed-image-load" data-src="Assets/Images/Generated-1x/A-1024.jpg" data-width="1024"></div>

Caveats to solve:

Integralist commented 11 years ago

@oncletom this is an essential proposal! I like very much :smile:

In response to your queries...

do we want to handle that [URI structures with pixel ratio prefixes] ?

...I think in this case we'd get better performance from our code (and so ultimately for the end user/developer as well) if we enforced a URL structure of some kind so it included the 1x. Otherwise our code would need a sufficient amount of logic to work this out for the user, but if the user just ensured that 1x was included in the URL structure then the code base becomes much quicker to parse and execute. Seems the better choice to me, what do you think?

And regarding your second question...

some pixel ratios are float values (like 1.3 or 1.5), should we keep them as is or floor/round those numbers?

...I'd guess we'd need to keep those the same, as rounding them could cause unintended issues.

thom4parisot commented 11 years ago

Okay I see.

I wonder if the 1x is relevant against legacy websites. How is it handles in BBC News? Do you suffix only hidpi pictures?

Integralist commented 11 years ago

@oncletom currently we do not implement hidpi images at all, the CMS used by journalists doesn't implement any feature to handle that at the moment (something I believe they're working on).

I just personally prefer to keep a consistency (by utilising an interface - of sorts) so having the user include a 1x in their file names. This should also make the implementation of the Imager code a lot simpler.

BUT that does mean we're not being very flexible with users whose images are automatically generated and who may not actually have access to the server that serves their images (some SOA/RESTful organisations could have a set-up like this where developers can consume images but doesn't allow them to access the files to change their names).

So maybe it's best if no {pixelRatio} is provided then we can assume that the user requires a non-hidpi image. What do you think?

thom4parisot commented 10 years ago

Ready to review.

I've added the notion of transforms to makes things more flexible internally, and to have an open room to have this part pluggable from options.

Especially if we want to enable pixel ratio rounding for sanity reasons (otherwise you are going to generate an non-maintainable list of ratios which is not future/ops-proof). 

<div class="delayed-image-load" data-src="Assets/Images/Generated{pixel_ratio}/A-{width}.jpg" data-width="1024"></div>
<div class="delayed-image-load" data-src="Assets/Images/Generated{pixel_ratio}/A-{width}.jpg" data-width="1024"></div>

becomes

<!-- window.devicePixelRatio === undefined or window.devicePixelRatio === 1 -->
<div class="delayed-image-load" data-src="Assets/Images/Generated/A-1024.jpg" data-width="1024"></div>
<div class="delayed-image-load" data-src="Assets/Images/Generated/A-1024.jpg" data-width="1024"></div>

<!-- window.devicePixelRatio === 2.4 -->
<div class="delayed-image-load" data-src="Assets/Images/Generated-2.4x/A-1024.jpg" data-width="1024"></div>
<div class="delayed-image-load" data-src="Assets/Images/Generated-2.4x/A-1024.jpg" data-width="1024"></div>
Integralist commented 10 years ago

@oncletom this particular pull request looks good :-)

Note: seems my comments from the other PR's are being displayed here as well

thom4parisot commented 10 years ago

@Integralist I've renamed the function and used the regular dot notation :-)

Also, now we compute the pixel ratio value once.

Integralist commented 10 years ago

@oncletom made a quick comment on syntax consistency

Integralist commented 10 years ago

@oncletom looks good I think