Adds the charm slider widget

Adds the charm slider widget

This adds a slider widget derived from Jeff Pihach's flickr carousel. It
doesn't wire it into anything yet.

https://code.launchpad.net/~jcsackett/juju-gui/charm-slider/+merge/152418

(do not edit description out of merge proposal)

[j.c.sackett, 2013-03-08 14:51:37]

Please take a look.

[rharding, 2013-03-08 15:12:02]

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#new...
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#new...
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#new...
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#new...
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#new...
app/widgets/charm-slider.js:66: 
extra space here

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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....
File lib/views/browser/charm-slider.less (right):

https://codereview.appspot.com/7641043/diff/1/lib/views/browser/charm-slider....
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....
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#newco...
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#newco...
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.

[j.c.sackett, 2013-03-08 18:41:22]

Agree with most comments, address the remainder. Code is coming up shortly.

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#new...
app/widgets/charm-slider.js:21: *
On 2013/03/08 15:12:02, rharding wrote:
> 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.

Per discussion, this doesn't actual require any setup; just pass in parentNode
to render, as with default Y.Widget.

https://codereview.appspot.com/7641043/diff/1/app/widgets/charm-slider.js#new...
app/widgets/charm-slider.js:128: index = parseInt(index, 10);
On 2013/03/08 15:12:02, rharding wrote:
> why does index need to be parsed? How is it coming in as a string?

We get index out of a data-* attribute on the element, but I have moved the
parseInt to where we fetch it, so it's an int when it hits this method.

[j.c.sackett, 2013-03-08 18:47:53]

Please take a look.

[jeff.pihach, 2013-03-08 19:23:49]

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#...
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#...
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#...
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#...
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#...
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#...
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#...
app/widgets/charm-slider.js:198: }, 'li'));
delegate() also has a context property

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

https://codereview.appspot.com/7641043/diff/8001/lib/views/browser/charm-slid...
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.

[j.c.sackett, 2013-03-08 19:43:09]

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#...
> 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#...
> 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#...
> 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#...
> 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#...
> 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#...
> 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#...
> app/widgets/charm-slider.js:198: }, 'li'));
> delegate() also has a context property
> 
>
https://codereview.appspot.com/7641043/diff/8001/lib/views/browser/charm-slid...
> File lib/views/browser/charm-slider.less (right):
> 
>
https://codereview.appspot.com/7641043/diff/8001/lib/views/browser/charm-slid...
> 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.

[j.c.sackett, 2013-03-08 19:50:05]

Please take a look.

[jeff.pihach, 2013-03-08 20:17:48]

LGTM

[rharding, 2013-03-08 20:18:07]

On 2013/03/08 20:17:48, jeff.pihach wrote:
> LGTM

lgtm as well

[j.c.sackett, 2013-03-08 20:22:21]

*** Submitted:

Adds the charm slider widget

This adds a slider widget derived from Jeff Pihach's flickr carousel. It
doesn't wire it into anything yet.

R=rharding, jeff.pihach
CC=
https://codereview.appspot.com/7641043