launchpad-reviewers team mailing list archive

Thread
Date

[Merge] lp:~wallyworld/launchpad/picker-textfield-plugin into lp:launchpad

To: mp+57819@xxxxxxxxxxxxxxxxxx
From: Ian Booth <ian.booth@xxxxxxxxxxxxx>
Date: Fri, 15 Apr 2011 08:28:41 -0000
Reply-to: mp+57819@xxxxxxxxxxxxxxxxxx
Sender: bounces@xxxxxxxxxxxxx

Ian Booth has proposed merging lp:~wallyworld/launchpad/picker-textfield-plugin into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #761494 in Launchpad itself: "picker doesn't save selected value into associated textfield"
  https://bugs.launchpad.net/launchpad/+bug/761494

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/picker-textfield-plugin/+merge/57819

On forms where a popup picker is used to select a value, the selected value is supposed to be copied to the relevant textfield. It isn't.

== Implementation ==

The recent change to add validation support had to call e.preventDefault() to stop the default 'save' event listener from prematurely closing the popup before the validation could be done. However, this had the unintended side effect of bypassing all other registered event subscribers, even though they are not the default one. Go figure. 

The fix/work around is to add a line of javascript to register an empty default 'save' event listener instead of calling e.preventDefault().

== Tests ==

Renamed lp/app/javascript/tests/picker.[js|html] to test_picker.[js|html] to make naming consistent

There was no existing test in the lp tests for this functionality so I added a new one:

test_picker.js -> test_TextFieldPickerPlugin_selected_item_is_saved()

== Lint ==

Linting changed files:
  lib/lp/app/javascript/picker.js
  lib/lp/app/javascript/tests/test_picker.html
  lib/lp/app/javascript/tests/test_picker.js

./lib/lp/app/javascript/picker.js
      92: Expected '!==' and instead saw '!='.
     220: Expected '!==' and instead saw '!='.
     294: Expected '!==' and instead saw '!='.
     342: Expected '!==' and instead saw '!='.
     342: Expected '!==' and instead saw '!='.
     413: Move 'var' declarations to the top of the function.
     413: Stopping.  (85% scanned).
       0: JSLINT had a fatal error

-- 
https://code.launchpad.net/~wallyworld/launchpad/picker-textfield-plugin/+merge/57819
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/picker-textfield-plugin into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js	2011-04-13 06:03:33 +0000
+++ lib/lp/app/javascript/picker.js	2011-04-15 08:28:38 +0000
@@ -358,9 +358,13 @@
         });
     var picker = new Y.lazr.Picker(new_config);
 
+    // We don't want the Y.lazr.Picker default save to fire since this hides
+    // the form. We want to do this ourselves after any validation has had a
+    // chance to be performed.
+    picker.publish('save', { defaultFn: function(){} } );
+    
     picker.subscribe('save', function (e) {
         Y.log('Got save event.');
-        e.preventDefault();
         var picker_result = e.details[Y.lazr.Picker.SAVE_RESULT];
         var do_save = function() {
             if (Y.Lang.isFunction(config.save)) {

=== renamed file 'lib/lp/app/javascript/tests/picker.html' => 'lib/lp/app/javascript/tests/test_picker.html'
--- lib/lp/app/javascript/tests/picker.html	2011-04-11 00:18:45 +0000
+++ lib/lp/app/javascript/tests/test_picker.html	2011-04-15 08:28:38 +0000
@@ -24,7 +24,7 @@
   <script type="text/javascript" src="../picker.js"></script>
 
   <!-- The test suite -->
-  <script type="text/javascript" src="picker.js"></script>
+  <script type="text/javascript" src="test_picker.js"></script>
 </head>
 <body class="yui3-skin-sam">
   <div class="yui3-widget yui3-activator yui3-activator-focused">

=== renamed file 'lib/lp/app/javascript/tests/picker.js' => 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/picker.js	2011-04-13 05:56:12 +0000
+++ lib/lp/app/javascript/tests/test_picker.js	2011-04-15 08:28:38 +0000
@@ -118,6 +118,29 @@
         Assert.isTrue(save_flag.event_has_fired, "save event wasn't fired.");
     },
 
+    test_TextFieldPickerPlugin_selected_item_is_saved: function () {
+        // Test that the picker saves the selected value to its associated
+        // textfield if one is defined.
+        var search_input = Y.Node.create(
+                '<input id="field.initval" value="foo" />');
+        node = Y.one(document.body).appendChild(search_input);
+        this.create_picker();
+        this.picker.plug(Y.lazr.TextFieldPickerPlugin,
+                         {input_element: '[id="field.initval"]'});
+        this.picker.set('results', this.vocabulary);
+        this.picker.render();
+        var got_focus = false;
+        search_input.on('focus', function(e) {
+            got_focus = true;
+        });
+        simulate(
+            this.picker.get('boundingBox').one('.yui3-picker-results'),
+                'li:nth-child(1)', 'click');
+        Assert.areEqual(
+            'fred', Y.one('[id="field.initval"]').get("value"));
+        Assert.isTrue(got_focus, "focus didn't go to the search input.");
+    },
+
     test_confirmation_yes: function() {
         // Test that the picker saves the selected value if the user answers
         // "Yes" to a confirmation request.