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

Imager 0.1.0 #49

Closed thom4parisot closed 10 years ago

thom4parisot commented 10 years ago

At last!

thom4parisot commented 10 years ago

Hu hu the fact it is again a 55 commits PR is a nice coincidence 8-)

MoOx commented 10 years ago

yeahyeahyeah

Integralist commented 10 years ago

@oncletom heya, just ran npm install && npm run test and got the following output, wasn't sure if this was expected or not (in that, although there are 404 errors, ultimately it looks like each Phantom check is succeeding)?

❯ npm run test

> imager@0.1.0 test /Users/Code/Imager
> jshint Imager.js && karma start --single-run

INFO [karma]: Karma v0.10.8 server started at http://localhost:9876/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.2 (Mac OS X)]: Connected on socket tY9nEXRUDwGAklr2ONt3
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/A-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/B-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 2 of 17 SUCCESS (0 secs / 0.029 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/B-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 3 of 17 SUCCESS (0 secs / 0.05 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/B-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 4 of 17 SUCCESS (0 secs / 0.071 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/B-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 5 of 17 SUCCESS (0 secs / 0.092 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/B-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 9 of 17 SUCCESS (0 secs / 0.237 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/B-640.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 10 of 17 SUCCESS (0 secs / 0.259 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/A-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 15 of 17 SUCCESS (0 secs / 0.284 secs)
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/A-320.jpg
WARN [web-server]: 404: /base/Demo%20-%20Grunt/Assets/Images/Generated/C-320.jpg
PhantomJS 1.9.2 (Mac OS X): Executed 17 of 17 SUCCESS (0.715 secs / 0.485 secs)
Integralist commented 10 years ago

@oncletom I've made some tweaks (see last three commits). There are a couple of outstanding comments I've made (see above). If you could comment back please and then once that's done I'll give a final review and then merge :-)

Integralist commented 10 years ago

@oncletom fixed that typo, whoops! Good spot :-)

thom4parisot commented 10 years ago

No problem! Also the 404 in the tests are related to the fact they have not been generated by the grunt responsive images task. This is not making the tests failing, just raising a notice.

We could either run grunt responsive_images before the tests are launched or ship dedicated test images, independant from the demo.

Integralist commented 10 years ago

@oncletom I reckon we ship dedicated test images, as it'll mean running the tests will be faster not having to rely on the grunt responsive_images task to run. Is this something you want to do now or wait until this get's merged? It's not a deal breaker by any means.

Integralist commented 10 years ago

@oncletom regarding load.lowPriority I wonder whether it's worth changing that to just load (e.g. $(document).on('load', callback);) so it doesn't confuse people into thinking load.lowPriority is a real event.

thom4parisot commented 10 years ago

We'll do that later then. Maybe in the same PR as moving all the demos in a dedicated folder.

Regarding the load.lowPriority, agreed, let's keep things simple by avoiding adding possible confusion.

Integralist commented 10 years ago

@oncletom coolio. I've just updated the event btw

Integralist commented 10 years ago

@oncletom I think this is good to merge now. You happy for me to push the big green button? :-)

thom4parisot commented 10 years ago

@Integralist oh I appreciate but please do it, I like the symbol and the positive progress as an outcome from my first attempt :-) :+1: