Add editorial request for browser sidebar.

Add editorial request for browser sidebar.

- Add a new cfg for the charmworld host. Currently the staging server.
- Update the Charm model to be friendly to reusing and work on a model that
extends it to start work on mapping Charmworld API0 data model.
- Add a new store that is the Charmworld0 Api with the first function to call
the sidebar_editorial data.
- Make sure the App passes in the db to the subapp which passes it to the
view.
- Update the sidebar view to display the slider widget for chosen charms.
- Update the sidebar to use charm-small to display 'new' charms from editorial
content.

https://code.launchpad.net/~rharding/juju-gui/sidebar_editorial/+merge/154158

(do not edit description out of merge proposal)

[rharding, 2013-03-21 20:38:02]

Please take a look.

[rharding, 2013-03-21 20:43:39]

Please take a look.

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

A few comments, but LGTM.

https://codereview.appspot.com/7947045/diff/3001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/app.js#newcode241
app/app.js:241: cfg = cfg || {};
This is quick, but haven't we been going with:

if (!cfg) {
  cfg == {}
}

Or is that something that doesn't work when cfg is null?

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode403
app/models/charm.js:403: }
A bunch of these ATTRs look the same as the Charm model being extended--do you
have to redefine them again?

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
File app/subapps/browser/views/sidebar.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:79: return node.getHTML();
It might be cleaner to just add a "createHTML" method on the CharmSmall that
returns the rendered template HTML.

That method could be used here directly, and used in the CharmSmall renderUI
when the HTML is needed for the setHTML call there.

[jeff.pihach, 2013-03-21 21:54:50]

Thanks for the work! I have a few small improvements, a few trivial comments
below some of which we have already chatted about.

Just a friendly reminder that we have decided to use camelCased names as a
convention and to slowly convert old code to camelCase as well. I didn't mark
every instance of underscores so please go through the files. :-)

Thanks again! Talk to you tomorrow.

https://codereview.appspot.com/7947045/diff/3001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/app.js#newcode896
app/app.js:896: charmworld_url: {},
camelCase

https://codereview.appspot.com/7947045/diff/3001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/config-debug.js#newcode15
app/config-debug.js:15: charmworld_url: 'http://staging.jujucharms.com',
camelCase to match the will be camelCased properties in the app

https://codereview.appspot.com/7947045/diff/3001/app/config-prod.js
File app/config-prod.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/config-prod.js#newcode15
app/config-prod.js:15: charmworld_url: 'http://staging.jujucharms.com',
same camelCase change

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode316
app/models/charm.js:316: writeOnce: true,
I am pretty sure there is a performance hit when using writeOnce so we should
only be using it if you think that there is a real risk of it being set by
accident.

As we talked in IRC this isn't the case so feel free to remove them :)

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode318
app/models/charm.js:318: return Y.Lang.isString(val) && !!charmIdRe.exec(val);
micro optimization warning: use typeof val == "string"

