Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Strategies, URL variables and Unit Testing #15

Closed
wants to merge 55 commits into from

7 participants

@oncletom
Owner

tl;dr

Heavy documentation and logic splitting. We can plug several way to replace the <div> container.

New Stuff

  • configurable URL patterns to let people use whatever file organisation for their resized images (like http://myserver.co.uk/images/{width}-hidpi/picture.jpg)
  • configurable delay to postpone width URL calculation
  • multi instance to target different part of the screen with different settings (sidebar and main content for example)
  • you can plug it on any even system (DOM Events, pubsub libraries)
  • ability to update the instance's pool of watched elements
  • tests

Removed Stuff

Basically, they are all about opinionated software design:

  • internal event listeners (it's now the business of the people using it in their app — See the Demo page or the README to read about how to plug on events)
  • $ shim (we just iterate on array-like of elements)


^ responsive images huh?

@oncletom
Owner

Open for reviews @maslen @Integralist @addyosmani :-)

oncletom added some commits
@oncletom oncletom Adding Gruntfile to process sourcecode 8be08e3
@oncletom oncletom Contributing guide ebccb50
@oncletom oncletom Karma tests setup a66b72f
@oncletom oncletom Updated README eeb9758
@oncletom oncletom Splitted files to allow custom builds 6fd5749
@oncletom oncletom ES3 for old compat 0f69522
@oncletom oncletom Renamed `div` strategy as `container` to be less missleading on the c…
…ontainer tag semantic
d220ea9
@oncletom oncletom Better documentation of what we could expect from the future devs 325f919
@oncletom oncletom Completed some more documentation. Ready to gets the hands dirty. eeed56b
@oncletom oncletom Hooking typo 6f15ccd
@oncletom oncletom Fixing the Chai karma integration 29cfadf
@oncletom oncletom Loading test fixtures and wrote tests for constructor f1baf5a
@oncletom oncletom Imager constructor implementation 207ed80
@oncletom oncletom Testing and implementing `replaceUri` method 0ed3efb
@oncletom oncletom Container replacer tests and implementations 500d773
@oncletom oncletom Added `getBestWidth` and sorting them in descending order to even fas…
…ten loops.

Adding partial nextTick support.
Optimizing loops.
dd3462e
@oncletom oncletom Removed the strategy detection to be faster. People should eventually…
… create an Imager per type of replacement. That's the job of the `querySelectorAll` code.

Replacer -> Strategy to be less missleading.
91ee302
@oncletom oncletom Migration to a prototype constructor and separate placeholder options e1338f6
@oncletom oncletom Adding back the default width value as `data-width` 4bc6396
@oncletom oncletom Adding and testing `callback` parameter on the `process` method. f2acee4
@oncletom oncletom Added live NodeList test (and it works out of the box) 2d10d98
@oncletom oncletom Testing high frequency calls 27fc56d
@oncletom oncletom Adding a configurable delay 52a030e
@oncletom oncletom Codestyle da7af19
@oncletom oncletom EditorConfig for tests 17c32df
@oncletom oncletom Renaming folders too
Replacers -> Strategies
6cc47e2
@oncletom oncletom Legacy strategy, the one used in the example 67b921d
@oncletom oncletom Presenting both strategies on the demo page 2a9ef71
@oncletom oncletom Adjusted example 206d5b7
@oncletom oncletom Copying the proper attributes on each placeholder strategy 4ea775b
@oncletom oncletom Preparing better test coverage b97e462
@oncletom oncletom Extended tests and couple of tweaks.
* Provided context to `callback` on `process()`.
* Can't create a placeholder anymore with element without a `data-src` attribute
5717920
@oncletom oncletom Testing Legacy Strategy 9366ec5
@oncletom oncletom Documentation 20c50cf
@oncletom oncletom Linting core files e731d94
@oncletom oncletom Made the `npm test` task running and CI ready. e8185c1
@oncletom oncletom Hinting tests 3971fc1
@oncletom oncletom Added Travis CI test file
And removed a dupe `dependencies` and `devDependencies` in `package.json`.
a84981e
@oncletom oncletom Removed dupe package and unused karma Jasmine 0e47600
@oncletom
Owner

@Integralist I've rebased on the latest master; and even adjusted the new demo page :-) working like a charm!

@addyosmani

Whoa. Massive work! Will review the commits today and tomorrow. Thanks for your time on this @oncletom!

@addyosmani

adding @sindresorhus for any comments on the review too. Still looking through.

@addyosmani

You mention what this does in the unit test but there isn't an accompanying description for it here. Can you add a comment above this with one?

Owner

Yep my fault; I'll add the JSDoc unless I did that in a child commit.

@addyosmani

I would prefer for this to remain container strategy. Saying BBC strategy suggests it's used across the BBC, whilst this is just used by BBC news afaik.

Owner

Agreed, it was just unsure about the name and refered to the way it was done in house.

The actual BBC Strategy replaces the DIV by an IMG; the "Container" is rather suitable to the other one, appending an IMG inside the DIV. Would you rather agree on "Replacement" Strategy?

@addyosmani

This may be as simple as it needs to be, but doesn't perform any type/content checking against neither values nor uri. We may punt on this if it's internal only. Continuing to read through.

Owner

Yep, used internally only as for now (hence I have not added the @api tag in the JSDoc).

@addyosmani

Expect true to be true?

Owner

It was just to test the karma setup. It has hopefully been removed further on :-)

