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

On 2013/03/08 19:23:49, jeff.pihach wrote:
> Thanks for the code! See my comments (as we discussed) below.

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

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode72
> app/widgets/charm-slider.js:72: if (Y.Lang.isValue(index)) {
> We decided to no longer use isValue() as all it's really doing is a
glorified
> falsy check

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode102
> app/widgets/charm-slider.js:102: sub(that.itemTemplate, {width: width,
index:
> index}));
> There is no need to do the that=this trick here because (like most
methods of
> this type in yui) it has a final param which is the context.

> http://yuilibrary.com/yui/docs/api/classes/Array.html#method_map

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode103
> app/widgets/charm-slider.js:103: tmpNode.setContent(item);
> setContent() is depricated use setHTML()

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode116
> app/widgets/charm-slider.js:116: Y.log('_generateSliderControls',
'info',
> this.name);
> I think we have standardized on using console.log()

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode119
> app/widgets/charm-slider.js:119: Y.Array.each(this.get('items'),
function(item,
> index) {
> Like I mentioned above, YUI's iterators have a context param as the
final param
> so the that=this can be removed in favour of that.

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode195
> app/widgets/charm-slider.js:195: var index =
this.getAttribute('data-index');
> event callbacks are passed an event object which hold reference to the
target.

> so this can be changed to e.currentTarget.getAttribute('data-index');

https://codereview.appspot.com/7641043/diff/8001/app/widgets/charm-slider.js#newcode198
> app/widgets/charm-slider.js:198: }, 'li'));
> delegate() also has a context property

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

https://codereview.appspot.com/7641043/diff/8001/lib/views/browser/charm-slider.less#newcode1
> lib/views/browser/charm-slider.less:1:
.yui3-browser-charm-slider-content {
> Because we are using less you can actually nest most of the rules in
this commit
> - let me know if you need help doing this.

I've address your points, code coming shortly.

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