Add the base subapp for Browser.

Add the base subapp for Browser.

- Add the subapps/browser code and wire it up to the main app.
- Update the templates to pull in templates in the subapp.
- Update the build to include the subapps directory so test-server/etc work.
- Add the FilterControl model that will be wired across the application.
- Incomplete notes:
  - the template is only a placeholder for more work.
  - the filter control isn't wired up to the app currently
  - this branch is just a stepping stone reaching up to diff limits to work
  forward from.

https://code.launchpad.net/~rharding/juju-gui/browser_subapp/+merge/151844

(do not edit description out of merge proposal)

[rharding, 2013-03-06 16:29:02]

Please take a look.

[jeff.pihach, 2013-03-06 16:54:35]

Thanks for the code, glad you figured out the template stuff. I have a few small
comments below.

https://codereview.appspot.com/7513043/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7513043/diff/1/app/index.html#newcode127
app/index.html:127: <div id="browser"></div>
This ID is pretty generic so we might end up with a id clash sometime in the
future. Could this be changed to be 'subapp-charm-browser' or something along
that line

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js
File app/models/browser.js (right):

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js#newcode1
app/models/browser.js:1: /*global YUI: false, window: false*/
should not be committed

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js#newcode59
app/models/browser.js:59: *
No comment as to what this class does

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js#newcode72
app/models/browser.js:72: _set_defaults: function() {
methods are to be camelcased according to our conventions.

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js#newcode98
app/models/browser.js:98: query_string = [
camelcase

YUI has a querystring module which would simplify this quite a bit.
http://yuilibrary.com/yui/docs/api/classes/QueryString.html#method_stringify

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js#newcode166
app/models/browser.js:166: // vim: set tabstop=2:shiftwidth=2
not to be committed

https://codereview.appspot.com/7513043/diff/1/app/modules-prod.js
File app/modules-prod.js (right):

https://codereview.appspot.com/7513043/diff/1/app/modules-prod.js#newcode46
app/modules-prod.js:46: // Sub Apps
This shouldn't be required with prod because `make prod` concatenates everything
under app. To be sure though - remove this and then run `make prod` to test.

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:1: /*global YUI: false*/
This shouldn't be committed into the repo

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:35: type: 'juju.browser.views.FullScreen',
These two view keys are calling the same view constructor

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:91: value: Y.one('#browser')
See the note in index.html

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/views/fulls...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:1: /*global YUI:false*/
not to be committed

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:37: * @method render
I think this will require an @return doc param

https://codereview.appspot.com/7513043/diff/1/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/7513043/diff/1/test/test_browser_app.js#newcode1
test/test_browser_app.js:1: /*global YUI: false, window: false, describe: false,
before:false,
not to be committed

https://codereview.appspot.com/7513043/diff/1/test/test_browser_models.js
File test/test_browser_models.js (right):

https://codereview.appspot.com/7513043/diff/1/test/test_browser_models.js#new...
test/test_browser_models.js:1: /*global YUI: false, window: false, describe:
false, before:false,
not to be committed

[rharding, 2013-03-06 18:30:33]

Comments below, code is coming up shortly.

https://codereview.appspot.com/7513043/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7513043/diff/1/app/index.html#newcode127
app/index.html:127: <div id="browser"></div>
On 2013/03/06 16:54:36, jeff.pihach wrote:
> This ID is pretty generic so we might end up with a id clash sometime in the
> future. Could this be changed to be 'subapp-charm-browser' or something along
> that line

Ok, tried to 'claim' our browser namespace, but changed to subapp-browser.

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js
File app/models/browser.js (right):

https://codereview.appspot.com/7513043/diff/1/app/models/browser.js#newcode98
app/models/browser.js:98: query_string = [
On 2013/03/06 16:54:36, jeff.pihach wrote:
> camelcase
> 
> YUI has a querystring module which would simplify this quite a bit.

Thanks, much nicer.
> http://yuilibrary.com/yui/docs/api/classes/QueryString.html#method_stringify

https://codereview.appspot.com/7513043/diff/1/app/modules-prod.js
File app/modules-prod.js (right):

https://codereview.appspot.com/7513043/diff/1/app/modules-prod.js#newcode46
app/modules-prod.js:46: // Sub Apps
On 2013/03/06 16:54:36, jeff.pihach wrote:
> This shouldn't be required with prod because `make prod` concatenates
everything
> under app. To be sure though - remove this and then run `make prod` to test.

Thanks for the heads up.

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:35: type: 'juju.browser.views.FullScreen',
On 2013/03/06 16:54:36, jeff.pihach wrote:
> These two view keys are calling the same view constructor

Yes, they'll both call the same view code which will read data to tell if what
should happen within that view.

We're working forward with a pair of main views, FullScreen and SidePanel and
will have them contain the logic about displaying based on the current
environment/query string info.

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/views/fulls...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/7513043/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:37: * @method render
On 2013/03/06 16:54:36, jeff.pihach wrote:
> I think this will require an @return doc param

Sorry, was part of the sample and no need to return at all so just removed the
return line.

[rharding, 2013-03-06 18:37:03]

Please take a look.

[rharding, 2013-03-06 18:50:54]

Please take a look.

[j.c.sackett, 2013-03-06 18:59:18]

Only one comment, otherwise lgtm.

https://codereview.appspot.com/7513043/diff/8001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/7513043/diff/8001/app/subapps/browser/browser....
app/subapps/browser/browser.js:90: value: Y.one('#browser')
Shouldn't this be valueFn, returning Y.one(...) ?

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

Thanks for those improvements! Couple trivial changes

https://codereview.appspot.com/7513043/diff/9001/app/models/browser.js
File app/models/browser.js (right):

https://codereview.appspot.com/7513043/diff/9001/app/models/browser.js#newcode90
app/models/browser.js:90: gen_querystring: function() {
camelCase ;^)

https://codereview.appspot.com/7513043/diff/9001/app/models/browser.js#newcode91
app/models/browser.js:91: var filter_data = {
camelCase ;^)

https://codereview.appspot.com/7513043/diff/9001/app/models/browser.js#newcod...
app/models/browser.js:140: // vim: set tabstop=2:shiftwidth=2
vim config

https://codereview.appspot.com/7513043/diff/9001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/7513043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:73: fullscreen_charm: function(req, res, next) {
camelCase ;^) and the yuidoc doesn't match the methodname

https://codereview.appspot.com/7513043/diff/9001/app/subapps/browser/browser....
app/subapps/browser/browser.js:90: value: '#browser'
This was changed in index.html but not here.

[rharding, 2013-03-06 20:15:49]

Please take a look.

[jeff.pihach, 2013-03-06 20:17:41]

LGTM Great start!

[rharding, 2013-03-06 20:27:10]

*** Submitted:

Add the base subapp for Browser.

- Add the subapps/browser code and wire it up to the main app.
- Update the templates to pull in templates in the subapp.
- Update the build to include the subapps directory so test-server/etc work.
- Add the FilterControl model that will be wired across the application.
- Incomplete notes:
  - the template is only a placeholder for more work.
  - the filter control isn't wired up to the app currently
  - this branch is just a stepping stone reaching up to diff limits to work
  forward from.

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