launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/diff-overlay-close into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/diff-overlay-close/+merge/54032

Summary
=======

The diff overlay for merge proposals (as seen on the branch index or bug index page) has a close button that doesn't do anything. This just adds a simple close function to the overlay bound to the close button's click event.

Preimplementation
=================

Spoke with Deryck Hodge about the reason for the breakage and ways to fix it. 

Implementation
==============
lib/lp/code/javascript/branchmergeproposal.diff.js
--------------------------------------------------
A click handler is bound to the close button that dismisses the diff overlay.

Tests
=====

There's not really a good test for this that I can see; we're testing a lot of interaction between things. Similarly, windmill tests are bad, because they suck, and we're not testing data back and forth between page and server. I'm at a loss. If my assumptions are wrong, call me out and I'll add tests.

QA
==

The diff overlay in the example in the bug should be dismissable via the close button.
-- 
https://code.launchpad.net/~jcsackett/launchpad/diff-overlay-close/+merge/54032
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/diff-overlay-close into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.diff.js'
--- lib/lp/code/javascript/branchmergeproposal.diff.js	2011-02-24 00:23:04 +0000
+++ lib/lp/code/javascript/branchmergeproposal.diff.js	2011-03-18 16:30:32 +0000
@@ -24,12 +24,22 @@
  */
 var DiffOverlay = function() {
     DiffOverlay.superclass.constructor.apply(this, arguments);
+    Y.after(this._dismissDiff, this, 'syncUI')
 };
 
 Y.extend(DiffOverlay, Y.lazr.PrettyOverlay, {
         bindUI: function() {
             // call PrettyOverlay's bindUI
             this.constructor.superclass.bindUI.call(this);
+        },
+
+        _dismissDiff: function() {
+            var cancel_icon = Y.one('.close-button');
+
+            // Setup a click handler to dismiss the diff overlay.
+            cancel_icon.on('click', function(e) {
+                Y.one('.yui3-diff-overlay').setStyle('display', 'none');
+            });
         }
     });