Adding in lazy rendering of sub apps

Adding in lazy rendering of sub apps



https://code.launchpad.net/~hatch/juju-gui/1136431-lazy-render/+merge/151118

(do not edit description out of merge proposal)

[jeff.pihach, 2013-02-28 22:34:09]

Please take a look.

[gary.poster, 2013-02-28 22:43:02]

LGTM.  trivial suggestions.

Thanks

Gary

https://codereview.appspot.com/7444046/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7444046/diff/1/app/app.js#newcode468
app/app.js:468: subApp._checkRendered();
trivial: If this is called outside of the subApp, it is by definition not
protected, right?  I'd suggest making this not prefixed (just .checkRendered()).

https://codereview.appspot.com/7444046/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7444046/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:31: _checkRendered: function() {
not really protected, yeah?

[matthew.scott, 2013-02-28 23:08:57]

LGTM - one minor comment to take or leave if you wish, plus Gary's questions.

https://codereview.appspot.com/7444046/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7444046/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:31: _checkRendered: function() {
Curious about the name.  'checkRendered' makes me think it's going to return a
boolean of whether or not the app is rendered.  I don't feel strongly about it,
though, and if there's precedent, by all means, go ahead.

[jeff.pihach, 2013-02-28 23:27:27]

Thanks for the reviews! Comments below

https://codereview.appspot.com/7444046/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7444046/diff/1/app/app.js#newcode468
app/app.js:468: subApp._checkRendered();
On 2013/02/28 22:43:02, gary.poster wrote:
> trivial: If this is called outside of the subApp, it is by definition not
> protected, right?  I'd suggest making this not prefixed (just
.checkRendered()).
yep I agree.

https://codereview.appspot.com/7444046/diff/1/app/assets/javascripts/sub-app.js
File app/assets/javascripts/sub-app.js (right):

https://codereview.appspot.com/7444046/diff/1/app/assets/javascripts/sub-app....
app/assets/javascripts/sub-app.js:31: _checkRendered: function() {
On 2013/02/28 23:08:57, matthew.scott wrote:
> Curious about the name.  'checkRendered' makes me think it's going to return a
> boolean of whether or not the app is rendered.  I don't feel strongly about
it,
> though, and if there's precedent, by all means, go ahead.
Yeah you're right this isn't really protected I'll unprefix and change the doc.

Matt - I agree, I'll change it to verifyRendered

[jeff.pihach, 2013-02-28 23:34:15]

*** Submitted:

Adding in lazy rendering of sub apps

R=gary.poster, matthew.scott
CC=
https://codereview.appspot.com/7444046