Code review comment for lp:~benji/juju-gui/no-foot-shooting

Sounds good benji. Thanks!

Gary

https://codereview.appspot.com/7129049/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode170
app/views/service.js:170: // it would break the application they are
currently using.
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > Suggest you add another comment similar to the following:
> >
> > // Note that the destroy button is not shown for the Juju GUI, so
this is a
> > fail-safe.

> I would be inclined to remove the code altogether. I accidentally
duplicated
> this between the menu's destroy handler and the destroy button at the
bottom of
> the service view. It is needed for the menu because the menu item is
only
> disabled in as far as its visual styling is different, the click event
is still
> fired.

> I have removed the code but if you like the fail-safe, I will be glad
to put it
> back (with commentary).

Cool, that's fine. I could go either way.

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode79
app/views/topology/service.js:79: // it would break the application they
are currently using.
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > I'm not sure yet from the code, but I'm hoping that this is again a
fail-safe
> > because the "destroy" option is not given to the user, in which case
a comment
> > to that effect would again be my preference.

> Nope, this is really the code that keeps the destroy from happening.
The menu
> item is only "disabled" visually. The click event is still fired.
This is the
> pre-existing pattern in the code (for the always-dead "Units" menu
item.

Ah, OK, cool.

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js#newcode963
app/views/utils.js:963: var charmUrl = candidate.charm ||
candidate.get('charm');
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > In the previous branch, Nicola and I both found this to be too
fragile. See
> the
> > version in
https://codereview.appspot.com/7069068/diff/1/app/views/utils.js .
> > You don't need to use that approach, and *maybe* it is not necessary
now for
> > some reason, but it was necessary in the previous branch for tests
to pass.

> All the tests pass with this version, so I am inclined to avoid the
"sloppier"
> version. The things passed in really should have a "charm" attribute
one way or
> the other.

> That being said, I don't feel strongly about it, so one more nudge
will get me
> to use the other branch's version.

Naah, let's stick with what you have, thanks.

https://codereview.appspot.com/7129049/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/7129049/diff/1/lib/views/stylesheet.less#newcode1507
lib/views/stylesheet.less:1507: .warning-message {
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > This is the kind of thing I wish we didn't do--but I did immediately
above,
> with
> > the "error" class! In some perfect world, our app would have enough
logical
> > consistency to its construction (and in our understanding of it)
that we could
> > do one of these sorts of things once.
> >
> > All that said, unless you are sure that this is some new generic CSS
> > functionality that you are providing, or the warning message is
really
> rendered
> > in the body of the page rather than within a tighter conceptual
region as
> > defined by a CSS class, or the warning message is used in two or
more very
> > different contexts, I suggest that you consider whether there is a
pertinent
> > section in the LESS to place this, so someone looking at the general
> > functionality in the LESS file can also see this part of it.

> I couldn't find any sections that really made semantic sense, so I put
> it in .view-container .control-group to at least limit its scope.
> Suggestions welcome.

I suggest you eat chocolate ice cream. It's really quite good.

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js
File test/test_templates.js (right):

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js#newcode81
test/test_templates.js:81: assert.notEqual(html.indexOf(warningMessage),
-1);
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > I get confused about this but I think we are using mocha plus chai.
If so,
> > maybe this works?
> >
> > assert.include(html, warningMessage, "html contains warning")

> "assert.include" works but there is no assert.notInclude so I decided
that being
> symmetric was more important than using a nice spelling in one case
and a
> non-nice spelling in another.

> > alternate spelling, which would work nicely with my next comment,
would be
> this.
> >
> > expect(html).to.contain(warningMessage)
> >
> > Would be nice if at least one of those worked. :-)

> Both expect(html).to.contain(...) and expect(html).to.not.contain(...)
work, but
> I hate that spelling with a passion. That hatred (and/or passion) may
not be
> justified, so for these at least I am willing to spell them in the BDD
way.
> Another option would be to have a small test helper. Perhaps one that
adds a
> "notInclude" to "assert".

> Thoughts?

As I said, I dislike the BDD spelling, but not as much as you do,
apparently. :-)

assert.exclude(...) might be nice. I don't want to hold up this branch
for it. If you like the idea, feel free to file a bug...or to act on it
now if you really, really feel like it. :-)

https://codereview.appspot.com/7129049/