launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Approve code*

Thanks Ian, couple of comments:

- I'd suggest creating some methods to use for the Array.each bits that make for some code that'a pretty complex to figure out what it's doing. This is for lines 108 of the diff. The test is a bit unclear as well in line 187. 

It might even be a case where just nested if's are more clear with the line breaks than the single big if condition.

- The title of the control is set to 'There are no shared artifacts.' which will be shown to the user on hover in most browsers. Is artifacts the right term here for general users? Is there something more clear that they're used to seeing? 
-- 
https://code.launchpad.net/~wallyworld/launchpad/update-sharing-policies-some-988630/+merge/103626
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.