← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wgrant/launchpad/involvement-config-validity into lp:launchpad

 

William Grant has proposed merging lp:~wgrant/launchpad/involvement-config-validity into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/involvement-config-validity/+merge/98328

This branch makes the involvement portlet slightly more HTML-compliant and less broken when rendered as HTML5.

I replaced the table progress bar with a div > img. This ends up full width, looking a bit more sensible. It also doesn't suffer spacing issues when rendered as HTML5.

It was also rather broken in IE due to <legend> abuse. activate_collapsibles was originally designed for expanding fieldsets in forms, so it basically expects to be run on a fieldset.collapsible with a legend as the header, so the non-form collapsibles abuse form elements to appease activate_collapsibles. I altered activate_collapsibles and its CSS to work with any .collapsible, and changed the non-form users to use a div instead of a fieldset or legend. This makes the HTML slightly less invalid, and restores function in IE.

Before and after screenshot here: http://people.canonical.com/~wgrant/launchpad/involvement-portlet-config.png
-- 
https://code.launchpad.net/~wgrant/launchpad/involvement-config-validity/+merge/98328
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/involvement-config-validity into lp:launchpad.
=== modified file 'lib/canonical/launchpad/icing/css/forms.css'
--- lib/canonical/launchpad/icing/css/forms.css	2012-03-10 15:33:33 +0000
+++ lib/canonical/launchpad/icing/css/forms.css	2012-03-20 03:44:18 +0000
@@ -169,37 +169,39 @@
     color: #666;
     font-weight: bold;
     }
