Creates the widget to display the small charm box

Creates the widget to display the small charm box

A new widget is created, CharmSmall, representing the small box
representations of a charm throughout the app (e.g. the slider of several
charms).

This isn't wired up to anything, it's just the beginnings of the widget. It
also doesn't display the stars for the rating at this time, as we don't have
the sprite assets.

https://code.launchpad.net/~jcsackett/juju-gui/charm-small-widget/+merge/151285

(do not edit description out of merge proposal)

[j.c.sackett, 2013-03-01 16:32:16]

Please take a look.

[rharding, 2013-03-01 16:52:37]

let's chat on a couple of points and let me know if this makes sense.

https://codereview.appspot.com/7455044/diff/1/app/templates/charm-small-widge...
File app/templates/charm-small-widget.handlebars (right):

https://codereview.appspot.com/7455044/diff/1/app/templates/charm-small-widge...
app/templates/charm-small-widget.handlebars:1: <div class="charm-small">
this should get the class from the widget. Normally widgets, when created, auto
add the yui3-xxxx classes to things.

https://codereview.appspot.com/7455044/diff/1/app/templates/charm-small-widge...
app/templates/charm-small-widget.handlebars:2: <img url="{{ iconfile }}"
class="charm-icon"/>
I'd drop the class for the icon. It can easily be targetd by .charm-small img

https://codereview.appspot.com/7455044/diff/1/app/templates/charm-small-widge...
app/templates/charm-small-widget.handlebars:3: <span class="charm-title">{{
title }}</span>
don't prefix the classes with charm-. Targeting should be done through the
parent class so it can just be title. 

What I'd prefer is to see it be semantic.

title = <header>/h3 or something ;ole
description=<p>
rating=div class=metadata or rating

