Code review comment for lp:~hatch/juju-gui/1130787-subapp-app-extension

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for the branch.

I have some minor comments below but there are still things I'm not
clear about here.

At minimum it seems that the parent should be the eventTarget of the
subApp when its created? Is that going to live in another branch? I
suppose this branch presents too small view of the final picture for me
to understand how this plays out in reality.

Also is there a use case for subapp removal? I think we can pass on that
for now as well, but we might note this somewhere.

Looking forward to seeing more.

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js
File app/assets/javascripts/app-subapp-extension.js (right):

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode9
app/assets/javascripts/app-subapp-extension.js:9: value: {}
As we don't currently support removal should this be writeOnce and/or
initOnly?

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode21
app/assets/javascripts/app-subapp-extension.js:21: config: {}
type -> instance, its more clear and follow the
App.views.viewName.instance model.

this.get('subApps'), this.subApplications and params named subApps all
occur below making things a little harder to follow than they need to
be.

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode56
app/assets/javascripts/app-subapp-extension.js:56:
this._augmentParentRoutes(routes);
parent seems unnecessary in the naming, _this_ is the parent.

_augmentRoutes or _mixinRoutes?

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode68
app/assets/javascripts/app-subapp-extension.js:68: @param {array}
subApps an array of sub abb objects and configs.
s/abb/app

https://codereview.appspot.com/7381055/diff/3001/app/assets/javascripts/app-subapp-extension.js#newcode71
app/assets/javascripts/app-subapp-extension.js:71: for (var i = 0; i <
subApps.length; i += 1) {
Y.each? We usually favor iterators to native loops.

https://codereview.appspot.com/7381055/

« Back to merge proposal