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