launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/picker-buttons-should-disappear into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #707234 in Launchpad itself: "Ajax controls disabled on bugs with many tasks due to multiple redundant copies of person picker make_picker function in view-source:https://bugs.launchpad.net/launchpad-project/+bugs?advanced=1";
  https://bugs.launchpad.net/launchpad/+bug/707234

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/picker-buttons-should-disappear/+merge/65562

Summary
=======
Personpicker introduces assign me and remove assignee buttons to all person assigning activities. However, it's confusing to see the assign me and remove me buttons when there are results from a search in the picker.

This branch makes the buttons disappear if there are search results, but provides the buttons if no results are found, in which case one might concievably want to assign one's self.

Proposed fix
============
Using the YUI event model, we want to detect the presence of results and hide the extra buttons if they exist after results are loaded.

Pre-implementation notes
========================
Spoke with Curtis Hovey, explaining likely implementation.

Implementation details
======================
The personpicker now has overrided the picker's bindUI call, adding a section that creates a handler for the 'resultChange' event fired by the picker. This handler checks for the result condition (e.g. are there results, and was a search done) and determines whether to apply the unseen class to the extra buttons based on that information.

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

Demo and Q/A
============
Do a search with the personpicker (e.g. on launchpad.dev/evolution/+edit-people). If you search for a value with results (e.g. 'name') the extra buttons are not shown. If you search for a value without results (e.g. 'nobody') the buttons are displayed.

Lint
====
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/widgets.js
  lib/lp/app/javascript/tests/test_personpicker.js
  lib/lp/app/javascript/tests/test_picker.js

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

The errors can be ignored as they're part of the javascript extension.
-- 
https://code.launchpad.net/~jcsackett/launchpad/picker-buttons-should-disappear/+merge/65562
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/picker-buttons-should-disappear into lp:launchpad.

=== modified file 'lib/lp/app/javascript/tests/test_personpicker.js'
--- lib/lp/app/javascript/tests/test_personpicker.js	2011-06-14 14:38:47 +0000
+++ lib/lp/app/javascript/tests/test_personpicker.js	2011-06-22 20:04:32 +0000
@@ -19,6 +19,22 @@
                 links: {me: '/~no-one'},
                 cache: {}
             };
+            this.vocabulary = [
+                {
+                    "value": "fred",
+                    "title": "Fred",
+                    "css": "sprite-person",
+                    "description": "fred@xxxxxxxxxxx",
+                    "api_uri": "~/fred"
+                },
+                {
+                    "value": "frieda",
+                    "title": "Frieda",
+                    "css": "sprite-person",
+                    "description": "frieda@xxxxxxxxxxx",
+                    "api_uri": "~/frieda"
+                }
+            ];
         },
 
         test_render: function () {
@@ -61,11 +77,30 @@
             personpicker = new Y.lp.app.widgets.PersonPicker(cfg);
             personpicker.render();
             personpicker.show();
-            
+
             Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
             Y.Assert.isUndefined(personpicker.remove_button);
             Y.Assert.isUndefined(personpicker.assign_me_button);
-
+        },
+
+        _change_results: function (picker, no_result) {
+            // simulates search by setting a value and the result
+            picker._search_input.set('value', 'foo');
+            if (no_result) {
+                picker.set('results', []);
+            } else {
+                picker.set('results', this.vocabulary);
+            }
+        },
+
+        test_buttons_vanish: function () {
+            personpicker = new Y.lp.app.widgets.PersonPicker(cfg);
+            personpicker.render();
+            this._change_results(personpicker, false);
+            Y.Assert.isTrue(personpicker._extra_buttons.hasClass('unseen'));
+
+            this._change_results(personpicker, true);
+            Y.Assert.isFalse(personpicker._extra_buttons.hasClass('unseen'));
         }
     }));
 

=== modified file 'lib/lp/app/javascript/widgets.js'
--- lib/lp/app/javascript/widgets.js	2011-06-21 21:43:31 +0000
+++ lib/lp/app/javascript/widgets.js	2011-06-22 20:04:32 +0000
@@ -78,6 +78,8 @@
 var PersonPicker = function() {
     PersonPicker.superclass.constructor.apply(this, arguments);
     this._extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
+    Y.after(this._renderPersonPickerUI, this, 'renderUI');
+    Y.after(this._syncPersonPickerResultsUI, this, 'resultsChange');
 };
 
 Y.extend(PersonPicker, namespace.Picker, {
@@ -116,8 +118,8 @@
         this.fire('save', {value: name});
     },
 
-    renderUI: function() {
-        this.constructor.superclass.renderUI.call(this);
+    _renderPersonPickerUI: function() {
+        //this.constructor.superclass.renderUI.apply(this, arguments);
         var remove_button, assign_me_button;
         var remove_button_text = "Remove assignee";
         var assign_me_button_text = "Assign me";
@@ -147,9 +149,21 @@
 
     syncUI: function() {
         // call Picker's sync
-        this.constructor.superclass.syncUI.call(this);
+        this.constructor.superclass.syncUI.apply(this, arguments);
         footer_slot = Y.one(footer_label);
         footer_slot.appendChild(this._extra_buttons);
+    },
+
+    bindUI: function() {
+        this.constructor.superclass.bindUI.apply(this, arguments);
+        this.after('resultsChange', function(e) {
+            var results = this.get('results');
+            if (this._search_input.get('value') && !results.length) {
+                this._extra_buttons.removeClass('unseen');
+            } else {
+                this._extra_buttons.addClass('unseen');
+            }
+        });
     }
 });
 PersonPicker.NAME = 'person-picker';