.travis.yml
@@ -0,0 +1,5 @@
+language: node_js
+node_js:
+ - 0.10

needs to be quoted

@oncletom Owner
oncletom added a note

Did not know that. Thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gruntfile.js
((2 lines not shown))
module.exports = function (grunt) {
+ "use strict";

single-quotes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gruntfile.js
((8 lines not shown))
+ options: {
+ configFile: 'karma.conf.js'
+ },
+ unit: {
+ background: true
+ },
+ ci: {
+ singleRun: true,
+ reporters: ['dots'],
+ browsers: ['PhantomJS']
+ }
+ },
+
+ jshint: {
+ options: {
+ jshintrc: ".jshintrc"

consistent quoting

which really applies to all the code

@oncletom Owner
oncletom added a note

Fine; I was too much used to Node.js conventions :-) I guess there even a JSHint setting for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
package.json
@@ -1,30 +1,50 @@
{
- "name": "Imager.js",
- "version": "0.0.1",
+ "name": "imager.js",
+ "version": "1.0.0",

i wouldn't go to 1.0.0 until this huge change is actually tested in the wild by multiple users.

@oncletom Owner
oncletom added a note

Got it. 0.1.0 or 1.0.0-alpha ?

0.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Demo - Grunt/index.html
((25 lines not shown))
- (\w-)
-
- 3. the file extension -> .jpg
- (\.(?:jpg|png|gif))$
-
- The only other part of the regex isn't captured and that is the number specified in the file name.
-
- One other thing to note is the use of the non-capturing group -> (?: )
- This means we can use parenthesis as a way to 'group' a part of the regex without needing to capture it
- */
- regex: /^(Assets\/Images\/Generated\/)(\w-)\d+(\.(?:jpg|png|gif))$/i
- });
+ var elements = document.querySelectorAll('.delayed-image-load'),
+ imager = Imager.init(elements, {
+ availableWidths: [320, 640, 1024],
+ strategy: 'legacy'

What's the reason for having multiple strategies?

@oncletom Owner
oncletom added a note

Well, good question.

First, my goal was to conciliate Imager.js and the BBC News in house ImageEnhancer. Same codebase, slightly different needs, less maintenance. Better for everybody.

Then, to popularise and help broadening the usage, I thought about the various tools like Drupal or CMSes in general where it may be complicated to change/alter the HTML code.

I favoured this approach to an if/elseif statements, because any user can extend its usage without hacking the initial code. Plus, it leads to a clean way to package the smallest amount of code in the browser (with an eventual grunt compile:all, grunt compile:strategy:container etc.).

I like ready to consume code, with an alternate and easy way to mash them in any software system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus

I like this change, especially the configurable URL patterns and pluggable event system :)

configurable delay to postpone width URL calculation

Why is this needed?

Atomic commits are good, but this many commits just counteracts the intention and just makes me look at the complete diff. Interactive rebase is your friend ;)

