Implements the view-container module

Implements the view-container module

The view-container module is responsible for rendering viewlets
into the dom and then sending their configuration to the
bindengine.

The bindengine and view-container were developed separately
but in parallel so there may be some issues wrt integration
at this time.

https://code.launchpad.net/~hatch/juju-gui/y-view-container/+merge/167833

(do not edit description out of merge proposal)

[jeff.pihach, 2013-06-06 18:57:01]

Please take a look.

[rharding, 2013-06-06 19:20:46]

LGTM with some comments below.

https://codereview.appspot.com/10090045/diff/1/app/templates/view-container.h...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10090045/diff/1/app/templates/view-container.h...
app/templates/view-container.handlebars:1: <div class="view-container-wrapper">
Do we need a template/call through handlebars for this? Can it just be these
lines [xxx,yyy].join()?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:31: overwritten on instantiation and so only basic
defaults are defined here.
if they're going to be overwritten can they just be set to null ootb so that
it's clear nothing set them to anything not null?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:87: //this.container.setHTML();
again, this is just commented out code. I'd stick this as an example in the
docstring if you want, but maybe by default this can throw an exception or
console.log('not implemented') or something?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:169: 
Can we just make these docs on the initalizer? Break down the things in options
as part of that single block?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:290: Y.namespace('juju').ViewContainer =
ViewContainer;
Can we move this up to the top of the file to make it immediately clear how to
get at ViewContainer please?

https://codereview.appspot.com/10090045/diff/1/test/test_view_container.js
File test/test_view_container.js (right):

https://codereview.appspot.com/10090045/diff/1/test/test_view_container.js#ne...
test/test_view_container.js:55: function(y) {
why the Y=y? Can't you move the definitions above into before() and ignore the
whole remapping of Y?

https://codereview.appspot.com/10090045/diff/1/test/test_view_container.js#ne...
test/test_view_container.js:95: it('should generate viewlet instances based on
the config', function() {
generates vs should

[rharding, 2013-06-06 19:26:26]

LGTM with comments from previous attempt in email. Please read it.

[jeff.pihach, 2013-06-06 19:40:09]

Thanks for the review, comments below and changes incoming.

https://codereview.appspot.com/10090045/diff/1/app/templates/view-container.h...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10090045/diff/1/app/templates/view-container.h...
app/templates/view-container.handlebars:1: <div class="view-container-wrapper">
I anticipate that we will want some type of hbs compiling functionality moving
forward but at this time we don't have any designs from UX so this is just a
placeholder.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:31: overwritten on instantiation and so only basic
defaults are defined here.
I wanted to provide sane defaults so that for basic use the dev wouldn't have to
pass in anything more than a basic template

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:87: //this.container.setHTML();
good idea will move to the docstring

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:169: 
I was thinking of that, but it makes it really hard to read so I left it out.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:290: Y.namespace('juju').ViewContainer =
ViewContainer;
can do

https://codereview.appspot.com/10090045/diff/1/test/test_view_container.js
File test/test_view_container.js (right):

https://codereview.appspot.com/10090045/diff/1/test/test_view_container.js#ne...
test/test_view_container.js:55: function(y) {
I did this so that the YUI instance is available anywhere in the test suite

https://codereview.appspot.com/10090045/diff/1/test/test_view_container.js#ne...
test/test_view_container.js:95: it('should generate viewlet instances based on
the config', function() {
On 2013/06/06 19:20:48, rharding wrote:
> generates vs should

Done.

[jeff.pihach, 2013-06-06 19:43:57]

Please take a look.

[benjamin.saller, 2013-06-06 19:57:28]

LGTM w/trivials

Thanks for this, I expect we'll have so shake out some things on the merge, but
I expect this will work without major issue.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:31: overwritten on instantiation and so only basic
defaults are defined here.
On 2013/06/06 19:20:48, rharding wrote:
> if they're going to be overwritten can they just be set to null ootb so that
> it's clear nothing set them to anything not null?

Agreed, these defaults don't do much. 

Also is the template wrapper only there in case the render method somehow trys
to return multiple top level DOM nodes? Not sure why we need this as the viewlet
is wrapped in the view-container already.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:75: container: '',
This shouldn't default to string if the type is Y.Node. It should be null, no?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:98: render: function(model) {
ahh, you refactored generateDOM to render. We are out of sync there. I can
change my callout to generateDOM to render as well.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:207: return container;
This might be worth noting in the doc, it chainable on the container, not
'this'.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:219:
container.all('.viewlet-container').setStyle('display', 'none');
Wouldn't .show() and .hide() do what you want here including support the
optional transitions?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:251: expandViewletConfig: function() {
maybe a leading _ on the methodname? This is private and could be inlined into
generateViewlets if you'd rather.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:255: if (true) { // remove when we upgrade jshint
oh, give the linter what it wants :) 

Object.hasOwnProperty(viewlet)

no?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:257: if (true) { // remove when we upgrade jshint
again

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:267: // /*jshint +W089 */
Isn't this scoped to the closure automatically?

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:275: destructor: function() {}
Remove if unused

https://codereview.appspot.com/10090045/diff/10001/lib/templates.js
File lib/templates.js (right):

https://codereview.appspot.com/10090045/diff/10001/lib/templates.js#newcode208
lib/templates.js:208: Y.Array.each(config.server.template_dirs, function(dir) {
How long has that been broken, geesh.

[jeff.pihach, 2013-06-06 20:45:28]

Thanks for the review - comments below and fixes incoming

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:31: overwritten on instantiation and so only basic
defaults are defined here.
This is here to provide a reliable top container for any template that the dev
may pass in as a viewlet template.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:75: container: '',
On 2013/06/06 19:57:28, benjamin.saller wrote:
> This shouldn't default to string if the type is Y.Node. It should be null, no?

Done.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:98: render: function(model) {
yes sorry I forgot to mention that.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:207: return container;
After reviewing it, it makes more sense to chain on the view-container
instance.. Changed

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:219:
container.all('.viewlet-container').setStyle('display', 'none');
I was under the impression (incorrectly) that show/hide set visibility.. changed

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:251: expandViewletConfig: function() {
made private - it's kind of hard to follow so separating it out makes the
generateViewlets method easier to follow IMHO.

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:255: if (true) { // remove when we upgrade jshint
oh FINE! ;)

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:267: // /*jshint +W089 */
I am not sure - I just reverted by habit

https://codereview.appspot.com/10090045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:275: destructor: function() {}
On 2013/06/06 19:57:28, benjamin.saller wrote:
> Remove if unused

Done.

[jeff.pihach, 2013-06-06 20:46:48]

Please take a look.

[jeff.pihach, 2013-06-06 21:06:03]

*** Submitted:

Implements the view-container module

The view-container module is responsible for rendering viewlets
into the dom and then sending their configuration to the
bindengine.

The bindengine and view-container were developed separately
but in parallel so there may be some issues wrt integration
at this time.

R=rharding, benjamin.saller
CC=
https://codereview.appspot.com/10090045