launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Approve code

Thank you for this branch. I have a few comments that need fixing before you land this branch.

=== modified file 'lib/lp/app/javascript/picker/person_picker.js'
--- lib/lp/app/javascript/picker/person_picker.js	2012-06-21 07:43:17 +0000
+++ lib/lp/app/javascript/picker/person_picker.js	2012-06-25 04:17:20 +0000
...

We prefer function expressions because they clearly show that the
function is a value of a variable. function statements are always hoisted
to the top of where scope is defined to ensure you can call them before they
are defined. Browsers and developers handle this badly.

+        function on_success(id, response, picker) {
        var on_success = function(id, response, picker) {

...
+        function on_failure(id, response, picker) {
        var on_failure = function(id, response, picker) {


=== modified file 'lib/lp/app/javascript/picker/tests/test_personpicker.html'
--- lib/lp/app/javascript/picker/tests/test_personpicker.html	2012-06-21 03:48:05 +0000
+++ lib/lp/app/javascript/picker/tests/test_personpicker.html	2012-06-25 04:17:20 +0000
...
I don't think the   belongs in this test, or in code to make this
visible to webkit...this link is missing action-icon. lazr-btn was intended
for <button>. I think a lot of code uses it as a marker, but we do not
have css rules to make it present weill with links.
+                  <a id="edit-anotherpickertest-btn"
+                    class="sprite edit lazr-btn yui3-activator-act"
+                    href="/fnord/+edit-people"
+                    >&nbsp;Edit</a>


=== modified file 'lib/lp/app/javascript/picker/tests/test_personpicker.js'
--- lib/lp/app/javascript/picker/tests/test_personpicker.js	2012-06-22 14:12:33 +0000
+++ lib/lp/app/javascript/picker/tests/test_personpicker.js	2012-06-25 04:17:20 +0000
...

@@ -325,6 +329,32 @@
This markup is invalid. The YUI test will not play in some browsers, notably
IE which knows that <tr> elements may only contain <td>. Some browsers will
will add the <td> for you in the DOM...but that means scripts may break
because they assume there is no <td>.
+        _simple_team_form: function() {
+            return '<table><tr>' +
+                '<input id="field.name">' +
+                '<input id="field.displayname"></tr></table>';
+        },

-- 
https://code.launchpad.net/~wallyworld/launchpad/new-team-picker-load-form/+merge/111543
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.