Code review comment for lp:~bcsaller/juju-gui/persistent-layout
Revision history for this message
| Benjamin Saller (bcsaller) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
https:/
File app/app.js (right):
https:/
app/app.js:351: active.update();
On 2013/01/20 15:58:45, gary.poster wrote:
> cool
Thanks, cutting to this should actually be a big improvement. I was
worried there would still be more work left before this would work
properly but it seems fine like this. This cuts the number of render
passes substantially and reduces the amount of work we do quite a lot.
https:/
File app/views/
https:/
app/views/
On 2013/01/20 15:58:45, gary.poster wrote:
> We now have this comment both in the event registration and in the
function
> definition. I'm not sure that inline event handler functions don't
have their
> place, to be honest. If we do separate them, I think we can only
expect to
> maintain comments in one place. I believe that the place should be
the
> function, so I would recommend deleting this copy of the comment.
Agreed with comment placement, this was the result of trying to quickly
cut a smaller branch, bad porting.
https:/
app/views/
function() {
On 2013/01/20 15:58:45, gary.poster wrote:
> I prefer the change that Nicola made, that you have reverted. In that
change,
> he renamed "ControlPanel" to "ServiceMenu," which seemed like a naming
> improvement to Nicola and to his two reviewers. He also was able to
be explicit
> that we only needed to "hide" here, which would be nice if we can
manage it.
Not intentional, this is how the trunk merge fell out. I'll restore his
version (which I had a hand in as well and agreed with).
https:/
app/views/
self.service_
On 2013/01/20 15:58:45, gary.poster wrote:
> You've reverted Nicola's changes. I think they were improvements, but
even if
> you disagree, socially a discussion would be better. Please don't do
this.
Bad Merge, fixed.
https:/
app/views/
On 2013/01/20 15:58:45, gary.poster wrote:
> You've reverted Nicola's changes. Please don't do this.
Same
https:/
File test/test_
https:/
test/test_
Y.clone(
This change may be worth of review comment. This is needed because we
delete annotations once their x/y has been read and applied. This
creates an issue in testing because its a reference all the way back to
the original test data stub.
https:/
File undocumented (left):
https:/
undocumented:1: app/app.js:599 "callback"
On 2013/01/20 15:58:45, gary.poster wrote:
> You increased the undocumented count from 241 to 246. It would be
nice if we at
> least kept the number stable across these refactorings. Given my
desire to be
> able to declare this complete by Monday, or Tuesday at the latest, I
won't stop
> you from landing for this, but maybe you could come back to write up
five
> methods later.
Added some doc strings, I think documenting callback handlers with a
full doc strings when they have uniform arguments and appeal to a
documented usage pattern is a bit of an anti-pattern (not that this is
the only type of doc I haven't written ;) . Happy to talk about this in
a Friday call if its an issue.
https:/