Thank you for the review, Benji. I have made the changes that you requested except for one or two that I discuss below. Feel free to reply if you are still concerned about something that I push back on, or explain/rationalize unsatisfactorily. Gary https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description-related.handlebars File app/templates/charm-description-related.handlebars (right): https://codereview.appspot.com/6814089/diff/1/app/templates/charm-description-related.handlebars#newcode9 app/templates/charm-description-related.handlebars:9: {{#if owner}}{{owner}}/{{/if}}{{package_name}} On 2012/11/06 14:49:55, benji wrote: > Is the "if" redundant? I /think/ handlebars will omit the value if it is null > or undefined. Notice the "/" after {{owner}}. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js File app/views/charm-panel.js (right): https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode65 app/views/charm-panel.js:65: /** On 2012/11/06 14:49:55, benji wrote: > Our other multi-line yuidoc comments have a row of asterisks down the left. Do > we want to settle on one style or the other? > My vote would be for asterisks as it helps the comments stand out a bit. OK, will do. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode71 app/views/charm-panel.js:71: @static On 2012/11/06 14:49:55, benji wrote: > I'm curious. What does "static" mean here? Is it that this method doesn't > access "this"? Remember I mentioned on a call that yuidoc has no concept of standalone functions that are not class initializers? I found three workarounds. First, you can simply add comments without tags. I believe I found that yuidoc or one of our linters was not really happy with this. It also seemed annoying to me that you could not specify params and return values. Second, Dav Glass said in an email message I found that he would use tags to specify that this was a class. I found this hack deeply unsatisfying. It also produces bad docs IMO, when I tried it. Third, you can say that this is a method. Of what? Good question. Maybe the window. But it is a static method, yes: it does not access "this". I have a card about this for the weekly retrospective. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode80 app/views/charm-panel.js:80: return { On 2012/11/06 14:49:55, benji wrote: > This isn't the object literal style we use elsewhere, but I suspect the normal > style won't work here because of JavaScript's rules for implicitly placing > semicolons. Correct. I could wrap it all in parentheses, but that seemed much of a muchness to me. What I wrote passes the linter. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode87 app/views/charm-panel.js:87: /** On 2012/11/06 14:49:55, benji wrote: > I find the Roman dislike for helpful vertical whitespace bothersome. Could you make a topic about vertical whitespace for a retrospective discussion? I wouldn't mind vertical spaces immediately above the yuidoc comments, separating them from the functions above them. OTOH, I find vertical whitespace inserted into function bodies to generally be distractingly arbitrary if you are not the person who made the whitespace. Unfortunately, vertical whitespace are still a stylistic change about which we can all disagree, despite our beautifiers and linters. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode342 app/views/charm-panel.js:342: @method On 2012/11/06 14:49:55, benji wrote: > This directive needs the method name. Thanks, good catch. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode374 app/views/charm-panel.js:374: If there was a failure, render it to the console and to the On 2012/11/06 14:49:55, benji wrote: > This comment and the one just above have different indentation styles. Yeah, it's in part because the first comment is from outside the hash, and the second one is within the hash; and possibly also in part because the beautifier or I are insane, or because we are insane when combined together. In any case, I've tried to unify by putting the first comment withing the hash. https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode401 app/views/charm-panel.js:401: @method On 2012/11/06 14:49:55, benji wrote: > This needs the method name. Thanks, fixed https://codereview.appspot.com/6814089/diff/1/app/views/charm-panel.js#newcode431 app/views/charm-panel.js:431: Fires an event indicating that the charm panel should switch to the On 2012/11/06 14:49:55, benji wrote: > I can't explain > why exactly, but removing the "s" from "Fires an event" makes this much better. > More "active" I guess. From a consistency perspective, I do vary between an imperative style (e.g., "Fire") and a descriptive style (e.g., "Fires"). I tried to settle on imperative here and elsewhere (e.g. "returns" -> "return"). https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js File test/test_charm_panel.js (right): https://codereview.appspot.com/6814089/diff/1/test/test_charm_panel.js#newcode278 test/test_charm_panel.js:278: assert(section_container.one('h4 i').hasClass('icon-chevron-up')); On 2012/11/06 14:49:55, benji wrote: > Not your fault: The above is rife with brittle selectors. It would be great to > fix this up. Probably my fault one way or the other. :-P That said, I'm not clear on what to do that would truly be better. I could put classes on everything, but that's fragile in a different dimension IMO. Since you don't insist, I'm leaving it. https://codereview.appspot.com/6814089/