launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-812044 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #812044 in Launchpad itself: "Expanded suggestions on +filebug leak branch icon"
  https://bugs.launchpad.net/launchpad/+bug/812044

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-812044/+merge/68263

= Bug 812044 =

Since we are using CSS sprites for the new expander treeCollapsed/treeExpanded icons, expander icon needs to be set to an element with limited height, or other sprites may "spill" into our node.

This is illustrated by wgrant's screenshot attached to the bug:

  https://launchpadlibrarian.net/75408979/bad-sprite.png

== Proposed fix ==

As discussed with Huw, we need to wrap the element inside another span or something like that, to avoid sprite spillage.  That's what I do, but since the original code allowed one to click on the entire contents area to unfold/fold bug details, I introduce a separate click handler for that on the table row.

Along the way, I replace self-closing <label /> tags with <label>&#8203;</label> tags so bug priority icon shows up in webkit-based browsers (tested in Chromium and Epiphany). &#8203; is Unicode zero width space.

I replace &nbsp; on the bug task page with zero width space as well for prettier display.

== Tests ==

lib/lp/bugs/javascript/tests/test_filebug_dupfinder.html

== Demo and Q/A ==

Go to https://launchpad.dev/firefox/+filebug and search for "a". Try expanding/collapsing details for similar bugs found.  No other icons should show up for any of the items, and expansion should continue to work wherever it's clicked (eg. on the bug title or always visible details).

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/filebug_dupefinder.js
  lib/lp/bugs/templates/bugtarget-macros-filebug.pt
  lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt

./lib/lp/bugs/javascript/filebug_dupefinder.js
     127: Expected '!==' and instead saw '!='.
     149: Expected '===' and instead saw '=='.
     416: Expected '===' and instead saw '=='.
     336: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~danilo/launchpad/bug-812044/+merge/68263
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-812044 into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/filebug_dupefinder.js'
--- lib/lp/bugs/javascript/filebug_dupefinder.js	2011-07-18 09:23:10 +0000
+++ lib/lp/bugs/javascript/filebug_dupefinder.js	2011-07-18 14:51:42 +0000
@@ -363,14 +363,21 @@
     if (bug_already_reported_expanders.size() > 0) {
         // Set up the onclick handlers for the expanders.
         Y.each(Y.all('.similar-bug'), function(row) {
+            var bug_expander = row.one('span.expander');
             var bug_details_div = row.one('div.duplicate-details');
             var bug_title_link = row.one('.duplicate-bug-link');
             bug_title_link.addClass('js-action');
             var view_bug_link = row.one('.view-bug-link');
             var expander = new Y.lp.app.widgets.expander.Expander(
-                row, bug_details_div);
+                bug_expander, bug_details_div);
             expander.setUp();
 
+            // Entire row, when clicked, expands/folds bug details.
+            row.on('click', function(e) {
+                e.halt();
+                expander.render(!expander.isExpanded());
+            });
+
             // The "view this bug" link shouldn't trigger the
             // collapsible, so we stop the event from propagating.
             view_bug_link.on('click', function(e) {

=== modified file 'lib/lp/bugs/templates/bugtarget-macros-filebug.pt'
--- lib/lp/bugs/templates/bugtarget-macros-filebug.pt	2011-07-18 09:23:10 +0000
+++ lib/lp/bugs/templates/bugtarget-macros-filebug.pt	2011-07-18 14:51:42 +0000
@@ -347,10 +347,17 @@
         <tbody>
           <tr>
             <td class="bug-already-reported-expander">
+              <tal:comment condition="nothing">
+                We use zero-width space (&#8203;) to force the otherwise
+                empty span using sprite image to show up.
+              </tal:comment>
+              <span class="expander">&#8203;</span>
               <label tal:attributes="for string:bug-already-reported-as-${bug/id};
                                      class bugtask/image:sprite_css"
-                     tal:condition="bugtask"/>
-              <label class="sprite bug" tal:condition="not:bugtask"/>
+                     tal:condition="bugtask">&#8203;</label>
+              <label class="sprite bug" tal:condition="not:bugtask">
+                &#8203;
+              </label>
             </td>
             <td>
               <div>

=== modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
--- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2011-07-18 09:23:10 +0000
+++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2011-07-18 14:51:42 +0000
@@ -16,7 +16,7 @@
         <a tal:condition="expandable"
            tal:attributes="href tasklink" class="bugtask-expander">
           <tal:link_text tal:replace="expander_link_text" />
-          &nbsp;
+          &#8203;
         </a>
         <tal:no_expander tal:condition="not: expandable"
                          tal:replace="expander_link_text" />