Code review comment for lp:~jcsackett/juju-gui/charm-slider

Some light weight changes. Many have a lot of occurances so didn't mark
each one for the first pass, but {Object} vs { Object } and String vs
string.

Some other suggestions and additional tests requested.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js
File app/widgets/charm-slider.js (right):

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode7
app/widgets/charm-slider.js:7: * @namespace juju.widgets.browser
namespace juju; modules widgets; submodule browser.notifier I think is
how it works.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode12
app/widgets/charm-slider.js:12: var sub = Y.Lang.sub;
comma here and combine with next var

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode21
app/widgets/charm-slider.js:21: *
In the doc string can you put a sample of min. html needed to use the
widget? Want to include enough to reuse without too much hunting.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode29
app/widgets/charm-slider.js:29: * @type { string }
Caps String for a class name. No spaces inside the {}.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode66
app/widgets/charm-slider.js:66:
extra space here

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode82
app/widgets/charm-slider.js:82:
extra space here (my snippets fault I know...)

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode83
app/widgets/charm-slider.js:83: _generateDOM: function() {
please move private methods to the top of the class functions.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode90
app/widgets/charm-slider.js:90: sub(that.itemTemplate, {width:
that.get('width'), index: index}));
you get width twice. Please just get the var and cache it to avoid
lookups that aren't required.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode115
app/widgets/charm-slider.js:115: * Advances the slider to the next item,
or a designated index
end comment with .

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode128
app/widgets/charm-slider.js:128: index = parseInt(index, 10);
why does index need to be parsed? How is it coming in as a string?

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode184
app/widgets/charm-slider.js:184: this.set('pauseOnHover', true);
can we rename this to pause(d)OnHover. It matches up better since it's a
completed state. Maybe even just move to paused. Who cares if we're
paused for hover or some manual control.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode200
app/widgets/charm-slider.js:200:
Y.juju.widgets.browser.CharmSlider.superclass.bindUI.apply(this);
to prevent the nested lookup use ns. please.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode244
app/widgets/charm-slider.js:244: * @default 500
since this is a little slider let's default to something closer to what
we would use like 150 or 200px.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode273
app/widgets/charm-slider.js:273: * @attribute pauseOnHover
so this would go to just paused.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#newcode338
app/widgets/charm-slider.js:338: 'base-build',
select and :!sort in vim.

https://codereview.appspot.com/7641043/diff/1/lib/views/browser/charm-slider.less
File lib/views/browser/charm-slider.less (right):

https://codereview.appspot.com/7641043/diff/1/lib/views/browser/charm-slider.less#newcode1
lib/views/browser/charm-slider.less:1:
.yui3-browser-charm-slider-content {
let's add css to at least in-line the navigation controls by default.
Should just be able to display: block-line; width: auto;

https://codereview.appspot.com/7641043/diff/1/lib/views/browser/charm-slider.less#newcode5
lib/views/browser/charm-slider.less:5:
.yui3-browser-charm-slider-content ul {
more :!sort

https://codereview.appspot.com/7641043/diff/1/test/test_charm_slider.js
File test/test_charm_slider.js (right):

https://codereview.appspot.com/7641043/diff/1/test/test_charm_slider.js#newcode15
test/test_charm_slider.js:15: Y.one(document.body).prepend(container);
why document.body vs just 'body'?

https://codereview.appspot.com/7641043/diff/1/test/test_charm_slider.js#newcode55
test/test_charm_slider.js:55: });
There's some missing tests for the event handling. We should probably at
least test pause and test the click events advancing by checking the
index of the current view shown.

https://codereview.appspot.com/7641043/