launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-436247 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #436247 in Launchpad itself: "links in side portlets are too close"
  https://bugs.launchpad.net/launchpad/+bug/436247

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-436247/+merge/66336

Bug 436247 is about the lists of "things" in portlets should be marked
up with <ul> and <li> instead of <div>.  This branch fixes that.

There are before and after screen shots attached (hopefully, if not,
I'll add a comment later pointing to them).

Lint: there is no lint, except for
./lib/canonical/launchpad/icing/style-3-0.css, which I'm ignoring
because fixing it would be a project in itself.  Maybe next time.

Tests: there aren't any affected tests.

QA: check to see if pages with portlets (bug pages especially) look
right and lists therein are built with unordered lists.

-- 
https://code.launchpad.net/~benji/launchpad/bug-436247/+merge/66336
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-436247 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css	2011-06-28 19:24:39 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css	2011-06-29 15:35:50 +0000
@@ -1949,7 +1949,7 @@
     /* Override sprite style for edit icon on "Mark as duplicate"
        to make the text not appear on a second line.
     */
-    display:inline;
+    display: inline;
     }
 
 /* Picker styles */

=== modified file 'lib/lp/bugs/javascript/subscribers_list.js'
--- lib/lp/bugs/javascript/subscribers_list.js	2011-06-27 14:23:58 +0000
+++ lib/lp/bugs/javascript/subscribers_list.js	2011-06-29 15:35:50 +0000
@@ -581,7 +581,7 @@
             .set('text', subscriber_levels[level]));
     // Node listing the actual subscribers.
     node.appendChild(
-        Y.Node.create('<div />')
+        Y.Node.create('<ul />')
             .addClass(CSS_CLASSES.list));
     return node;
 };
@@ -726,7 +726,7 @@
  * @return {Object} Node containing a subscriber link.
  */
 SubscribersList.prototype._createSubscriberNode = function(subscriber) {
-    var subscriber_node = Y.Node.create('<div />')
+    var subscriber_node = Y.Node.create('<li />')
         .addClass(CSS_CLASSES.subscriber);
 
     var subscriber_link = Y.Node.create('<a />');

=== modified file 'lib/lp/bugs/templates/bug-portlet-actions.pt'
--- lib/lp/bugs/templates/bug-portlet-actions.pt	2011-04-12 17:37:13 +0000
+++ lib/lp/bugs/templates/bug-portlet-actions.pt	2011-06-29 15:35:50 +0000
@@ -6,8 +6,8 @@
   class="portlet vertical"
   tal:define="context_menu context/menu:context"
 >
-  <div id="duplicate-actions">
-    <span id="mark-duplicate-text"
+  <ul id="duplicate-actions">
+    <li id="mark-duplicate-text"
         tal:define="link context_menu/markduplicate"
         tal:condition="python: link.enabled and not
                       context.duplicateof"
@@ -16,17 +16,12 @@
     <a
       tal:attributes="href link/path"
       class="menu-link-mark-dupe">Mark as duplicate</a>
-    </span>
-    <tal:block
-      tal:condition="context/number_of_duplicates"
-      tal:define="number_of_dupes context/number_of_duplicates"
-    >
-    </tal:block>
+    </li>
     <tal:block
       tal:define="duplicateof context/duplicateof"
       tal:condition="duplicateof"
     >
-      <a
+      <li><a
         tal:define="link context_menu/markduplicate"
         tal:condition="python: link.enabled"
         id="change_duplicate_bug"
@@ -36,42 +31,33 @@
       <a
         tal:condition="duplicateof/required:launchpad.View"
         tal:attributes="href duplicateof/fmt:url; title
-           duplicateof/title; style string:margin-right: 4px;
-           id string:duplicate-of;"
+            duplicateof/title; style string:margin-right: 4px;
+            id string:duplicate-of;"
         tal:content="string:bug #${duplicateof/id}"
       >bug #42</a>
         <span
           tal:condition="not:duplicateof/required:launchpad.View"
-          tal:replace="string:a private bug" />
-    </span>
+          tal:replace="string:a private bug" /></span></li>
     </tal:block>
-  </div>
-  <tal:createquestion
-    define="link context_menu/createquestion"
-    condition="link/enabled"
-    replace="structure link/render" />
-
-  <tal:removequestion
-    define="link context_menu/removequestion"
-    condition="link/enabled"
-    replace="structure link/render" />
-
-  <div>
-    <tal:addbranch
-        define="link context_menu/addbranch"
-        condition="link/enabled"
-        replace="structure link/render" />
-  </div>
-  <div>
-    <tal:linktocve
-        define="link context_menu/linktocve"
-        condition="link/enabled"
-        replace="structure link/render" />
-  </div>
-  <div>
-    <tal:unlinkcve
-        define="link context_menu/unlinkcve"
-        condition="link/enabled"
-        replace="structure link/render" />
-  </div>
+    <li
+      tal:define="link context_menu/createquestion"
+      tal:condition="link/enabled"
+      tal:content="structure link/render" />
+    <li
+      tal:define="link context_menu/removequestion"
+      tal:condition="link/enabled"
+      tal:content="structure link/render" />
+    <li
+      tal:define="link context_menu/addbranch"
+      tal:condition="link/enabled"
+      tal:content="structure link/render" />
+    <li
+      tal:define="link context_menu/linktocve"
+      tal:condition="link/enabled"
+      tal:content="structure link/render" />
+    <li
+      tal:define="link context_menu/unlinkcve"
+      tal:condition="link/enabled"
+      tal:content="structure link/render" />
+  </ul>
 </div>

=== modified file 'lib/lp/bugs/templates/bug-portlet-subscription.pt'
--- lib/lp/bugs/templates/bug-portlet-subscription.pt	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/templates/bug-portlet-subscription.pt	2011-06-29 15:35:50 +0000
@@ -36,17 +36,19 @@
             tal:content="view/notifications_text/muted" />
     </div>
     <div id="sub-unsub-spinner">Subscribing...</div>
-    <tal:show-mute condition="view/user">
-      <div tal:attributes="class python:
-                'hidden' if not view.user_should_see_mute_link else None"
-           id="mute-link-container">
-        <span tal:replace="structure context_menu/mute_subscription/render"
-        />&nbsp;<a target="help" class="sprite maybe mute-help"
-            href="/+help/subscription-mute.html"
-          >&nbsp;<span class="invisible-link">Mute help</span></a>
-      </div>
-    </tal:show-mute>
-    <div tal:content="structure context_menu/editsubscriptions/render" />
+    <ul>
+      <li condition="view/user">
+        <div tal:attributes="class python:
+                  'hidden' if not view.user_should_see_mute_link else None"
+            id="mute-link-container">
+          <span tal:replace="structure context_menu/mute_subscription/render"
+          />&nbsp;<a target="help" class="sprite maybe mute-help"
+              href="/+help/subscription-mute.html"
+            >&nbsp;<span class="invisible-link">Mute help</span></a>
+        </div>
+      </li>
+      <li tal:content="structure context_menu/editsubscriptions/render" />
+    </ul>
   </div>
   <script type="text/javascript">
     LPS.use('io-base', 'node',