launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Thanks for the review.

> 
> === 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.
>

Fair point. I would never do this myself if I were writing the code from
scratch. This code pattern was lifted from formoverlay, and is also used
in subscribers_list and filebug_dupfinder. I'll file a bug to ensure
these other places are fixed.


> 
> === 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>
> 
> 

Ah, the &nbsp; is old markup originally added to the test harness by
copying old rendered lp html before the recent css changes and this html
in the test was copied to create anotherpicker anchor for this branch.
The tests don't seem to care either way. I'll certainly amend the markup
to confirm with the recent changes.


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

Yes, you are correct. Clearly my attempt at some trivial html for the
test failed. Will fix.

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