Merge lp:~salgado/lazr-js/picker-refactoring into lp:lazr-js

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

Change the picker to clean itself upon save, with config options to have it cleared or not upon save/cancel

[Guilherme Salgado (salgado)]

My changes have caused the picker not to be hidden when we click on the close button, so I had to do the changes below to fix it.

=== modified file 'src-js/lazrjs/picker/picker.js'
--- src-js/lazrjs/picker/picker.js 2009-11-12 16:02:51 +0000
+++ src-js/lazrjs/picker/picker.js 2009-11-12 16:58:01 +0000
@@ -72,6 +72,7 @@
     /**
      * Whether or not the widget should be cleared when the save event is
      * fired.
+ *
      * @property clear_on_save
      * @type Boolean
      * default true
@@ -81,6 +82,7 @@
     /**
      * Whether or not the widget should be cleared when the cancel event is
      * fired.
+ *
      * @property clear_on_cancel
      * @type Boolean
      * default false
@@ -630,6 +632,7 @@
      * @protected
      */
     _defaultCancel : function(e) {
+ Picker.superclass._defaultCancel.apply(this, arguments);
         if ( this.clear_on_cancel ) {
             this._clear();
         }

=== modified file 'src-js/lazrjs/picker/tests/picker.js'
--- src-js/lazrjs/picker/tests/picker.js 2009-11-12 16:02:51 +0000
+++ src-js/lazrjs/picker/tests/picker.js 2009-11-12 16:55:25 +0000
@@ -379,6 +379,14 @@
             }, 3000);
     },

