launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/localpackagediffs-commenting-bug-795573 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #795573 in Launchpad itself: "Comment box not disappearing after comment submission on +localpackagediffs"
  https://bugs.launchpad.net/launchpad/+bug/795573
  Bug #796233 in Launchpad itself: "It's possible to add an empty DistroSeriesDifferenceComment on DistroSeries:+localpackagediffs"
  https://bugs.launchpad.net/launchpad/+bug/796233

For more details, see:
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-commenting-bug-795573/+merge/64418

This branch makes the "Add comment" part of the DistroSeriesDifference
UI make a little more sense:

- Ditches toggle_comment_in_progress(). Functions that toggle things
  are almost always bad: changes should be made according to state,
  not according to how often a function has been called in the past.

- toggle_comment_in_progress() is replaced with a "start" and "end"
  handler in the primary LP client call in add_comment_handler().

- If the comment text is empty or all whitespace the comment box is
  red flashed and the comment is not submitted.

- When adding the new comment into the UI, it now slides in gradually,
  and is green flashed once it's fully in place.

- Once the new comment is in the database - not necessarily correctly
  rendered in the UI - the "Add comment" form is hidden (again via a
  slide). See the cb_callback argument to add_comment_handler() to see
  how that is done.

-- 
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-commenting-bug-795573/+merge/64418
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/localpackagediffs-commenting-bug-795573 into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-06-10 19:03:53 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-06-13 15:51:03 +0000
@@ -89,26 +89,6 @@
     };
 
     /**
-     * Toggle the spinner and enable/disable comment fields.
-     *
-     * @param comment_form {Node} The node that contains the relevant
-     *                     comment fields.
-     */
-    var toggle_comment_in_progress = function(comment_form) {
-        var spinner = comment_form.one('img');
-        if (Y.Lang.isNull(spinner)) {
-            comment_form.one('div.widget-bd').append(
-                '<img src="/@@/spinner" />');
-            comment_form.all('textarea,button').set(
-                'disabled', 'disabled');
-        } else {
-            comment_form.one('img').remove();
-            comment_form.all('textarea,button').set(
-                'disabled', '');
-        }
-    };
-
-    /**
      * Handle the add comment event.
      *
      * This method adds a comment via the API and update the UI.
@@ -117,12 +97,21 @@
      *                            fields.
      * @param api_uri {string} The uri for the distroseriesdifference to which
      *                the comment is to be added.
+     *
+     * @param cb_success {function}
+     *     Called when a comment has successfully been added. (Deferreds would
+     *     be awesome right about now.)
      */
-    var add_comment_handler = function(comment_form, api_uri) {
-
-        var comment_text = comment_form.one('textarea').get('value');
-
-        toggle_comment_in_progress(comment_form);
+    var add_comment_handler = function(comment_form, api_uri, cb_success) {
+
+        var comment_area = comment_form.one('textarea');
+        var comment_text = comment_area.get('value');
+
+        // Treat empty comments as mistakes.
+        if (Y.Lang.trim(comment_text).length == 0) {
+            Y.lazr.anim.red_flash({node: comment_area}).run();
+            return;
+        }
 
         var success_handler = function(comment_entry) {
             // Grab the XHTML representation of the comment
@@ -130,38 +119,51 @@
             config = {
                 on: {
                     success: function(comment_html) {
-                        comment_node = Y.Node.create(comment_html);
+                        var comment_node = Y.Node.create(comment_html);
                         comment_form.insert(comment_node, 'before');
-                        var anim = Y.lazr.anim.green_flash({
-                            node: comment_node
-                            });
-                        anim.run();
+                        var reveal = Y.lazr.effects.slide_out(comment_node);
+                        reveal.on("end", function() {
+                            Y.lazr.anim.green_flash(
+                                {node: comment_node}).run();
+                        });
+                        reveal.run();
                     }
-                    },
+                },
                 accept: Y.lp.client.XHTML
-                };
+            };
             lp_client.get(comment_entry.get('self_link'), config);
             comment_form.one('textarea').set('value', '');
-            toggle_comment_in_progress(comment_form);
+            cb_success();
         };
         var failure_handler = function(id, response) {
             // Re-enable field with red flash.
-            toggle_comment_in_progress(comment_form);
-            var anim = Y.lazr.anim.red_flash({
-                node: comment_form
-                });
-            anim.run();
+            Y.lazr.anim.red_flash({node: comment_form}).run();
         };
 
         var config = {
             on: {
                 success: success_handler,
-                failure: failure_handler
+                failure: failure_handler,
+                start: function() {
+                    // Show a spinner.
+                    comment_form.one('div.widget-bd')
+                        .append('<img src="/@@/spinner" />');
+                    // Disable the textarea and button.
+                    comment_form.all('textarea,button')
+                        .setAttribute('disabled', 'disabled');
                 },
+                end: function() {
+                    // Remove the spinner.
+                    comment_form.all('img').remove();
+                    // Enable the form.
+                    comment_form.all('textarea,button')
+                        .removeAttribute('disabled');
+                }
+            },
             parameters: {
                 comment: comment_text
-                }
-            };
+            }
+        };
         lp_client.named_post(api_uri, 'addComment', config);
     };
 
@@ -188,22 +190,37 @@
 
         // The comment area should slide in when the 'Add comment'
         // action is clicked.
-        var slide;
-        placeholder.one('a.widget-hd').on('click', function(e) {
-            e.preventDefault();
-            if (!slide) {
-                slide = Y.lazr.effects.slide_out(
+        var slide_anim = null;
+        var slide = function(direction) {
+            // Slide out if direction is true, slide in if direction
+            // is false, otherwise do the opposite of what's being
+            // animated right now.
+            if (slide_anim === null) {
+                slide_anim = Y.lazr.effects.slide_out(
                     placeholder.one('div.widget-bd'));
-            } else {
-                slide.set('reverse', !slide.get('reverse'));
-            }
-            slide.stop();
-            slide.run();
-        });
+                if (Y.Lang.isBoolean(direction)) {
+                    slide_anim.set("reverse", !direction);
+                }
+            }
+            else {
+                if (Y.Lang.isBoolean(direction)) {
+                    slide_anim.set("reverse", !direction);
+                }
+                else {
+                    slide_anim.set('reverse', !slide_anim.get('reverse'));
+                }
+                slide_anim.stop();
+            }
+            slide_anim.run();
+        };
+        var slide_out = function() { slide(false); };
+
+        placeholder.one('a.widget-hd').on(
+            'click', function(e) { e.preventDefault(); slide(); });
 
         placeholder.one('button').on('click', function(e) {
             e.preventDefault();
-            add_comment_handler(placeholder, api_uri);
+            add_comment_handler(placeholder, api_uri, slide_out);
         });
     };