Merge lp:~bryce/launchpad/lp-586150-whiteboard-widening into lp:launchpad

[Bryce Harrington]

Gives additional horizontal space for the whiteboard section.

The Ubuntu project makes heavy use of the whiteboard for coordinating development tasks. Launchpad's stylesheet wraps lines to make prose read better. However with todo lists this tends to not work out as well. This change provides a bit more horizontal space, which makes the Ubuntu blueprint work item lists read a lot nicer.

[Curtis Hovey (sinzui)]

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
> --- lib/canonical/launchpad/icing/style-3-0.css.in 2010-05-17 10:56:27 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-05-28 03:48:33 +0000
> @@ -409,6 +409,10 @@
> /* Keep the abilty to hide list entries. */
> display: none;
> }
> +div.widetext p {
> + max-width: 60em;
> + margin-bottom: 1.0em;
> + }

This is a very narrow definition makes this an exception for the blueprint
white board. It cannot will not be resued. If we really want an exception for
*just* the blueprint whiteboard, this then belongs in the page's
head-epilogue slot.

This rule needs to accommodate the kinds of markup and circumstances that
happen in many places in Launchpad to be included in the global CSS. I think
we need this rule, so the selector must be generalised.

I think .wide will suffice since we have a .narrow. I should be permitted to
use this with a table or a form. lists that should be the same width as the
paragraphs they are with so div and p are too specific

/me looks at other wide cases.
    table#package-requests {
        width: 60em;
        margin: 1em 0;
        }

Score! Well we have found a common rule that blueprints and soyuz can used

I think the rule should be:
.wide {
    max-width: 60em;
    margin: 1em 0;
    }

Update your file and lib/lp/soyuz/templates/archive-macros.pt
    <table id="package-requests" class="wide listing"

And we can remove one more dead rule from style.css.

review: Approve (code ui)

[Bryce Harrington (bryce)]

I tried this bug it did not work:

.wide {
    max-width: 60em;
    margin: 1em 0;
    }

However if I change it to this, it does work:

.wide p {
    max-width: 60em;
    margin: 1em 0;
    }

I'm not sure where in Launchpad the package-requests table appears, so haven't been able to test how either of these CSS settings affect it. I've poked around the PPA pages where it looks like it should appear but can't seem to locate it, is there some trick to make that portlet show up?

[Bryce Harrington (bryce)]

Also, isn't 'max-width' a p-specific markup tag? Is that going to apply to tables with the same effect as 'width'?

[Curtis Hovey (sinzui)]

No, but ultimately your point is valid. max-width is for block elements, inline elements and table elements are ignored (display: inline and display:table respectively). In my defence, it was way past my bed time when I did that review; I should have waiting till morning when the caffeine had taken affect. We need to refine the rule so that we have the same semantics to provide comprable effect.

.wide * {
    max-width: 60em;
    margin: 1em 0;
    }
table.wide {
    max: 60em;
    margin: 1em 0;
    }

[Bryce Harrington (bryce)]

Okay, that looks more sensible.

However I notice the * has a curious effect on the whiteboard layout... see this screenshot:

  http://launchpadlibrarian.net/49280819/whiteboard-margin-increase.png

[Curtis Hovey (sinzui)]

Is that the correct screen cap? I do not see an whiteboard on the merge proposal page.

[Bryce Harrington (bryce)]

I kinda like how it looks with a bit of extra space between the lines... makes the text less harsh and dense. But the above is a bit too much. Cutting down to 0.5em looks more like I think it should.

OTOH if it's going to have weird side effects, maybe leaving the margin untweaked would be safest?

[Bryce Harrington (bryce)]

Oh and I notice you said 'max: 60em' but shouldn't that be 'width: 60em'? (I still haven't found where this particular table shows up so haven't been able to test it).

Anyway, with the above caveats I've pushed an update.

[Curtis Hovey (sinzui)]

Yes. you are correct about width 60em

The image does not show the whiteboard. I can image eveyything looks double-spaced since every wide element has 1em before and after it. We already have margin rules for elements, so I think we can drop the .wide * margin rule. This will let the default rules work. If you need to force an 1em between the whiteboard and the line in the page, hack it into the template.

[Bryce Harrington (bryce)]

> The image does not show the whiteboard.

Hmm, weird. Try again:
  http://launchpadlibrarian.net/49285310/whiteboard-doublespace.png

