launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers/+merge/64094

Summary
=======
After creating the new js personpicker widget, we realized that there was an alternative means of getting a person picker via lp.app.picker.addPickerPatcher, which created a picker via picker.create. Because it wasn't activated by means of the PersonPicker in popup.py, picker_type did not get set to person. 

Proposed fix
============
Set up addPickerPatcher to use a personpicker to create the "Assign me" and "Remove assignee" buttons as it needs them, since this is one of the major things added in the person picker, and we would like to avoid duplicate javascript.

Pre-implementation notes
========================
Spoke with Curtis Hovey and Ian Booth about the duplication and the idea of simply using personpicker to create the assignme and remove buttons in picker patcher.

Implementation details
======================
lib/lp/app/javascript/widgets.js
--------------------------------
Updated PersonPicker to generate the extra buttons based on the presence of show_assign_me and show_remove config options, so it can use the information passed along when the addPickerPatcher is used.

Also, added the remove and assign buttons as properties on the picker so that addPickerPatcher could easily override the functions attached to those buttons click events.

lib/lp/app/javascript/picker.js
-------------------------------
Removed the code generating the extra buttons in addPickerPatcher, since that's now done by the personpicker. Retained the custom assign_me and remove methods, which are now added to the personpicker's assign/remove buttons after purgeElement is called to remove the personpicker's usual events when those buttons are clicked.

Tests
=====
firefox lib/lp/app/javascript/tests/test_personpicker.html
firefox lib/lp/app/javascript/tests/test_picker.html

Demo and Q/A
============
Check the various pickers; they should all function the same as prior to this branch landing. This is just cleaning up code.

Launchpad lint
==============

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/widgets.js
  lib/lp/app/javascript/picker.js

./lib/lp/app/javascript/widgets.js
      14: 'Picker' has not been fully defined yet.
      62: 'PersonPicker' has not been fully defined yet.

I tried changing the lines causing this to use 'this' instead of 'Picker' or 'PersonPicker', but that broke the code. Perhaps this is an issue in the linter?
-- 
https://code.launchpad.net/~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers/+merge/64094
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js	2011-06-09 07:53:07 +0000
+++ lib/lp/app/javascript/picker.js	2011-06-09 22:36:35 +0000
@@ -43,6 +43,7 @@
     var remove_button_text = 'Remove';
     var null_display_value = 'None';
     var show_search_box = true;
+
     resource_uri = Y.lp.client.normalize_uri(resource_uri);
 
     if (config !== undefined) {
@@ -65,6 +66,10 @@
         if (config.show_search_box !== undefined) {
             show_search_box = config.show_search_box;
         }
+        if (config.picker_type === undefined &&
+                (show_assign_me_button || show_remove_button)) {
+            config.picker_type = 'person';
+        }
     }
 
     var content_box = Y.one('#' + content_box_id);
@@ -121,6 +126,8 @@
         });
     };
 
+    // Create a new assign_me and remove_assigne function that uses the
+    // patcher save method instead of the standard picker save.
     var assign_me = function () {
         picker.hide();
         save({
@@ -156,27 +163,16 @@
     config.save = save;
     var picker = namespace.create(vocabulary, config, activator);
     picker._resource_uri = resource_uri;
-    var extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
-    var remove_button, assign_me_button;
+    var remove_button = picker.remove_button;
+    var assign_me_button = picker.assign_me_button;
     if (show_remove_button) {
-        remove_button = Y.Node.create(
-            '<a class="yui-picker-remove-button bg-image" ' +
-            'href="javascript:void(0)" ' +
-            'style="background-image: url(/@@/remove); padding-right: 1em">' +
-            remove_button_text + '</a>');
+        Y.Event.purgeElement(remove_button);
         remove_button.on('click', remove);
-        extra_buttons.appendChild(remove_button);
     }
     if (show_assign_me_button) {
-        assign_me_button = Y.Node.create(
-            '<a class="yui-picker-assign-me-button bg-image" ' +
-            'href="javascript:void(0)" ' +
-            'style="background-image: url(/@@/person)">' +
-            'Assign Me</a>');
+        Y.Event.purgeElement(assign_me_button);
         assign_me_button.on('click', assign_me);
-        extra_buttons.appendChild(assign_me_button);
     }
-    picker.set('footer_slot', extra_buttons);
 
     // If we are to pre-load the vocab, we need a spinner.
     // We set it up here because we only want to do it once and the

=== modified file 'lib/lp/app/javascript/widgets.js'
--- lib/lp/app/javascript/widgets.js	2011-06-09 08:39:57 +0000
+++ lib/lp/app/javascript/widgets.js	2011-06-09 22:36:35 +0000
@@ -64,6 +64,22 @@
 };
 
 Y.extend(PersonPicker, namespace.Picker, {
+    initializer: function(cfg) {
+        PersonPicker.superclass.initializer.apply(this, arguments);
+
+        var show_assign_me_button = true;
+        var show_remove_button = true;
+
+        if (cfg.show_assign_me_button !== undefined) {
+            show_assign_me_button = cfg.show_assign_me_button;
+        }
+        if (cfg.show_remove_button != undefined) {
+            show_remove_button = cfg.show_remove_button;
+        }
+        this._show_assign_me_button = show_assign_me_button;
+        this._show_remove_button = show_remove_button;
+    },
+
     hide: function() {
         this.get('boundingBox').setStyle('display', 'none');
     },
@@ -87,21 +103,27 @@
         var remove_button_text = "Remove assignee";
         var assign_me_button_text = "Assign me";
 
-        remove_button = Y.Node.create(
-            '<a class="yui-picker-remove-button bg-image" ' +
-            'href="javascript:void(0)" ' +
-            'style="background-image: url(/@@/remove); padding-right: 1em">' +
-            remove_button_text + '</a>');
-        remove_button.on('click', this.remove, this);
-        this._extra_buttons.appendChild(remove_button);
+        if (this._show_remove_button) {
+            remove_button = Y.Node.create(
+                '<a class="yui-picker-remove-button bg-image" ' +
+                'href="javascript:void(0)" ' +
+                'style="background-image: url(/@@/remove); padding-right: 1em">' +
+                remove_button_text + '</a>');
+            remove_button.on('click', this.remove, this);
+            this._extra_buttons.appendChild(remove_button);
+            this.remove_button = remove_button;
+        }
 
-        assign_me_button = Y.Node.create(
-            '<a class="yui-picker-assign-me-button bg-image" ' +
-            'href="javascript:void(0)" ' +
-            'style="background-image: url(/@@/person)">' +
-            assign_me_button_text + '</a>');
-        assign_me_button.on('click', this.assign_me, this);
-        this._extra_buttons.appendChild(assign_me_button);
+        if (this._show_assign_me_button) {
+            assign_me_button = Y.Node.create(
+                '<a class="yui-picker-assign-me-button bg-image" ' +
+                'href="javascript:void(0)" ' +
+                'style="background-image: url(/@@/person)">' +
+                assign_me_button_text + '</a>');
+            assign_me_button.on('click', this.assign_me, this);
+            this._extra_buttons.appendChild(assign_me_button);
+            this.assign_me_button = assign_me_button;
+        }
     },
 
     syncUI: function() {