Code review comment for lp:~wallyworld/launchpad/new-team-picker-load-form

Revision history for this message
Curtis Hovey (sinzui) wrote :

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>';
+ },

review: Approve (code)

« Back to merge proposal