This is a very good start Brad. I have some concerns about using the underlying js engine. I have some proposals that use YUI to minimise engine problems. There are some style issue I think need fixing too. The test is very nice, and it will prove useful when refactoring some of this library. > === added file 'lib/canonical/launchpad/javascript/registry/productseries-setbranch.js' > --- lib/canonical/launchpad/javascript/registry/productseries-setbranch.js 1970-01-01 00:00:00 +0000 > +++ lib/canonical/launchpad/javascript/registry/productseries-setbranch.js 2010-03-16 20:32:39 +0000 > @@ -0,0 +1,93 @@ > +/* Copyright 2010 Canonical Ltd. This software is licensed under the > + * GNU Affero General Public License version 3 (see the file LICENSE). > + * > + * Control enabling/disabling of complex form on the > + * productseries/+setbranch page. > + * > + * @module Y.registry.productseries_setbranch > + * @requires node > + */ This module require DOM too. Is registry the correct location for this script? I think the code module may be better. Consider a project that will never have code/branches; this code will never be used. I think the code and bug configurations belong in their respective apps. My test for placing code in registry is to ask what happens if the app is removed? If we remove Launchpad Code, this script would have to be removed in a separate effort. > +YUI.add('registry.productseries_setbranch', function(Y) { > + Y.log('loading registry.productseries_setbranch'); > + var module = Y.namespace('registry.productseries_setbranch'); > + > + module._getSelectedRCS = function() { We use PEP 8 for function and method names. These names should be lower case with underscores. When you see someone setting a module method with studlyCaps, it is probably because that method is defined in a prototype or event. Review all your method names. > + var rcs_types = module._rcs_types(); > + var selected = 'None'; > + for (var i = 0; i < rcs_types.length; i++) { > + if (rcs_types[i].checked) { > + selected = rcs_types[i].value; > + break; > + } > + } > + return selected; > + }; > + > + module.__rcs_types = null; > + > + module._rcs_types = function() { > + if (module.__rcs_types === null) { > + module.__rcs_types = document.getElementsByName('field.rcs_type'); > + } > + return module.__rcs_types; > + }; > + > + module.setEnabled = function(field_id, is_enabled) { > + var field = Y.DOM.byId(field_id); > + field.disabled = !is_enabled; > + }; > + > + module.onclickRcsType = function() { > + /* Which rcs type radio button has been selected? */ > + // CVS > + var rcs_types = module._rcs_types(); > + var selectedRCS = module._getSelectedRCS(); > + module.setEnabled('field.cvs_module', selectedRCS == 'CVS'); > + }; Event handler functions receive an event argument, traditionally named 'e' module.onclick_rcs_type = function(e) { You need to update all of these methods, though you are not required to use e, so no other changes are needed. > + module.onclickBranchType = function() { > + /* Which branch type radio button was selected? */ > + var selectedRCS = module._getSelectedRCS(); > + var types = document.getElementsByName('field.branch_type'); > + var type = 'None'; > + var i; > + for (i = 0; i < types.length; i++) { I think you can move the var declaration for i into the loop. > + if (types[i].checked) { > + type = types[i].value; > + break; > + } > + } > + // Linked > + module.setEnabled('field.branch_location', type == 'link-lp-bzr'); > + // New, empty branch -- do nothing > + // Import > + var is_external = (type == 'import-external'); > + module.setEnabled('field.repo_url', is_external); > + module.setEnabled('field.cvs_module', > + (is_external & selectedRCS == 'CVS')); > + var rcs_types = module._rcs_types(); > + for (j = 0; j < rcs_types.length; j++) { I think this j needs a var. > + rcs_types[j].disabled = !is_external; > + } > + }; > + > + > + Y.on('domready', function() { > + var branch_types = document.getElementsByName('field.branch_type'); > + var i; > + for (i = 0; i < branch_types.length; i++) { > + branch_types[i].onclick = module.onclickBranchType; > + } > + var rcs_types = document.getElementsByName('field.rcs_type'); > + var rcs_types = module._rcs_types(); > + for (i = 0; i < rcs_types.length; i++) { > + rcs_types[i].onclick = module.onclickRcsType; > + } > + // Set the initial state. > + module.onclickRcsType(); > + module.onclickBranchType(); > + }); I Think the two *.onclick assignments are risky. We prefer to use YUI's on method, and in this example, that means we want to wrap the DOM object in a YUI object. So... Y.one(branch_types[i]).on('click', module.onclick_branch_type); Y.one(rcs_types[i]).on('click', module.onclick_rcs_type); There is some historic problems with getElementsByName in IE. I think this style works and resolved the issue of getting a YUI object with an on method. Y.all('input[name=field.rcs_type]').each(function(field) { field.on('click', module.onclick_branch_type);}); > + }, "0.1", {"requires": ["node", "DOM"]} > +); > === added file 'lib/canonical/launchpad/javascript/registry/tests/productseries-setbranch.html' > --- lib/canonical/launchpad/javascript/registry/tests/productseries-setbranch.html 1970-01-01 00:00:00 +0000 > +++ lib/canonical/launchpad/javascript/registry/tests/productseries-setbranch.html 2010-03-16 20:32:39 +0000 ... > + > + > + > + > + > + > + > +
> + > + > + > + > + > + > + > +
> + > + (Optional) > +
> + + name="field.branch_location" size="35" > + maxlength="" > + onkeypress="" style="" class=""> Do I really need to provide these attributes in the markup that the template generates? If not, I think empty attributes should be remove to clearly state what the script needs. > + > +
> +

The Bazaar branch for this series in > + Launchpad, if one exists.

> +
> +
> +