It eases css targeting as well.

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

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:10: YUI.add('charm-small', function(Y) {
prefix all of our code with browser-

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:12: var ns = Y.namespace('juju.widgets');
so juju.widgets.browser

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:22: * @return {undefined} Mutates only.
I think lint will let you just leave it out.

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:24: initialize: function(cfg) {},
initialize(r)

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:33: var content = this.TEMPLATE({
you can just pass the template this.getATTRS() if the names match up to the
template.

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:51: bindUI: function() {
we'll need a touch event here to be able to add. If you touch it, we should
show, and on another touch event not within our dom close. ...we'll have to look
at how touch works though as having each instance of this hiding seems overkill.

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:60: this.fire(ns.CHARM_ADD);
prefer to have events called out. ns.EVENT_CHARM_ADD

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:65: title: {value: ''},
docstring attributes.Llet me know if you use snipmate and i can get you ones for
doc_(class|method|attr)

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:68: iconfile: {value: ''},
s/iconfile/icon. It'll actually be a url to the icon I think? Have to check on
that since it's not currently implemented but I'd assume the icons would be
links to charmworld. Maybe they should be data: encoded attributes.

https://codereview.appspot.com/7455044/diff/1/test/test_charm_small_widget.js
File test/test_charm_small_widget.js (right):

https://codereview.appspot.com/7455044/diff/1/test/test_charm_small_widget.js...
test/test_charm_small_widget.js:24: var cfg = {container: charm_container};
the .render() method takes a parentNode. We should be able to pass that into
.render() and not require the config/container setup.

https://codereview.appspot.com/7455044/diff/1/test/test_charm_small_widget.js...
test/test_charm_small_widget.js:42: charm.render();
so this would be charm.render(charm_container);

[j.c.sackett, 2013-03-01 21:13:31]

I agree with these points, but for some minor points below.

I've made these changes, but currently I'm having difficulties getting them to
rietveld.

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

https://codereview.appspot.com/7455044/diff/1/app/widgets/charm-small.js#newc...
app/widgets/charm-small.js:22: * @return {undefined} Mutates only.
On 2013/03/01 16:52:37, rharding wrote:
> I think lint will let you just leave it out.

It might, but elsewhere this convention is being followed, even when
unnecessary.

[j.c.sackett, 2013-03-01 22:29:48]

Please take a look.

[rharding, 2013-03-01 22:38:22]

lgtm, thanks for the updates.

[jeff.pihach, 2013-03-01 23:19:11]

Thanks for all this code (and tests...yay!) I have a few small improvement
requests and some trivial comments below.

Thanks again, have a great weekend!

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js
File app/widgets/charm-small.js (right):

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js#n...
app/widgets/charm-small.js:34: this.get('contentBox').append(content);
You might want to consider using setHTML() here instead of append().

If there is some lagging content in the contentBox or if render() gets called
multiple times you will notice that it will continue to append the content
instead of replace it. This is a pretty trivial but I've found that it usually
works to my advantage.

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js#n...
app/widgets/charm-small.js:46: var add_button =
this.get('contentBox').one('button');
We have standardized on camel case (myRadVariable) for method and variable names
and pascal case (MyRadConstructor) for instantiatable objects

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js#n...
app/widgets/charm-small.js:50: this.on('mouseout', function() {
I don't know what the intended functionality is here but if you don't intend
these mouse* event callbacks to be called multiple times as the user moves over
the elements you probably want to rely on YUI's `mouseenter` and `mouseleave`,
or simply `hover` events which are in the `event-mouseenter` and `event-hover`
respectively.

More information can be found here:
http://yuilibrary.com/yui/docs/event/mouseenter.html

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js#n...
app/widgets/charm-small.js:53: add_button.on('click', function() {
It's generally not a good idea to rely on the browsers gc to clean up event
handlers when their hosts have gone (because often it doesn't work as expected).

Please keep reference to these event handlers and detach them in the destructor
to ensure that we aren't slowly eating away ram.

[j.c.sackett, 2013-03-04 00:32:15]

Jeff--
Thanks, I've made your suggested changes, but encountered a problem with
mouseenter &c, which I've explained in the comments.

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js
File app/widgets/charm-small.js (right):

https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js#n...
app/widgets/charm-small.js:50: this.on('mouseout', function() {
On 2013/03/01 23:19:11, jeff.pihach wrote:
> I don't know what the intended functionality is here but if you don't intend
> these mouse* event callbacks to be called multiple times as the user moves
over
> the elements you probably want to rely on YUI's `mouseenter` and `mouseleave`,
> or simply `hover` events which are in the `event-mouseenter` and `event-hover`
> respectively.
> 
> More information can be found here:
> http://yuilibrary.com/yui/docs/event/mouseenter.html

The intended function is a hover; I've made the change to use the 'hover' event,
but there doesn't seem to be a way to test it--event-simulate doesn't support
'hover', 'mouseenter', or 'mouseleave', and  I can't find an
event-mouseenter-simulate or similar in the docs. Thoughts?

[rharding, 2013-03-04 02:55:49]

On 2013/03/04 00:32:15, j.c.sackett wrote:
> Jeff--
> Thanks, I've made your suggested changes, but encountered a problem with
> mouseenter &c, which I've explained in the comments.
> 
> https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js
> File app/widgets/charm-small.js (right):
> 
>
https://codereview.appspot.com/7455044/diff/6001/app/widgets/charm-small.js#n...
> app/widgets/charm-small.js:50: this.on('mouseout', function() {
> On 2013/03/01 23:19:11, jeff.pihach wrote:
> > I don't know what the intended functionality is here but if you don't intend
> > these mouse* event callbacks to be called multiple times as the user moves
> over
> > the elements you probably want to rely on YUI's `mouseenter` and
`mouseleave`,
> > or simply `hover` events which are in the `event-mouseenter` and
`event-hover`
> > respectively.
> > 
> > More information can be found here:
> > http://yuilibrary.com/yui/docs/event/mouseenter.html
> 
> The intended function is a hover; I've made the change to use the 'hover'
event,
> but there doesn't seem to be a way to test it--event-simulate doesn't support
> 'hover', 'mouseenter', or 'mouseleave', and  I can't find an
> event-mouseenter-simulate or similar in the docs. Thoughts?

Sorry, not immediately relevent, but I had a thought. Can we just do this whole
displaying of the add button via a css:hover? I think our targets for this are
modern enough browsers and we don't need to carry JS events for it at all.

[j.c.sackett, 2013-03-04 21:06:06]

Please take a look.

[rharding, 2013-03-04 21:11:23]

lgtm thanks for updating.

https://codereview.appspot.com/7455044/diff/21001/app/widgets/charm-small.js
File app/widgets/charm-small.js (right):

https://codereview.appspot.com/7455044/diff/21001/app/widgets/charm-small.js#...
app/widgets/charm-small.js:27: var addButton =
this.get('contentBox').one('button');
comma this and don't add multiple vars per code convention in the gui.

https://codereview.appspot.com/7455044/diff/21001/config.js
File config.js (right):

https://codereview.appspot.com/7455044/diff/21001/config.js#newcode7
config.js:7: 'browser_dir': __dirname + '/lib/views/browser',
I don't think you need this addition.

[j.c.sackett, 2013-03-04 21:16:58]

I've address your comments, thanks!

https://codereview.appspot.com/7455044/diff/21001/app/widgets/charm-small.js
File app/widgets/charm-small.js (right):

https://codereview.appspot.com/7455044/diff/21001/app/widgets/charm-small.js#...
app/widgets/charm-small.js:27: var addButton =
this.get('contentBox').one('button');
On 2013/03/04 21:11:23, rharding wrote:
> comma this and don't add multiple vars per code convention in the gui.

Done.

https://codereview.appspot.com/7455044/diff/21001/config.js
File config.js (right):

https://codereview.appspot.com/7455044/diff/21001/config.js#newcode7
config.js:7: 'browser_dir': __dirname + '/lib/views/browser',
On 2013/03/04 21:11:23, rharding wrote:
> I don't think you need this addition.

You're right; and it's gone.

[jeff.pihach, 2013-03-04 21:27:51]

LGTM - Thanks for the work!

[jeff.pihach, 2013-03-04 21:27:51]

LGTM - Thanks for the work!

[j.c.sackett, 2013-03-04 21:32:52]

*** Submitted:

Creates the widget to display the small charm box

A new widget is created, CharmSmall, representing the small box
representations of a charm throughout the app (e.g. the slider of several
charms).

This isn't wired up to anything, it's just the beginnings of the widget. It
also doesn't display the stars for the rating at this time, as we don't have
the sprite assets.

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