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

Ability to add elements to Imager after initialisation #118

Closed willbamford closed 9 years ago

willbamford commented 9 years ago

This is very similar to (and inspired by) https://github.com/BBC-News/Imager.js/pull/92, with a slight change in the interface and the addition of a few tests.

closes #92 closes #110 refs #24

willbamford commented 9 years ago

Not sure why Travis failing - suspect it's not related to the changes. Anyone in News available to review this? We could do with this change on BBC Childrens.

thom4parisot commented 9 years ago

Hello @WebSeed, thanks for contributing :-)

Your implementation is sensibly what I expected it to be. Could you refactor the constructor so as it uses the .add() method instead please? (initialising Imager is basically adding images to an empty tracking collection).

I'd be happy to merge and release after that :-)

Travis fails because the Browserstack credentials are not propagated in remote forks (to avoid leaking the secured data) – nothing you can do about sadly (although you can run the BS tests locally if you use your own FM BS credentials — let me know if you need some help about that).

willbamford commented 9 years ago

Okay, thanks @oncletom. I've updated the constructor to use the add method - was hoping to make the interface to the constructor and add very similar to each other but the constructor allows for a lot of ways of defining which elements are tracked. Thought it safer to not change the existing interface (no existing unit tests changed).

Also, there were a few competing standards re. function declaration and whitespace, so I changed this too. Apologies if this has created a noisy diff - happy to remove this if necessary.

willbamford commented 9 years ago

@oncletom Sorry to be a pain - any update on accepting this PR?

thom4parisot commented 9 years ago

Sorry for the delay @WebSeed, a bit chasing time at the moment.

I have a made a couple of comments. Let me know what you think about them. So far so good otherwise :-)

willbamford commented 9 years ago

Hi @oncletom, no problem regarding the delay - thanks for the feedback. I think I've addressed the comments.

thom4parisot commented 9 years ago

Thanks @WebSeed I will review your changes tomorrow and publish it :-)

thom4parisot commented 9 years ago

Merged and shipped in v0.5.0. Thanks @WebSeed for your contribution and your patience :-)

willbamford commented 9 years ago

Cheers 😃