Code review comment for lp:~hatch/juju-gui/bundle-detail-view
Revision history for this message
| Richard Harding (rharding) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
I think this is exactly on the right path. I'd tweak a few things. I
think the View can be a bit simpler by dealing with a single ATTR for a
model instance or a token id and that logic should be pushed to the
store. That way all those decisions are made there.
We can chat about details later but looks good and thanks for working on
getting this into play.
https:/
File app/store/
https:/
app/store/
ns is already defined? why do we need juju.charmworld
https:/
app/store/
a bundle from Charmworld.
don't need "Public method" we already can tell that.
https:/
app/store/
bindScope) {
if you compare this to the charm call, there's some work we could do
here around the caching. We should add support for caching details as we
do in the charms so that subsequent calls will be instantly available.
The cache is maintained by the store instance I think as I know it's
shared around both Browser and main app code.
https:/
app/store/
bundle's details.
https:/
app/store/
defaultSeries) {
should we have a promiseBundle? This came after the original charm call
and maybe working with a promise from the start would be nice here?
https:/
app/store/
did you verify this works for bundle ids?
https:/
app/store/
{
any calls to this are hints that we need to look for the later check for
using the default charm icon and add a check for default bundle icon.
https:/
app/store/
bindScope) {
we'll need a bundle related method as well. Per the call with Gary
today, related bundles are lists of other bundles in the same basket as
the current one.
https:/
File app/subapps/
https:/
app/subapps/
I'm assuming existing tests pass with this change?
https:/
File app/subapps/
https:/
app/subapps/
Y.Base.
I'd change this to BrowserTokenView
https:/
app/subapps/
function(bundle) {
this seems to be missing a bunch of shared logic with _renderCharmView.
The check for use in the inspector, are there any tabs? What about
sharing icon/setup?
https:/
app/subapps/
so again, maybe the tokenID is the new thing?
https:/
app/subapps/
rather than having diff charm/bundle attrs what about var token =
this.get('token'); if (token.isCharm()) else if (token.isBundle()) ?
Or even adding a thing to the model to have a type.
this.call('_render' + token.type + 'View') or something.
https:/
app/subapps/
=== -1) {
I feel like this check should be in the store vs here in the view. Maybe
the cleanest thing is to combine .charm/.bundle into a method where you
pass an id and the store does the logic. Checking for 'bundle' in the id
seems to be an implementation detail of the store and not something I'd
have Views worry about.
https:/
File app/templates/
https:/
app/templates/
}} icon" class="icon">
this needs to exist for bundles. They'll have icons as well.
https:/