Here's how it looks with the current state of the branch (with the .wide * margin rules dropped):
  http://launchpadlibrarian.net/49285335/whiteboard-no-margin-tweaks.png

I think this looks fine, the CSS seems sane, and am happy to call it done if you are. Is there a need for anyone else to review this or shall I call it done and plan to land it once PQM has reopened?

[Curtis Hovey (sinzui)]

Let's call this done then. You can land it next week wne PQM opens.

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2010-05-17 10:56:27 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2010-06-02 20:11:28 +0000
@@ -164,6 +164,12 @@
     /* Wrap the text before the eye gets lost. */
     max-width: 45em;
     }
+.wide * {
+    max-width: 60em;
+    }
+table.wide {
+    width: 60em;
+    }
 pre, code, samp, tt, .console {
     font-size: 116%;
     margin-bottom: 0.8em;

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2010-04-27 12:38:38 +0000
+++ lib/canonical/launchpad/icing/style.css	2010-06-02 20:11:28 +0000
@@ -309,10 +309,6 @@
     padding: 0 1em;
     margin:0 1em 1em 0;
 }
-table#package-requests {
-    width: 60em;
-    margin: 1em 0;
-}
 table#package-requests td {
     padding: 0.5em;
 }

=== modified file 'lib/lp/blueprints/templates/specification-index.pt'
--- lib/lp/blueprints/templates/specification-index.pt	2009-11-07 18:51:07 +0000
+++ lib/lp/blueprints/templates/specification-index.pt	2010-06-02 20:11:28 +0000
@@ -270,43 +270,37 @@
                 </li>
               </ul>
             </div>
+
+            <div class="portlet" style="border-top: none;">
+              <h2 tal:condition="not:context/feedbackrequests">Feedback requests</h2>
+
+              <div tal:replace="structure context/@@+portlet-feedbackqueue" />
+
+              <ul class="horizontal">
+                <li tal:define="link context_menu/givefeedback"
+                    tal:condition="link/enabled">
+                  <a tal:replace="structure link/fmt:link" />
+                </li>
+                <li tal:define="link context_menu/requestfeedback"
+                    tal:condition="link/enabled">
+                  <a tal:replace="structure link/fmt:link" />
+                </li>
+              </ul>
+            </div>
+
         </div>
       </div>
     </div>
 
-    <div class="yui-g">
-      <div class="yui-u first">
-        <div class="portlet">
-          <h2>
-            Whiteboard
-            <a tal:replace="structure context_menu/whiteboard/fmt:icon" />
-         </h2>
-
-          <div
-            tal:content="structure context/whiteboard/fmt:text-to-html"
-            tal:condition="context/whiteboard">Whiteboard.</div>
-        </div>
-      </div>
-
-      <div class="yui-u">
-        <div class="portlet">
-          <h2
-            tal:condition="not:context/feedbackrequests">Feedback requests</h2>
-
-          <div tal:replace="structure context/@@+portlet-feedbackqueue" />
-
-          <ul class="horizontal">
-            <li tal:define="link context_menu/givefeedback"
-                 tal:condition="link/enabled">
-              <a tal:replace="structure link/fmt:link" />
-            </li>
-            <li tal:define="link context_menu/requestfeedback"
-                 tal:condition="link/enabled">
-              <a tal:replace="structure link/fmt:link" />
-            </li>
-          </ul>
-        </div>
-      </div>
+    <div class="portlet">
+      <h2>
+        Whiteboard
+        <a tal:replace="structure context_menu/whiteboard/fmt:icon" />
+      </h2>
+
+      <div class="wide"
+           tal:content="structure context/whiteboard/fmt:text-to-html"
+           tal:condition="context/whiteboard">Whiteboard.</div>
     </div>
 
     <div class="portlet">

=== modified file 'lib/lp/soyuz/templates/archive-macros.pt'
--- lib/lp/soyuz/templates/archive-macros.pt	2009-12-03 18:33:22 +0000
+++ lib/lp/soyuz/templates/archive-macros.pt	2010-06-02 20:11:28 +0000
@@ -219,7 +219,7 @@
     </p>
 
     <tal:request_list tal:condition="requests">
-      <table id="package-requests" class="listing"
+      <table id="package-requests" class="wide listing"
         tal:attributes="summary
                         string:Package copy requests for ${context/name}">
         <thead>