Merge lp://staging/~gary/juju-gui/doc-cleanup into lp://staging/juju-gui/experimental

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: not available
Merged at revision: 685
Proposed branch: lp://staging/~gary/juju-gui/doc-cleanup
Merge into: lp://staging/juju-gui/experimental
Diff against target: 667 lines (+263/-302)
4 files modified
CHANGES.yaml (+3/-3)
Makefile (+2/-2)
app/views/utils.js (+59/-92)
undocumented (+199/-205)
To merge this branch: bzr merge lp://staging/~gary/juju-gui/doc-cleanup
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+165497@code.staging.launchpad.net

Commit message

Add docs and clean up

Add docs to a few functions. Remove some functions that were not used. Fix generating docs, and a few typos here and there.

https://codereview.appspot.com/9704043/

R=hatch

Description of the change

Add docs and clean up

Add docs to a few functions. Remove some functions that were not used. Fix generating docs, and a few typos here and there.

https://codereview.appspot.com/9704043/

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

Reviewers: mp+165497_code.launchpad.net,

Message:
Please take a look.

Description:
Add docs and clean up

Add docs to a few functions. Remove some functions that were not used.
Fix generating docs, and a few typos here and there.

https://code.launchpad.net/~gary/juju-gui/doc-cleanup/+merge/165497

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9704043/

Affected files:
   M CHANGES.yaml
   M Makefile
   A [revision details]
   M app/views/utils.js
   M undocumented

Revision history for this message
Jeff Pihach (hatch) wrote :

LGTM Thanks for the cleanup!

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

https://codereview.appspot.com/9704043/diff/1/app/views/utils.js#newcode36
app/views/utils.js:36: @class utils
These comment blocks (all the way down) should probably be indented 2
spaces.

https://codereview.appspot.com/9704043/diff/1/app/views/utils.js#newcode181
app/views/utils.js:181: var removeSVGClass = function(selector,
class_name) {
Do the YUI versions of these functions not work?

hasSVGClass
addSVGClass
removeSVGClass

https://codereview.appspot.com/9704043/

Revision history for this message
Gary Poster (gary) wrote :

Thank you for the review!

I think your question is a great one about the SVG functions, but that's
for another day, another branch.

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

https://codereview.appspot.com/9704043/diff/1/app/views/utils.js#newcode36
app/views/utils.js:36: @class utils
On 2013/05/23 22:35:28, jeff.pihach wrote:
> These comment blocks (all the way down) should probably be indented 2
spaces.

I didn't think the indent was required, but done.

https://codereview.appspot.com/9704043/diff/1/app/views/utils.js#newcode181
app/views/utils.js:181: var removeSVGClass = function(selector,
class_name) {
On 2013/05/23 22:35:28, jeff.pihach wrote:
> Do the YUI versions of these functions not work?

> hasSVGClass
> addSVGClass
> removeSVGClass

I don't know. That's a question for a different clean up branch, I
think. (The main point of this branch is to test Aaron's new landing
command, actually. :-) )

https://codereview.appspot.com/9704043/

Revision history for this message
Gary Poster (gary) wrote :

*** Submitted:

Add docs and clean up

Add docs to a few functions. Remove some functions that were not used.
Fix generating docs, and a few typos here and there.

R=jeff.pihach
CC=
https://codereview.appspot.com/9704043

https://codereview.appspot.com/9704043/

Revision history for this message
Brad Crittenden (bac) wrote :

doh, too late, didn't see you'd submitted already. still, a lovely
branch.

https://codereview.appspot.com/9704043/diff/6001/app/views/utils.js
File app/views/utils.js (left):

https://codereview.appspot.com/9704043/diff/6001/app/views/utils.js#oldcode308
app/views/utils.js:308:
Gross, we had it in there twice just for good measure? Good eye.

https://codereview.appspot.com/9704043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches