Code review comment for lp:~bac/launchpad/productseries-js

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

This looks good to land, but I ask that you refrain until YUI unittests run automatically.

> === renamed file 'lib/canonical/launchpad/javascript/registry/productseries-setbranch.js' => 'lib/canonical/launchpad/javascript/code/productseries-setbranch.js'
> --- lib/canonical/launchpad/javascript/registry/productseries-setbranch.js 2010-03-15 21:13:23 +0000
> +++ lib/canonical/launchpad/javascript/code/productseries-setbranch.js 2010-03-19 13:35:15 +0000
...
> + module.setup = function() {
> + Y.all('input[name=field.rcs_type]').each(function(field) {
> + field.on('click', module.onclick_rcs_type);});
> + Y.all('input[name=field.branch_type]').each(function(field) {
> + field.on('click', module.onclick_branch_type);});

I think these could use the ListNode.on() method to make the code a little
easier to read.

        Y.all('input[name=field.rcs_type]').on(
            'click', module.onclick_rcs_type);
        Y.all('input[name=field.branch_type]').on(
            'click', module.onclick_branch_type);
...

> === renamed file 'lib/canonical/launchpad/javascript/registry/tests/test_productseries_setbranch.js' => 'lib/canonical/launchpad/javascript/code/tests/test_productseries_setbranch.js'
> --- lib/canonical/launchpad/javascript/registry/tests/test_productseries_setbranch.js 2010-03-15 21:13:23 +0000
> +++ lib/canonical/launchpad/javascript/code/tests/test_productseries_setbranch.js 2010-03-19 14:29:27 +0000
...
> @@ -48,31 +50,50 @@
>
> tearDown: function() {
> delete this.tbody;
> - },
> + },
>
> test_handlers_connected: function() {
> - Y.Assert.areEqual('onclickBranchType()',
> - this.link_lp_bzr.getAttribute('onclick'),
> - 'branch type onclick handler not correct');
> - Y.Assert.areEqual('onclickBranchType()',
> - this.create_new.getAttribute('onclick'),
> - 'branch type onclick handler not correct');
> - Y.Assert.areEqual('onclickBranchType()',
> - this.import_external.getAttribute('onclick'),
> - 'branch type onclick handler not correct');
> -
> + // Manually invoke the setup function to ensure the handlers are set.+ module.setup();

Wrap the code at 78 characters.
...

@@ -202,7 +234,8 @@
     var console = new Y.Console({newestOnTop: false});
     console.render('#log');

- Y.on('domready', function() {
+ // Start the test runner on Y.after to ensure all setup has had a chance to complete.

Wrap the code at 78 characters.
...

review: Approve (js)

« Back to merge proposal