imager.js updated.

[stephenplusplus]

Carried over from stephenplusplus/imager#1:

Hey guys,

I took a stab at elaborating on the prototype from https://gist.github.com/Integralist/6157139. I thought we could use a repository just for the imager library itself, and the other one would be the collaboration of the plug-in, the grunt task, etc. (I set this up as a bower component, but of course didn't and won't register anything)

Does this function as originally intended? I added in image caching (one of many parts that could use a review).

The script runs without dependencies currently. (the first few labels are things that can be modularized/outsourced/improved/scrapped depending on what browser support/dependencies is targeted) It does allow jQuery to take the wheel if it's present on the page. If I went off in a wrong direction, I have no problem scrapping this.

Hope this helps the overall mission!

[addyosmani]

Thanks once again, @stephenplusplus. @Integralist The implementation updates here LGTM, but would love a review from you if there's time.

[Integralist]

@addyosmani No problem. I had a quick scan yesterday and it seemed OK, but I'll review it properly later today and comment accordingly /@stephenplusplus

[addyosmani]

Thanks @Integralist!

@stephenplusplus once we've reviewed this, I think the next thing to properly tackle will be #1. If you're interested in helping with that, happy to collaborate on it further.

[Integralist]

@stephenplusplus @addyosmani just for future reference I've updated the README to include a section on those wanting to help contribute (I've kept it in line with other BBC open-source projects).

The point of interest is "ensuring commits are as 'atomic' as possible). So my concern is that there is one commit in this pull request and yet it is changing lots of the code.

For example, I would prefer to have a single atomic commits like so...

• commit for the pattern of wrapping the code in an iife and passing in the global window & document
• commit polyfill'ing requestAnimationFrame

...etc.

Apologies @stephenplusplus, I don't mean to disregard your good work but it would be much appreciated if you could ensure the commits are broken down as suggested above.

I'm in the process of reviewing your code changes now so if you can hold fire on the above and I'll let you know once I'm done (in case there are any other changes).

Thanks again Stephen for your time and help :-)

[stephenplusplus]

Absolutely, Mark! I'm more than happy to comply with that format.

[Integralist]

Unless I'm mistaken this line is defining a property requestAnimationFrameShimishStuff on an object that doesn't exist? Also the property isn't referenced again so there is no point in wrapping the polyfill code inside it.

[stephenplusplus]

It's just a label. More fun than a comment, but agree that it leads to unnecessary confusion. Will kick it out!

[Integralist]

@stephenplusplus @addyosmani I've finished reviewing the code. I've made a few comments regarding the code style and formatting. But as per my original comment about making sure the pull request abides by the need for atomic commits it might be easier if we close this pull request and re-open a fresh one so the atomic nature of the requests can be implemented.

@addyosmani I've noticed the code currently doesn't work (by that I mean things like the CSS aren't being generated and the images aren't being replaced), this is even before the changes implemented by this pull request. It maybe due to over engineering a general purpose solution (e.g. use of compass and a load of other tech solutions -> but I'll open that as a separate issue for you).

[stephenplusplus]

Thanks for the extensive review, Mark! I've gone through and applied changes per your feedback.

If you're not sick of this PR yet, feel free to look over and see if this is more what you had in mind. If I soured on method names again, I'm open to better suggestions.

I've also removed the pub sub implementation, as it seemed like unnecessary overhead the more I reviewed what we're doing. If you have a differing opinion, it won't trouble me at all to place it back in.

After your review of this, I will open more PRs to keep a sane git trail of these changes.

Thanks!

[stephenplusplus]

I'll change this to object next go around. Woops :)

[Integralist]

@stephenplusplus once you've completed all changes then feel free to drop a final comment in this discussion area mentioning @addyosmani and myself to re-review :-)

[stephenplusplus]

@Integralist & @addyosmani, it's ready to re-review now. 👍

[addyosmani]

• @Integralist the contributing guide and readme look fantastic. I'm happy to adhere to the atomic commits approach and appreciate @stephenplusplus doing the same for his changes.
• With respect to the current implementation not working (prior to this pull request), I would definitely appreciate feedback on how we can engineer a general solution without doing too much. We can probably just ship a Grunt setup with the minimal amount of work required to get the solution working and leave it up to devs to configure any additional tech they want (Compass and so on).

How would you feel about us merging in @stephenplusplus's work (when we're content with the changes) and then identifying the specific steps we want to automate via tooling?

From my end, I imagine this would be:

• Generate the necessary images with grunt-responsive-image
• Ensure Imager.js has the relevant configuration for the images generated
• Perform and production-level optimization required to ship the app

I had also been considering whether or not we should provide an option automate the step of using <div class="delayed-image-load" data-src="http://placehold.it/340" data-width="340"></div> (i.e the delayed-image-load, data-src, data-width etc) when using plain old images in your page. It may be overreaching for this project however and for a first release, we could just ask that developers use the correct attribute setup themselves.

[Integralist]

@addyosmani spot on with your comments :-) we'll work towards getting @stephenplusplus changes merged in (once we've re-reviewed) and then work towards minimalistic grunt set-up to get project at least releasable and then after promoting it we can work on greater configuration. Sounds like a good plan to me. Thanks :-)

[addyosmani]

Great! @stephenplusplus do you think you'll get a chance to redo this set of changes as more atomic commits for changes? I'll clean up the tooling in the repo as soon as your changes are updated. I can't currently see anything I strongly disagree with in the current PR otherwise.

[stephenplusplus]

I probably won't be able to get back on this until Monday. Summer weekends,
they're just too busy!

On Saturday, August 10, 2013, Addy Osmani wrote:

Great! @stephenplusplus https://github.com/stephenplusplus do you think
you'll get a chance to redo this set of changes as more atomic commits for
changes? I'll clean up the tooling in the repo as soon as your changes are
updated. I can't currently see anything I strongly disagree with in the
current PR otherwise.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2#issuecomment-22446056
.

[addyosmani]

SGTM. Have a good weekend!

[stephenplusplus]

Look at all those commits ^

[Integralist]

@addyosmani @stephenplusplus hey guys, I've merged in this change. As mentioned previously neither the original repo Addy pushed up nor these changes mean the example application is working so I'll take a quick look at trying to get a basic working version as per my original gist.