+ test_cancel_event_hides_widget: function () {
+ this.picker.render();
+
+ this.picker.fire('cancel', 'bogus');
+ Assert.isFalse(
+ this.picker.get('visible'), "The widget should be hidden.");
+ },
+
     test_save_event_hides_widget: function () {
         this.picker.render();

[lp:~salgado/lazr-js/picker-refactoring]

131. By Guilherme Salgado on 2009-11-12

Unbreak the close button of the picker so that it hides the picker as it used to.

[Edwin Grubbs (edwin-grubbs)]

Hi Salgado,

This looks good. A couple of minor comments below.

merge-approved

-Edwin

> === modified file 'src-js/lazrjs/picker/picker.js'
> --- src-js/lazrjs/picker/picker.js 2009-11-12 16:02:51 +0000
> +++ src-js/lazrjs/picker/picker.js 2009-11-12 16:58:01 +0000
> @@ -72,6 +72,7 @@
> /**
> * Whether or not the widget should be cleared when the save event is
> * fired.

I think it would be clearer to say "Whether or not the *search box and result list* should be cleared".

> + *
> * @property clear_on_save
> * @type Boolean
> * default true
> @@ -81,6 +82,7 @@
> /**
> * Whether or not the widget should be cleared when the cancel event is
> * fired.
> + *

Same here.

> * @property clear_on_cancel
> * @type Boolean
> * default false
> @@ -630,6 +632,7 @@
> * @protected
> */
> _defaultCancel : function(e) {
> + Picker.superclass._defaultCancel.apply(this, arguments);
> if ( this.clear_on_cancel ) {
> this._clear();
> }
>

review: Approve (code)

[Guilherme Salgado (salgado)]

On Thu, 2009-11-12 at 17:18 +0000, Edwin Grubbs wrote:
> Review: Approve code
> Hi Salgado,
>
> This looks good. A couple of minor comments below.

Thanks Edwin,

I've done the changes you suggested and the other ones we discussed
off-list. Here's the diff

=== modified file 'src-js/lazrjs/picker/picker.js'
--- src-js/lazrjs/picker/picker.js 2009-11-12 17:06:57 +0000
+++ src-js/lazrjs/picker/picker.js 2009-11-12 18:03:52 +0000
@@ -70,26 +70,6 @@

 Y.extend(Picker, Y.lazr.PrettyOverlay, {
     /**
- * Whether or not the widget should be cleared when the save event is
- * fired.
- *
- * @property clear_on_save
- * @type Boolean
- * default true
- */
- clear_on_save: true,
-
- /**
- * Whether or not the widget should be cleared when the cancel event is
- * fired.
- *
- * @property clear_on_cancel
- * @type Boolean
- * default false
- */
- clear_on_cancel: false,
-
- /**
      * The search input box node.
      *
      * @property _search_input
@@ -625,7 +605,7 @@

     /**
      * By default, the cancel event just hides the widget, but you can
- * have it also cleared by setting clear_on_save to 'true'.
+ * have it also cleared by setting clear_on_cancel to 'true'.
      *
      * @method _defaultCancel
      * @param e {Event.Facade} An Event Facade object.
@@ -633,7 +613,7 @@
      */
     _defaultCancel : function(e) {
         Picker.superclass._defaultCancel.apply(this, arguments);
- if ( this.clear_on_cancel ) {
+ if ( this.get('clear_on_cancel') ) {
             this._clear();
         }
     },
@@ -650,7 +630,7 @@
      */
     _defaultSave : function(e) {
         this.hide();
- if ( this.clear_on_save ) {
+ if ( this.get('clear_on_save') ) {
             this._clear();
         }
     },
@@ -696,6 +676,24 @@

 Picker.ATTRS = {
     /**
+ * Whether or not the search box and result list should be cleared when
+ * the save event is fired.
+ *
+ * @attribute clear_on_save
+ * @type Boolean
+ */
+ clear_on_save: { value: true },
+
+ /**
+ * Whether or not the search box and result list should be cleared when
+ * the cancel event is fired.
+ *
+ * @attribute clear_on_cancel
+ * @type Boolean
+ */
+ clear_on_cancel: { value: false },
+
+ /**
      * Minimum number of characters that need to be entered in the search
      * string input before a search event will be fired. The search string
      * will be trimmed before testing the length.

=== modified file 'src-js/lazrjs/picker/tests/picker.js'
--- src-js/lazrjs/picker/tests/picker.js 2009-11-12 17:06:57 +0000
+++ src-js/lazrjs/picker/tests/picker.js 2009-11-12 17:56:28 +0000
@@ -406,13 +406,13 @@
     },

     test_save_does_not_clear_widget_when_clear_on_save_is_false: function () {
- this.picker.render();
- this.picker.clear_on_save = false;
+ picker = new Y.Picker({clear_on_save: false});
+ picker.render();

- this.picker._search_input.set('value', 'foo');
- this.picker.fire('save', 'bogus');
+ picker._search_input.set('value', 'foo');
+ picker.fire('save', 'bogus...

[lp:~salgado/lazr-js/picker-refactoring]

132. By Guilherme Salgado on 2009-11-12

Make the picker actually honor the new config values that specify whether or not it should clear itself.

[Edwin Grubbs (edwin-grubbs)]

Looks good.

[lp:~salgado/lazr-js/picker-refactoring]

133. By Guilherme Salgado on 2009-11-12

merge from trunk

=== modified file 'src-js/lazrjs/picker/picker.js'
--- src-js/lazrjs/picker/picker.js	2009-10-22 19:27:45 +0000
+++ src-js/lazrjs/picker/picker.js	2009-11-12 20:45:23 +0000
@@ -166,6 +166,10 @@
          * @preventable _defaultSave
          */
         this.publish(SAVE, { defaultFn: this._defaultSave } );
+
+        // Subscribe to the cancel event so that we can clear the widget when
+        // requested.
+        this.subscribe('cancel', this._defaultCancel);
     },
 
     /**
@@ -499,7 +503,7 @@
         // is changed, and reset the selected one to the first one.
         this.after('batchesChange', function (e) {
             this._syncBatchesUI();
-            if (this.get(SELECTED_BATCH) == 0){
+            if (this.get(SELECTED_BATCH) === 0){
                 // If the attribute is already set to the same value,
                 // the 'after' events won't be triggered, so we have
                 // to trigger it manually.
@@ -550,6 +554,21 @@
         this._search_input.focus();
     },
 
+    /*
+     * Clear all elements of the picker, resetting it to its original state.
+     *
+     * @method _clear
+     * @param e {Object} The event object.
+     * @protected
+     */
+    _clear: function() {
+        this._search_input.set('value', '');
+        this.set('error', '');
+        this.set('results', [{}]);
+        this._results_box.set('innerHTML', '');
+        this.set('batches', []);
+    },
+
     /**
      * Handle clicks on the 'Search' button or entering the enter key in the
      * search field.  This fires the search event.
@@ -585,8 +604,25 @@
     },
 
     /**
-     * By default, the save event simply hides the widget. The search entered
-     * by the user is passed in the first details attribute of the event.
+     * By default, the cancel event just hides the widget, but you can
+     * have it also cleared by setting clear_on_cancel to 'true'.
+     *
+     * @method _defaultCancel
+     * @param e {Event.Facade} An Event Facade object.
+     * @protected
+     */
+    _defaultCancel : function(e) {
+        Picker.superclass._defaultCancel.apply(this, arguments);
+        if ( this.get('clear_on_cancel') ) {
+            this._clear();
+        }
+    },
+
+    /**
+     * By default, the save event clears and hides the widget, but you can
+     * have it not cleared by setting clear_on_save to 'false'. The search
+     * entered by the user is passed in the first details attribute of the
+     * event.
      *
      * @method _defaultSave
      * @param e {Event.Facade} An Event Facade object.
@@ -594,6 +630,9 @@
      */
     _defaultSave : function(e) {
         this.hide();
+        if ( this.get('clear_on_save') ) {
+            this._clear();
+        }
     },
 
     /**
@@ -637,6 +676,24 @@
 
 Picker.ATTRS = {
     /**
+     * Whether or not the search box and result list should be cleared when
+     * the save event is fired.
+     *
+     * @attribute clear_on_save
+     * @type Boolean
+     */
+    clear_on_save: { value: true },
+
+    /**
+     * Whether or not the search box and result list should be cleared when
+     * the cancel event is fired.
+     *
+     * @attribute clear_on_cancel
+     * @type Boolean
+     */
+    clear_on_cancel: { value: false },
+
+    /**
      * Minimum number of characters that need to be entered in the search
      * string input before a search event will be fired. The search string
      * will be trimmed before testing the length.

=== modified file 'src-js/lazrjs/picker/tests/picker.js'
--- src-js/lazrjs/picker/tests/picker.js	2009-10-21 21:43:07 +0000
+++ src-js/lazrjs/picker/tests/picker.js	2009-11-12 20:45:23 +0000
@@ -206,7 +206,7 @@
             [false, true, false, true], results.hasClass(Y.lazr.ui.CSS_ODD));
     },
 
-    test_clicking_search_button_fires_save_event: function () {
+    test_clicking_search_button_fires_search_event: function () {
         this.picker.render();
 
         var bb = this.picker.get('boundingBox');
@@ -379,6 +379,14 @@
             }, 3000);
     },
 
+    test_cancel_event_hides_widget: function () {
+        this.picker.render();
+
+        this.picker.fire('cancel', 'bogus');
+        Assert.isFalse(
+            this.picker.get('visible'), "The widget should be hidden.");
+    },
+
     test_save_event_hides_widget: function () {
         this.picker.render();
 
@@ -387,6 +395,48 @@
             this.picker.get('visible'), "The widget should be hidden.");
     },
 
+    test_save_event_clears_widget_by_default: function () {
+        this.picker.render();
+
+        this.picker._search_input.set('value', 'foo');
+        this.picker.fire('save', 'bogus');
+        Assert.areEqual(
+            '', this.picker._search_input.get('value'),
+            "The widget hasn't been cleared");
+    },
+
+    test_save_does_not_clear_widget_when_clear_on_save_is_false: function () {
+        picker = new Y.Picker({clear_on_save: false});
+        picker.render();
+
+        picker._search_input.set('value', 'foo');
+        picker.fire('save', 'bogus');
+        Assert.areEqual(
+            'foo', picker._search_input.get('value'),
+            "The widget has been cleared but it should not");
+    },
+
+    test_cancel_event_does_not_clear_widget_by_default: function () {
+        this.picker.render();
+
+        this.picker._search_input.set('value', 'foo');
+        this.picker.fire('cancel', 'bogus');
+        Assert.areEqual(
+            'foo', this.picker._search_input.get('value'),
+            "The widget has been cleared but it should not");
+    },
+
+    test_cancel_event_clears_widget_when_clear_on_cancel_is_true: function () {
+        picker = new Y.Picker({clear_on_cancel: true});
+        picker.render();
+
+        picker._search_input.set('value', 'foo');
+        picker.fire('cancel', 'bogus');
+        Assert.areEqual(
+            '', picker._search_input.get('value'),
+            "The widget hasn't been cleared");
+    },
+
     test_search_clears_any_eror: function () {
         this.picker.render();
         this.picker.set('error', "An error");