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 CH hint to @oncletom Pixel ratio implementation #35

Closed laurentperez closed 10 years ago

laurentperez commented 10 years ago

Hi

I've added the ability to send a CH-DPR header to give the DPR following https://github.com/igrigorik/http-client-hints/blob/master/draft-grigorik-http-client-hints-01.txt.

I did not remove the DPR from the url because it can still be useful when the server only allows usage of pregenerated static files. Having the DPR in the url also allows you to extract data from access logs to see the % of clients using hidpi clients.

But one thing to keep in mind with the url approach is that some devices have very high DPRs. XPeriaS and Galaxy S4 reach a DPR of 3, not sure if this is a good idea to serve 3x flat files on a 3G connection because of the url. The CH hint is easier to "dishonor" on the server side for example when a 3G connection is detected.

I'm not familiar with Karma, I don't know how to pass the broken "Imager.js handling {width} in data-src should does not use RegExp anymore " test.

I'm basically sending the pull request to see if this is of any interest to you guys. I still think the CH hint is the best way to send the DPR information. @Integralist , I was the guy at your left at the Responsive Images meeting in Paris, sorry for not contributing earlier ;)

thom4parisot commented 10 years ago

@laurentperez wow cool to see a PR for my PR :-D What is this "hint" adds?

We also need to take in account the code should work on IE8/old browsers so we should not even have to run the whole request (if responseType does not exist in the instantiated XHR object, the pixel ratio has definitely a value of 1).

Also, I have the feeling you should PR that on my fork, on the oncletom:feature-pixel-ratio branch to make it easier to merge (as we need to cover your code by tests too).

Thanks :-)

laurentperez commented 10 years ago

updated the description, sorry I'm really not familiar with git ; I cloned your feature-pixel-ratio branch and worked from here, I tried to make a pull request on your branch but I guess I failed, yes : can you give me the git commands to PR on your branch ?

thom4parisot commented 10 years ago

Hop, here is the link. It's not very common to see that so don't worry if you are not familiar with this approach :-)

But as you can see, if you create a PR from the previous link (it's using your ch branch you'll have only one commit). If you continue pushing things in the branch it will add new things to the PR, even if I continue working on my branch :-)

laurentperez commented 10 years ago

Thanks. I'll push to the ch branch on my fork then.

I'll disable xhr when responsetype is unsupported, looks like it doesn't work on Android 2.x too.

Integralist commented 10 years ago

@laurentperez heya, good to speak again :-) @oncletom sorry I've been out of the loop for a bit due to work, can I get an update on where we are with this PR (this one still looks to be a PR against the master branch rather than @oncletom's branch) so wanted to make sure I was looking at the correct thing :-)

thom4parisot commented 10 years ago

We can close it; he proposed it again in oncletom/Imager.js#2 :-)

Integralist commented 10 years ago

@oncletom I thought he had! Cool, I'll close this then :-)

laurentperez commented 10 years ago

yes, sorry about that, wrong pull command