Implementing #67

[BPScott]

This allows the widthsMap to be dynamically created using an array of availableWidths and a function, rather than having to build an availableWidths object outside of Imager.

This allows you to do this:

<div class="delayed-image-load-16x9" data-src="http://ichef.bbci.co.uk/images/ic/{width}/p01mzt0r.jpg" data-alt="alt"></div>

new Imager('.delayed-image-load-16x9', {
    availableWidths: [160, 240, 320, 480, 560],
    widthInterpolater: function(w){ return w + 'x' + (i / (16/9)); }   
  });

To build a url that looks like: http://ichef.bbci.co.uk/images/ic/160x90/p01mzt0r.jpg

[thom4parisot]

I'm writing that from a mobile phone.

This is a good idea: is the existing feature of availableWidths as a function addresses what you need? Have a look at the readme to know more about it.

Just to be sure if you need dynamic widths array or a way to alter the width election.

[thom4parisot]

Oh ok I see. Have a look at Imager.transforms.width and replace it with your flavour.

The side effect is it shared among all Imager instances. Is it what you are looking for?

[BPScott]

This commit is kinda annoying. The actual change is a 4 line diff, while there's 11 lines of fixing whitespace so that equals signs line up and the rest comes from a test case + docs.

Using availableWidths as a function doesn't fit this need as as I would need to reimplement all the width clamping logic from determineAppropriateResolution which IMO really should be Imager's problem. As an aside, I mentioned in #66 that I don't think that making users reimplement this width clamping is ever useful and thus I currently don't see the value in allowing availableWidths to be defined as a function, but that's a different issue.

Overwriting Imager.transforms.width comes close but also doesn't solve the use case as each Imager instance should be able to build its own widthsMap in a unique way - for instance I should be able to an instance for 16:9 images and another instance for 4:3 images. I demo a fudged version of this by building objects that I pass to availableWidths in this CodePen. With this PR, the code would be a bunch of Imager instantiations like what is written in the top PR message.

Pointing me at Imager.transforms.width has made me realise something interesting... This pull request deals with code far earlier than when Imager.transforms.width is called - as part of defining this.widthsMap using Imager.createWidthsMap.

Before this PR, widthsMap was created with null values, looking like:

var x = new Imager({ availableWidths: [160, 240, 320, 480, 560]});
console.log(x.widthsMap);
// Logs  {"160":null, "240":null, "320":null, "480":null, "560":null}

After this PR, the widths map is populated with real values:

var x = new Imager({ availableWidths: [160, 240, 320, 480, 560]});
console.log(x.widthsMap);
// Logs  {"160":160,"240":240,"320":320,"480":480,"560":560}

The same as if availableWidths was defined as an object:

var x = new Imager({ availableWidths: { "160":160, "240":240, "320":320, "480":480, "560":560 } });
console.log(x.widthsMap);
// Logs  {"160":160,"240":240,"320":320,"480":480,"560":560}

This means that the widths map is already set out using non-null values as soon as imager is instantiated. Thus because there shall no longer be the possibility for null values, Imager.transforms.width could be simplified from:

function (width, map) { return map[width] || width; }

to:

function (width, map) { return map[width]; }

[Integralist]

@BPScott Heya, I like the fact that we could simplify the Imager.transforms.width method :thumbsup: :-)

I'll admit I'm a little lost after reading the comments about where we stand. @oncletom did you have any further feedback?

If not then as far as I can see, the state of play looks like this:

• availableWidths doesn't help solve the problem this PR was created to handle
• Imager.transforms.width also doesn't help solve the problem this PR was created to handle
• The changes made in this PR mean Imager.transforms.width can be simplified (@BPScott are you OK to commit that tweak as part of this PR please)

After @BPScott sorts out that last point, then all that remains is for any feedback from @oncletom on the code/feature itself and to make sure the tests are passing. Otherwise this is looking pretty good to me.

[thom4parisot]

I need to get more hands on that before providing a better feedback; but as far as I've read, this seems to be a good addition.

[BPScott]

@Integralist that state of play analysis looks good to me. I'll make the change to Imager.transforms.width when I get a chance and shall rebase onto this branch.

[BPScott]

@Integralist Just tried to make the change to Imager.transforms.width and it's a no-go as it breaks other scenarios when a given width can potentially not appear in the width map.

The breaking scenario is when you use a data-width that wasn't in your array of availableWidths. For instance the following, where 300 is not defined as a value in availableWidths, would end up interpolating {width} with undefined.

<div class="delayed-image-load" data-src="http://placehold.it/{width}" data-width="300" data-alt="alternative text"></div>

Imager.transforms.width shall have to stay as it is. Thus this PR is ready to be merged in it's current state (pending signoff from @oncletom).

[thom4parisot]

@BPScott I'll give a more dedicated look tomorrow :-)

[thom4parisot]

OK now I've finished my stuff and meetings, I'm now looking at it :-)

[thom4parisot]

Okay that's a great addition, I'm all good for it :-)

If you need some help for the rebase let me know.

[BPScott]

Guess who buggered off on holiday for a week and didn't bother to tell anybody? This guy.

I've fixed setting this.widthInterpolator to use opts.widthInterpolator || returnDirectValue; instead of the ternary operator as requested and have rebased onto the top of master.

@oncletom we're good to merge.

[thom4parisot]

:+1: