Improvements

[xat]

Hi Jono,

I refactored some stuff, maybe you wanna take a look :) I tested it on my computer within chrome and firefox. If you decide to merge please also test the code before.

xat

[jonom]

This looks great thanks xat, much cleaner. I am travelling for the next few days but will test as soon as I can. Only potential issue is that in order for image containers to adjust focus while the window is being resized (which is a nicer appearance) I think it needs to be throttling rather than debouncing. But debouncing will be a better choice for some projects so perhaps I should add a config setting to choose between throttling or debouncing.

Also I don't really have a good understanding of variable scope in javascript - are the variables localised to the plugin where you have them now?

[xat]

Okay, I changed it to throttle.

Yes, the variables are only visible inside the scope of the plugin. This is because they are declared inside a function scope with 'var' before:

;(function($) { /* variables are declared inside the function scope */ })(jQuery);.

If they would have been declared outside the function scope or without 'var' before then they would have been global.

[xat]

I've just added another change: Users can now stop focusPoint from listening to the resize event by calling:

$('.focuspoint').data('focusPoint').stop();

They can start it again by calling:

$('.focuspoint').data('focusPoint').start();

[jonom]

Awesome. Just need a bit of spare time to do some thorough testing and then I'll merge it in.

[xat]

Okay, nice :) I'll double check it with my Browserstack account.

[xat]

@jonom I've checked the Pull Request against these browsers:

IE 8 - 11 Safari 5.1 - 7 Firefox 31 and 32 Chrome 37 Android 4.4.4 iPhone with iOS 6 and 7 (simulator) iPad with iOS 6 and 7 (simulator)

Everything seemed to be fine on those browsers.

[jonom]

Thanks man. I've added some tests to master under demos/tests/index.html - with your refactored code it just seems to be failing on the Missing image-w and image-h test. Would you be able to have a look at why that might be? I tried to work it out but there's a few functions in use there I haven't seen before and I only have intermittent internet access atm so hard to look things up.

[xat]

okay, I've fixed it and changed the promise thing to a callback-solution.

[jonom]

Thanks for this nice contribution @xat

[xat]

thanks for merging :) just tell me if I can help with anything in the future.

[jonom]

I'm just wondering about consistency within the plugin - at the moment if you want to manually adjust focus you do something like $(some-container).adjustFocus() but if you want to stop listening to the window resize event you do $(some-container).data('focusPoint').stop(). I'm thinking the way I implemented adjustFocus was hacky (not localised to the plugin) and should be done the same way you've done the start/stop methods - do you agree / is this best practice? If so I would like to change the way adjustFocus is called for v1.1 to be in line with your new functions.

I'm also wondering if the start/stop methods should have more specific names like 'startWindowResizeListener'? In the future the plugin might recognise changes to the container size independent of a window resize so this might be more future-proof.

Lastly, I'm wondering about a shortcut to these methods but not sure if it's a good idea. I could make the plugin accept either an array or a string as a first argument. If it's an array it could treat is as options/settings and if it's a string it could call a function. Then you could have:

$(some-container).focusPoint('adjustFocus');
$(some-container).focusPoint('startWindowResizeListener');
$(some-container).focusPoint('stopWindowResizeListener');

etc. Any thoughts on that?

[xat]

Yes, I agree doing something like $(some-container).focusPoint('adjustFocus'); or $(some-container).data('focusPoint').adjustFocus()would be nicer then $(some-container).adjustFocus() because some users might not realise that adjustFocus is actually part of the focusPoint plugin.

Personally I like short API method names better because I think developers can remember them easier. But I agree that start() and stop() may be to over simplified. Maybe something in between?

Yeah, I like the idea of being able to call those methods by passing in a string instead of an options-object!