launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad UI Reviewers (launchpad-ui-reviewers): ui
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #687379 Merge proposal diff overlay is bigger than the screen and can't be closed
  https://bugs.launchpad.net/bugs/687379

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/diff-overlay-is-huuuuuuge/+merge/50196

Summary
=======

Diff overlay popups use the viewport for alignment, which isn't reliable. It results in diff popups frequently setting a top css attribute well off the page.

An easy way to address this is to align the overlay with the node being opened. This has the happy side effect of the overlay opening at the point the user is already looking at, so they don't need to scroll up to start reading.

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

* Spoke with Deryck about dev vs production css issues.

Proposed Fix
============

Change the overlay align statement to align on a node, rather than on the viewport.

Implementation
==============

lib/lp/code/javascript/branchmergeproposal.diff.js
--------------------------------------------------
Changed the align parameter on the overlay to use the node provided to the popup function for alignment, which is the link to activate the popup. Switched from aligning center of node with center of popup to using the top right of both, so the diff popup appears within the page bounds.

Tests
=====

bin/test -t test_branch_popupdiff

Demo & QA
=========

Check the links on the linked bug (moving to qastaging, of course). The diff popup should start at the link, and continue downwards, within the left and right page bounds.

Also, see screenshots in first comment on this review.

Lint
====

make  lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/javascript/branchmergeproposal.diff.js

-- 
https://code.launchpad.net/~jcsackett/launchpad/diff-overlay-is-huuuuuuge/+merge/50196
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/diff-overlay-is-huuuuuuge into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.diff.js'
--- lib/lp/code/javascript/branchmergeproposal.diff.js	2011-02-04 16:43:35 +0000
+++ lib/lp/code/javascript/branchmergeproposal.diff.js	2011-02-17 17:44:25 +0000
@@ -73,8 +73,9 @@
                 var diff_overlay = new DiffOverlay({
                         bodyContent: Y.Node.create(formatted_diff),
                         align: {
-                            points: [Y.WidgetPositionAlign.CC,
-                                     Y.WidgetPositionAlign.CC]
+                            node: node,
+                            points: [Y.WidgetPositionAlign.TL,
+                                     Y.WidgetPositionAlign.TL]
                         },
                         progressbar: false
                     });