-fieldset.collapsible {
+.collapsible {
     /* Collapsible sections
        Some page sections are hidden by default, expanded by clicking a link.
        see lp.js:activate_collapsibles() */
     border: none;
     margin: 0;
+    }
+fieldset.collapsible {
     padding: 16px 0 0; /* "Add a comment/attachment" form in bug reports */
     }
-fieldset.collapsible h2 {
+.collapsible h2 {
     margin-top: 0;
-}
-fieldset .collapsed {
+    }
+.collapsible .collapsed {
     display: none;
     }
-fieldset .expanded {
+.collapsible .expanded {
     display: block;
     }
-fieldset.collapsible legend {
+.collapsible > :first-child {
     font-weight: normal;
     }
-.collapsible legend a, .collapsible legend a:hover {
+.collapsible > :first-child, .collapsible :first-child a:hover {
     text-decoration: none;
     }
+.collapsible > :first-child a span {
+    text-decoration: underline;
+    }
 img.collapseIcon {
     text-decoration: none;
     vertical-align: middle;
     }
-.collapsible legend a span {
-    text-decoration: underline;
-    }
-.collapsed {
+.collapsible .collapsed {
     border: none;
     margin-bottom: 0;
     }

=== modified file 'lib/lp/app/javascript/lp.js'
--- lib/lp/app/javascript/lp.js	2012-01-12 10:28:38 +0000
+++ lib/lp/app/javascript/lp.js	2012-03-20 03:44:18 +0000
@@ -61,7 +61,7 @@
     Y.lp.activate_collapsibles = function() {
         // CSS selector 'legend + *' gets the next sibling element.
         Y.lp.app.widgets.expander.createByCSS(
-            '.collapsible', 'legend', 'legend + *', true);
+            '.collapsible', '> :first-child', '> :first-child + *', true);
     };
 
     /**

=== modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
--- lib/lp/registry/templates/pillar-involvement-portlet.pt	2011-07-07 16:49:41 +0000
+++ lib/lp/registry/templates/pillar-involvement-portlet.pt	2012-03-20 03:44:18 +0000
@@ -56,29 +56,19 @@
 
     <tal:show-registration condition="registration">
       <h2>Configuration Progress</h2>
-      <div class="centered" id="progressbar">
-        <table width="80%" border="1">
-          <tr>
-            <td>
-              <img
-                 tal:attributes="alt string:${registration/done}% registration complete;
-                                 title string:${registration/done}% registration complete;
-                                 width registration/done;
-                                 style string:width: ${registration/done}%;"
-                 src="/@@/green-bar"
-                 height="10"/>
-            </td>
-          </tr>
-        </table>
+      <div id="progressbar" style="border: 1px solid gray; margin-bottom: 10px">
+        <img
+            tal:attributes="alt string:${registration/done}% registration complete;
+                            title string:${registration/done}% registration complete;
+                            style string:display: block;; width:${registration/done}%"
+            src="/@@/green-bar"
+            height="10"/>
       </div>
 
-      <div id="show-hide-configs"
-           class="collapsible">
-        <legend>
-          <div class="config-options" style="margin-bottom: 0.5em;">
-            Configuration options
-          </div>
-        </legend>
+      <div id="show-hide-configs" class="collapsible">
+        <div class="config-options">
+          Configuration options
+        </div>
         <div
           tal:attributes="class
             python: 'expanded'

=== modified file 'lib/lp/registry/templates/product-files.pt'
--- lib/lp/registry/templates/product-files.pt	2011-11-26 04:03:29 +0000
+++ lib/lp/registry/templates/product-files.pt	2012-03-20 03:44:18 +0000
@@ -67,8 +67,8 @@
                 <div tal:attributes="id release/version/fmt:css-id/release-information-"
                      tal:condition="python: release.release_notes or release.changelog">
 
-                  <fieldset class="collapsible" style="padding: 0px;">
-                    <legend>Release information</legend>
+                  <div class="collapsible">
+                    <div>Release information</div>
                     <div class="unseen">
                     <div tal:condition="release/release_notes">
                       <strong>Release notes:</strong>
@@ -87,9 +87,9 @@
                            tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">
                         ProductRelease.changelog.
                       </div>
-                  </div>
-                  </div>
-                  </fieldset>
+                    </div>
+                  </div>
+                  </div>
                 </div>
                 <div>
                   <table class="listing" style="margin-top: 1em;">

=== modified file 'lib/lp/registry/templates/product-index.pt'
--- lib/lp/registry/templates/product-index.pt	2012-02-01 15:31:32 +0000
+++ lib/lp/registry/templates/product-index.pt	2012-03-20 03:44:18 +0000
@@ -14,18 +14,6 @@
 
 <head>
   <tal:head-epilogue metal:fill-slot="head_epilogue">
-    <style type="text/css">
-      div.collapsible {
-        border:medium none;
-        margin:0;
-        padding:16px 0 0;
-      }
-
-      div.collapsed {
-        display: None
-      }
-    </style>
-
     <noscript>
       <style type="text/css">
         div.collapsible, div.collapsed {display: block;}

=== modified file 'lib/lp/registry/templates/productrelease-portlet-data.pt'
--- lib/lp/registry/templates/productrelease-portlet-data.pt	2011-11-26 04:03:29 +0000
+++ lib/lp/registry/templates/productrelease-portlet-data.pt	2012-03-20 03:44:18 +0000
@@ -81,15 +81,14 @@
 
     <h2>Changelog&nbsp;<a tal:replace="structure view/release/menu:context/edit/fmt:icon" /></h2>
 
-    <fieldset class="collapsible" style="padding: 0px;"
-      tal:condition="view/release/changelog">
-      <legend>View the full changelog</legend>
+    <div class="collapsible" tal:condition="view/release/changelog">
+      <div>View the full changelog</div>
 
-      <div id="changelog" class="unseen" style="margin-bottom: 0px;"
+      <div id="changelog" class="unseen"
         tal:content="structure view/release/changelog/fmt:obfuscate-email/fmt:text-to-html">
         ProductRelease.changelog.
       </div>
-    </fieldset>
+    </div>
 
     <p tal:condition="not: view/release/changelog">
       <em>This release does not have a changelog.</em>