:)

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode377
app/models/charm.js:377: maintainer: {writeOnce: true},
Thanks for defining the ATTRS even though they would be automatically created :)
you++

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode399
app/models/charm.js:399: if (Y.Lang.isValue(val)) {
Even with this check; val could still be set as undefined, NaN, or a proper
integer so I would suggest either removing it entirely or beefing up the check.

https://codereview.appspot.com/7947045/diff/3001/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/store/charm.js#newcode173
app/store/charm.js:173: 'failure': function(io_request) {
quotes aren't needed

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/template...
File app/subapps/browser/templates/sidebar.handlebars (right):

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/template...
app/subapps/browser/templates/sidebar.handlebars:8: <div class="slider"></div>
trivial: I wonder if `slider` and `new` are too general - in that they may end
up picking up styles from a more general class at some point.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
File app/subapps/browser/views/sidebar.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:111: this.get('store').sidebar_editorial({
maybe this method should be renamed to getSidebarEditorial just to give it a
more expressive method name.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:112: 'success': function(data) {
quotes not needed.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:132: 'failure': function(data, request) {
quotes not needed.

https://codereview.appspot.com/7947045/diff/3001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/7947045/diff/3001/test/test_browser_app.js#new...
test/test_browser_app.js:75: view.destroy();
Yay!

https://codereview.appspot.com/7947045/diff/3001/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/7947045/diff/3001/test/test_charm_store.js#new...
test/test_charm_store.js:247: api = new Y.juju.Charmworld0({
Please make sure all new instances are destroyed at the end of the test suite so
that they don't accidentally effect tests which follow.

(Yesterday I spent wayyyy to long trying to find the cause of a test failure
because of a left over instance in memory with event listeners from old code
which didn't cause any issues until new stuff was added)

[rharding, 2013-03-22 12:46:13]

On 2013/03/21 21:09:22, j.c.sackett wrote:
> A few comments, but LGTM.
> 
> https://codereview.appspot.com/7947045/diff/3001/app/app.js
> File app/app.js (right):
> 
> https://codereview.appspot.com/7947045/diff/3001/app/app.js#newcode241
> app/app.js:241: cfg = cfg || {};
> This is quick, but haven't we been going with:
> 
> if (!cfg) {
>   cfg == {}
> }
> 
> Or is that something that doesn't work when cfg is null?

I see a case where an if (cfg === undefined) is used but I had thought this
standard JS for setting a possibly empty value to an empty object.


> https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js
> File app/models/charm.js (right):
> 
>
https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode403
> app/models/charm.js:403: }
> A bunch of these ATTRs look the same as the Charm model being extended--do you
> have to redefine them again?

Long term the plan is to move to a single model object. I duplicated so that I
could verify going down one list that things match up. I'll start to try to
transfer code over as we take over the browser duties and think it'll be easier
to just remove the original Charm model. So I guess I'm arguing that yes,
there's dupe code, but it'll make it cleaner/easier to maintain. 


> 
>
https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
> File app/subapps/browser/views/sidebar.js (right):
> 
>
https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
> app/subapps/browser/views/sidebar.js:79: return node.getHTML();
> It might be cleaner to just add a "createHTML" method on the CharmSmall that
> returns the rendered template HTML.
> 
> That method could be used here directly, and used in the CharmSmall renderUI
> when the HTML is needed for the setHTML call there.

The issue with that is the widget code won't be run setting the css classes and
creating the content boxes as when you do a .render(). The getHTML() will do
that so that our CSS is properly scoped and the like.

[rharding, 2013-03-22 13:08:27]

Comments below, updated code coming along in a second.

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode399
app/models/charm.js:399: if (Y.Lang.isValue(val)) {
On 2013/03/21 21:54:50, jeff.pihach wrote:
> Even with this check; val could still be set as undefined, NaN, or a proper
> integer so I would suggest either removing it entirely or beefing up the
check.

I don't think we're trying to avoid setting a null-like value for the ATTR. We
just don't want to try to run those through parseInt. So I think this is a sane
check to determine if it's say to try a parseInt without having it error out.

https://codereview.appspot.com/7947045/diff/3001/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/store/charm.js#newcode173
app/store/charm.js:173: 'failure': function(io_request) {
On 2013/03/21 21:54:50, jeff.pihach wrote:
> quotes aren't needed

They are needed for me to get through the linter. Otherwise it complains that
the callback doesn't have a YUI doc block and I can't submit it.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/template...
File app/subapps/browser/templates/sidebar.handlebars (right):

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/template...
app/subapps/browser/templates/sidebar.handlebars:8: <div class="slider"></div>
On 2013/03/21 21:54:50, jeff.pihach wrote:
> trivial: I wonder if `slider` and `new` are too general - in that they may end
> up picking up styles from a more general class at some point.

I've thought about this, but all of the code and uses are centered through the
container. The .less files are nested and namespaced. I'd hate to have to start
prefixing everything under the sun if it's not an issue. View's events and such
are all delegated from their container as well for event based issues.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
File app/subapps/browser/views/sidebar.js (right):

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:111: this.get('store').sidebar_editorial({
On 2013/03/21 21:54:50, jeff.pihach wrote:
> maybe this method should be renamed to getSidebarEditorial just to give it a
> more expressive method name.

Heh, I find renderEditorialView more expressive. It's not returning HTML for the
caller to put into the dom, it's doing the actual rendering. It's a method on
the class browser.Sidebar so the sidebar is implied just due to the class we're
in.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:112: 'success': function(data) {
On 2013/03/21 21:54:50, jeff.pihach wrote:
> quotes not needed.

Again needed for linter.

https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
app/subapps/browser/views/sidebar.js:132: 'failure': function(data, request) {
On 2013/03/21 21:54:50, jeff.pihach wrote:
> quotes not needed.

Again, needed for linter.

https://codereview.appspot.com/7947045/diff/3001/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/7947045/diff/3001/test/test_charm_store.js#new...
test/test_charm_store.js:247: api = new Y.juju.Charmworld0({
On 2013/03/21 21:54:50, jeff.pihach wrote:
> Please make sure all new instances are destroyed at the end of the test suite
so
> that they don't accidentally effect tests which follow.
> 
> (Yesterday I spent wayyyy to long trying to find the cause of a test failure
> because of a left over instance in memory with event listeners from old code
> which didn't cause any issues until new stuff was added)

I will double check. This class has no event listener and is just a tool for
fetching api results. I wouldn't expect to destroy it any more than a String or
Array instance.

[rharding, 2013-03-22 13:12:47]

Please take a look.

[j.c.sackett, 2013-03-22 13:19:01]

On 2013/03/22 12:46:13, rharding wrote:
> On 2013/03/21 21:09:22, j.c.sackett wrote:
> > https://codereview.appspot.com/7947045/diff/3001/app/app.js#newcode241
> > app/app.js:241: cfg = cfg || {};
> > This is quick, but haven't we been going with:
> > 
> > if (!cfg) {
> >   cfg == {}
> > }
> > 
> > Or is that something that doesn't work when cfg is null?
> 
> I see a case where an if (cfg === undefined) is used but I had thought this
> standard JS for setting a possibly empty value to an empty object.

Dig, thanks. I'm not entirely up on idiomatic JS.
 
 
>
https://codereview.appspot.com/7947045/diff/3001/app/models/charm.js#newcode403
> > app/models/charm.js:403: }
> > A bunch of these ATTRs look the same as the Charm model being extended--do
you
> > have to redefine them again?
> 
> Long term the plan is to move to a single model object. I duplicated so that I
> could verify going down one list that things match up. I'll start to try to
> transfer code over as we take over the browser duties and think it'll be
easier
> to just remove the original Charm model. So I guess I'm arguing that yes,
> there's dupe code, but it'll make it cleaner/easier to maintain. 

I'm swayed by the argument; just wanted to know the reasons for duping.

>
https://codereview.appspot.com/7947045/diff/3001/app/subapps/browser/views/si...
> > app/subapps/browser/views/sidebar.js:79: return node.getHTML();
> > It might be cleaner to just add a "createHTML" method on the CharmSmall that
> > returns the rendered template HTML.
> > 
> > That method could be used here directly, and used in the CharmSmall renderUI
> > when the HTML is needed for the setHTML call there.
> 
> The issue with that is the widget code won't be run setting the css classes
and
> creating the content boxes as when you do a .render(). The getHTML() will do
> that so that our CSS is properly scoped and the like.

Ah, that's true. Ok then.

[j.c.sackett, 2013-03-22 13:24:59]

LGTM, with one super nitpicky comment.

https://codereview.appspot.com/7947045/diff/14001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/7947045/diff/14001/app/config-debug.js#newcode15
app/config-debug.js:15: charmworldUrl: 'http://staging.jujucharms.com',
This applies to a couple of places; as long as we're camelCasing thing, can we
caps the whole of "URL"? It's an acronym, not a word--"Url" just looks weird to
me.

[jeff.pihach, 2013-03-22 14:18:35]

LGTM Thanks for making those changes!

[rharding, 2013-03-22 14:31:41]

*** Submitted:

Add editorial request for browser sidebar.

- Add a new cfg for the charmworld host. Currently the staging server.
- Update the Charm model to be friendly to reusing and work on a model that
extends it to start work on mapping Charmworld API0 data model.
- Add a new store that is the Charmworld0 Api with the first function to call
the sidebar_editorial data.
- Make sure the App passes in the db to the subapp which passes it to the
view.
- Update the sidebar view to display the slider widget for chosen charms.
- Update the sidebar to use charm-small to display 'new' charms from editorial
content.

R=j.c.sackett, jeff.pihach
CC=
https://codereview.appspot.com/7947045