@oncletom
Owner

@sindresorhus well, I kept the delay thing after seeing in the ImageEnhancer code, and talking with @kaelig; where the most pragmatic thing is to favour loading the other components of the page then loading the additional content. Especially if you are changing the device orientation, it's better to make sure the phone had time to resize all the contents prior to eventually loading the new responsive URLs.

In this case, regarding the atomic commits, I did not know if I was going to be able to create multiple PR, for every specific feature… I progressed in a very transversal way, making things complicated to decouple within the Git history.
Sorry about that, I wish I could have done that a cleaner way.

Demo/Assets/Styles/base.css
((5 lines not shown))
+}
+
+.strategy-container .delayed-image-load{
@kaelig
kaelig added a note

Space before {

@oncletom Owner
oncletom added a note

Any reason except readability or convention?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
README.md
((15 lines not shown))
-## Contributing
+You have then to perform the replacement using the following JavaScript code:
+
+```javascript
+var images = document.querySelectorAll(".delayed-image-load");
@kaelig
kaelig added a note

You might want to give an example with getElementsByClassName which completely outperforms querySelectorAll. See http://jsperf.com/getelementsbyclassname-vs-queryselectorall/35

@oncletom Owner
oncletom added a note

I did it on purpose to make sure it was not a live NodeList. The performance test is more complicated than just running it as it you have a NodeList collection, the browser will have to update the JS variable in the case of a live NodeList; which is not the case of a static one. The tradeoff is paid at some point, at the beginning or during the lifecycle of a page.

In the case of a mobile approach, I think we might prefer reducing the extra computations that this small amount of time on page load. Not an easy answer :-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@oncletom
Owner

@sindresorhus @addyosmani @Integralist hi guys, I'm back! Adjusted things following your advices (codestyle consistency, doc, "BBC" strategy name).

Do you have other blockers, improvements or suggestions?

@Integralist
Owner

@oncletom heya, thanks for taking the time to work on this. I'm going to review it this weekend (apologies for taking so long to get around to it).

One thing I would like to see is some form of documentation/written example.

Also I'm wondering whether the current implementation should be kept in a sub folder as a simplified version for users? Admittedly I've not been through your code yet but the impression I'm getting is that the current implementation is smaller and offers users an easy route to get started and would be good to keep as an alternative if they didn't want the "all singing, all dancing" version you've worked on in this PR.

@addyosmani your thoughts on my above comments would be appreciated.

@oncletom
Owner

@integralist well, the Readme doc is not complete enough? I documented easy and custom routes to use it.

On an end user point of view, the only difference is to do yourself the dom selection, and to hook yourself on events. Which is documented and as complicated as listening to a click event.

If there is room to simplify things I'll do it.

@Integralist
Owner

Sorry @oncletom didn't notice the README had been updated. Was going to review properly on Sunday.

@oncletom
Owner

@Integralist cool, let me know in any case :-)

@oncletom

Saves bytes but creates a new object. Mobile savvy? (but actually the empty object could be cached or kept private if we adopt an IIFE style).

If you have an IIFE, we can store slice in a variable:

var slice = [].slice;
// ...
this.nodes = slice.call(collection || []);

hmm... also, save the slice effort for when really needed:

this.nodes = collection ? slice.call(collection) : [];
@oncletom
Owner

Updated a couple of things to generate min output and to tweak some bytes.

screen shot 2013-09-07 at 21 58 48

@Integralist
Owner

@oncletom @addyosmani I've had some time to go through this PR and to consider whether it's appropriate to the focus of the project, and my feeling is that (although this adds a couple of potentially useful additions) the new features introduce a complexity which could be accomplished in a simpler fashion (the sheer size of the PR and the number of changes are indicative of this).

To make sure it wasn't just myself seeing it this way I consulted with a couple of the team members here at BBC and also with the W3C Responsive Images Community Group to gauge feedback of this PR and it seems the added complexity was an issue but the ultimate opinion shared by all (both BBC team and community) was that this should stay as a forked version of the current Imager.js code.

What I would then suggest is that I update our README to point to Thomas' forked repo's (similar to how Underscore and Lodash co-exist) so developers can make the decision on which version they prefer to use.

In the background I can begin work on implementing some of the new additions (re: pixel density specification and handling dynamically added placeholders) in a way I feel is more appropriate for this project.

I'm going to close this PR but feel free to continue on the discussion below. But like I say I've gone with the majority decision (especially considering the W3C RICG are the exact audience this repo is targeting).

Obviously this goes without saying, but thank you for the amazing work you've done on this PR and hope you're OK with my suggestion to point to your repo's as an alternative option for users.

@oncletom
Owner

Too bad you made this decision without talking with me.

Waste of time.

@kaelig

Well at least I learned the basics of Angular while building this configuration tool http://kaelig.github.io/Imager.js-customizer/ :)

@Integralist
Owner

@oncletom If you you can simplify the work you've done then obviously I'll be happy to re-review.

I consulted with the community who are the target user audience for this library, they and other members of the team felt a fork was more appropriate (which matched my own opinion).

Again, I very much appreciate the time you've spent, but as I'm sure you're aware: doing a PR on an open-source project doesn't automatically guarantee it'll be excepted.

This shouldn't be considered a waste of time as I'll be making mention of your work in the readme file so developers know they have an option they can choose from.

@kaelig always a silver lining ;-)

@DavidBruant

I consulted with the community who are the target user audience for this library

It might be appropriate to document the project governance in the Readme. For instance "major changes have to be discussed in <link to mailing-list> first".
You also don't provide a link to this consultation. I can't find it on the public mailing-list, so I imagine it happened behind closed doors. I have no problem with this form of governance but it'd be nice if that was told upfront so people know about it.

I can understand @oncletom's frustration when after a 14 days PR with 55 commits, the PR is closed without a single warning, without discussion.

After someone does 55 commits within 2 weeks of work, following the contribution guidelines, asking for feedback, iterating several times, it might have not been entirely crazy to keep him in the loop when making the decision of what to do with his work.
WE ARE FUCKING HUMAN BEINGS FOR FUCK SAKE!

What I would then suggest is that I update our README to point to Thomas' forked repo's (similar to how Underscore and Lodash co-exist) so developers can make the decision on which version they prefer to use.

The comparison with Lodash is inaccurate since @oncletom hasn't expressed an intent to maintain a fork of Imager.js.

This shouldn't be considered a waste of time as I'll be making mention of your work in the readme file so developers know they have an option they can choose from.

Since the fork won't be maintained (I haven't talked with Thomas about it yet, but I imagine he doesn't plan to maintain it), evolution of the BBC-News versions won't be ported and Thomas' fork will very soon fall behind and won't be a viable choice for developers. So what you're saying is true, but only for every short period of time.
Probably not the sort of impact Thomas was looking for on this project.

Despite what your reassuring words, his time has been effectively wasted; it's hard to find more accurate words. Reviewers time too for that matter!

the sheer size of the PR and the number of changes are indicative of this

I'm not familiar enough with the project, but I can agree with this. Still, it might be worthwhile for you as project maintainer to discuss change by change with Thomas and ask to re-submit them in several independent smaller PRs (did I write something in full-caps about "fucking human beings" already?). It is absurd to reject 100% of a big PR like that. I can't believe none of the 55 well-cut commits, all are to be thrown away or belong to a different fork. To quote the contributing guidelines:

Make sure commits are as 'atomic' as possible (this way specific changes can be removed or cherry-picked more easily)

This seems like a way for your project to benefit from Thomas' work. Not sure what else to say. It's all in your guidelines...

doing a PR on an open-source project doesn't automatically guarantee it'll be excepted.

Glamorous typo.

@oncletom
Owner

Got angry. Had a breath. Had some cheese. Got better

@oncletom
Owner

So final words. Emotional reaction and frustration as a drawback of a passionate work ;-)

We will talk about this PR to design features independently from each other.

Thanks @brunobord for the beer. I had wine instead but it worked too ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 29, 2013
  1. @oncletom
  2. @oncletom

    Contributing guide

    oncletom authored
  3. @oncletom

    Karma tests setup

    oncletom authored
  4. @oncletom

    Updated README

    oncletom authored
  5. @oncletom
  6. @oncletom

    ES3 for old compat

    oncletom authored
  7. @oncletom
  8. @oncletom
  9. @oncletom
  10. @oncletom

    Hooking typo

    oncletom authored
  11. @oncletom
  12. @oncletom
  13. @oncletom
  14. @oncletom
  15. @oncletom
  16. @oncletom

    Added `getBestWidth` and sorting them in descending order to even fas…

    oncletom authored
    …ten loops.
    
    Adding partial nextTick support.
    Optimizing loops.
  17. @oncletom

    Removed the strategy detection to be faster. People should eventually…

    oncletom authored
    … create an Imager per type of replacement. That's the job of the `querySelectorAll` code.
    
    Replacer -> Strategy to be less missleading.
  18. @oncletom
  19. @oncletom
  20. @oncletom
  21. @oncletom
  22. @oncletom

    Testing high frequency calls

    oncletom authored
  23. @oncletom

    Adding a configurable delay

    oncletom authored
  24. @oncletom

    Codestyle

    oncletom authored
  25. @oncletom

    EditorConfig for tests

    oncletom authored
  26. @oncletom

    Renaming folders too

    oncletom authored
    Replacers -> Strategies
  27. @oncletom
  28. @oncletom
  29. @oncletom

    Adjusted example

    oncletom authored
  30. @oncletom
  31. @oncletom
  32. @oncletom

    Extended tests and couple of tweaks.

    oncletom authored
    * Provided context to `callback` on `process()`.
    * Can't create a placeholder anymore with element without a `data-src` attribute
  33. @oncletom

    Testing Legacy Strategy

    oncletom authored
  34. @oncletom

    Documentation

    oncletom authored
  35. @oncletom

    Linting core files

    oncletom authored
  36. @oncletom
  37. @oncletom

    Hinting tests

    oncletom authored
  38. @oncletom

    Added Travis CI test file

    oncletom authored
    And removed a dupe `dependencies` and `devDependencies` in `package.json`.
  39. @oncletom
  40. @oncletom
  41. @oncletom
Commits on Sep 5, 2013
  1. @oncletom
  2. @oncletom
  3. @oncletom
  4. @oncletom

    Default strategy is `replacer`.

    oncletom authored
    Adjusting the tests which were breaking because of that.
  5. @oncletom

    Typo

    oncletom authored
Commits on Sep 7, 2013
  1. @oncletom

    Completed missing example

    oncletom authored
    And a couple of few tweaks and codestyle consistency.
  2. @oncletom

    Added builders

    oncletom authored
  3. @oncletom
  4. @oncletom
  5. @oncletom

    Fixing typo in dest build

    oncletom authored
  6. @oncletom
  7. @oncletom
  8. @oncletom
  9. @oncletom

    Downsizing the default number of sizes to fit common devices values (…

    oncletom authored
    …thinking about making this value mandatory w/o defaultsas it's business specific)
Something went wrong with that request. Please try again.