yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Thanks for the re-review Benji.


https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#newcode82
app/views/charm-panel.js:82: * @return {Array} A filtered, grouped set
of filtered entries.
On 2012/12/03 20:58:15, benji wrote:
> So, is the return value filtered?  ;P

YES, I wanted to make sure it was well known!

fixed

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#newcode93
app/views/charm-panel.js:93: var isSubFilter = function(charm) {
On 2012/12/03 20:58:15, benji wrote:
> Why not just "function isSubFilter"?  Oh, wait, I think there is a bug
in some
> browsers that makes that not work right.  Is that it?

no, it was just plain silliness.

fixed.  we'll see now if i break those browsers.

https://codereview.appspot.com/6842119/diff/12001/app/views/charm-panel.js#newcode980
app/views/charm-panel.js:980: // "charms" panel.
On 2012/12/03 20:58:15, benji wrote:
> I don't feel strongly one way or the other (or both, even), but I
thought I
> would point out that some of the code in this branch uses /* */ style
comments
> and some uses // comments.

I'll fix it so that YUIDOC are /** */ and others are //.

https://codereview.appspot.com/6842119/diff/12001/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6842119/diff/12001/test/test_charm_panel.js#newcode326
test/test_charm_panel.js:326: .setAttribute('id',
'charm-search-test').append(
On 2012/12/03 20:58:15, benji wrote:
> Since this code is chaining method calls, shouldn't the ".append" be
the first
> code on a new line?  Like so: http://paste.ubuntu.com/1408800/

Done.

https://codereview.appspot.com/6842119/

-- 
https://code.launchpad.net/~bac/juju-gui/cs-filter/+merge/137050
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bac/juju-gui/cs-filter into lp:juju-gui.