> This is my branch to fix timeout issues and IProject issues with the
> milestone page and improve the layout.
> 

Hi Curtis, thanks for all the details here - and for finding a solution
to a common problem!

r=me (code/ui*)

I've got some questions below, but I'll leave it up to you whether they
are worth considering.

>     lp:~sinzui/launchpad/milestone-design-oops
>     Diff size: 155
>     Launchpad bug: https://bugs.launchpad.net/bugs/430726
>     Test command: ./bin/test -vv -t "milestone-views|object-milestones"
>     Pre-implementation: wgrant
>     Target release: 3.1.0

Did I miss 3.0?

> 
> 
> = Fix timeout issues and IProject issues with the milestone page =
> 
> There is a content and a design problem with the milestone page.
> 
> 1. There are many more timeouts because of the cost of showing the bug tags.
>     Remove them, they do not work well are they are in the design.
> 
> 2. For most users, the side bar has one link, and if the context is an
>    IProject, there is no side bar. The content does not fill the page well.
>    Remove the side bar from the IProject case, consider moving the three
>    action links inline or float a fake side bar so that the content can fill
>    the page.
> 
> 
> == Rules ==
> 
> 1. Removed the tags from the page.

Great.

> 
> 2. This is hard to do. I had several failures before settling on the CSS
>    approach.
>    a) I could not conditional set the page macro

I haven't need to do this but, if it's helpful, I found an example at:

http://docs.zope.org/zope2/zope2book/source/ZPT.html#macro-details

I assume this should work the same for us?

>    b) I could not reliably make a mock side bar in a main_only template --
>       If someone changed the yui-* definition in the CSS, the template would
>       be out of sync.

Yep, good catch.

>    c) I tried using CSS to collapse the yui-* widths to reclaim the space,
>       but Firefox ignored them
>    d) I decided to give up a just add a conditional CSS block that hides the
>       empty background of the side bar.

This seems to work well.

> 
> 3. I then saw that the Project milestone table is still very compressed
>    because it has a "Product" column. Well that is not right. It should
>    read "Project"
> 
> 4. The I experimented with a new class that was on a different z-index and
>    very wide. The bug and spec work items filled the space very well.

Great.

> 
> 5. I decided to a .wide to the release files portlets so that its description
>    was not compressed.

Yes, I agree it looks better.

> 
> 
> == QA ==
> 
> The side bar and wide tables show look like these images.
>     http://people.canonical.com/~curtis/milestones/
> 
> 
> == Lint ==
> 
> Linting changed files:
>   lib/canonical/launchpad/icing/style-3-0.css
>   lib/lp/registry/browser/tests/milestone-views.txt
>   lib/lp/registry/stories/milestone/object-milestones.txt
>   lib/lp/registry/templates/milestone-index.pt
> 
> 
> == Test ==
> 
>     * lib/lp/registry/browser/tests/milestone-views.txt
>       * Added a test to verify that CSS is added to hide the project group
>         milestone side bar.
>     * lib/lp/registry/stories/milestone/object-milestones.txt
>       * Updated the test to verify that the UI uses "Project", not "Product".
> 
> 
> == Implementation ==
> 
>     * lib/canonical/launchpad/icing/style-3-0.css
>       * Added a wide rule for content to reclaim the space lost to the
>         side bar.
>     * lib/lp/registry/templates/milestone-index.pt
>       * Added CSS for the is_project_milestone condition
>       * Added .wide to the product release files portlet and the bug/blueprint
>         work items portlet
>       * Removed the bug tags that were cause a zillion queries.

Heh.

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> --- lib/canonical/launchpad/icing/style-3-0.css	2009-09-14 10:13:55 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css	2009-09-17 07:26:10 +0000
> @@ -237,7 +237,10 @@
>      padding: 0 0 0.5em 0;
>      margin: 0 0 1em;
>      }
> -
> +.wide {
> +    z-index: 10;
> +    width: 131%;
> +    }

I wonder if we should give this a more meaningful name, perhaps something
like 'full-width-main-side'? Maybe that' too verbose. See what you think.


>  .warning.message {
>      margin-top: 17px;
>      }
> 
> === modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
> --- lib/lp/registry/browser/tests/milestone-views.txt	2009-09-11 20:38:21 +0000
> +++ lib/lp/registry/browser/tests/milestone-views.txt	2009-09-17 07:26:10 +0000
> @@ -259,6 +259,29 @@
>  
>      >>> login_person(person)
>  
> +
> +Project milestones
> +------------------
> +
> +The projectgroup milestones are virtual and cannot be modified. The template
> +generates CSS that hides the space occupied by the side portlets.
> +
> +    >>> projectgroup = factory.makeProject(name='flock')
> +    >>> product.project = projectgroup
> +    >>> project_milestone = projectgroup.getMilestone('kakapo')
> +    >>> view = create_initialized_view(
> +    ...     project_milestone, '+index', principal=person)
> +    >>> print find_tag_by_id(view.render(), 'hide-side-portlets')['type']
> +    text/css
> +
> +A normal milestone does not have the CSS rule.
> +
> +    >>> view = create_initialized_view(
> +    ...     milestone, '+index', principal=person)
> +    >>> print find_tag_by_id(content, 'hide-side-portlets')
> +    None

The test is fine - I've got a more general question/discussion point
that I'd be keen to chat with you about some other time:

I remember you mentioning this way of testing the template as part of
the view test - and I can understand that it makes the stories much easier
to read by removing the unnecessary detail, but I'm not convinced that
the view test is the place for it. I've always assumed the dependencies
are:

model -> view -> template

That is, if I update a model, all tests could fail. If I update a view,
the model test cannot fail, but the view and template tests might. If I
update a template, the model and view tests should not fail, but the
template tests might.

I think what I'm trying to get at is, perhaps we should have some separate
template tests that do exactly what you've done here (or alternatively as
a unittest)? That way the stories stay clean, but the view tests would
remain focused on view logic?

> +
> +
>  Editing milestones
>  ------------------
>  
> 
> === modified file 'lib/lp/registry/stories/milestone/object-milestones.txt'
> --- lib/lp/registry/stories/milestone/object-milestones.txt	2009-09-14 15:03:45 +0000
> +++ lib/lp/registry/stories/milestone/object-milestones.txt	2009-09-17 07:26:10 +0000
> @@ -122,13 +122,13 @@
>  
>      >>> specs = find_tag_by_id(anon_browser.contents, 'milestone_specs')
>      >>> print extract_text(specs)
> -    Blueprint Product Priority Assignee Delivery
> +    Blueprint Project Priority Assignee Delivery
>      Title evolution specification   Evolution  High  Unknown
>      Title gnomebaker specification  gnomebaker High  Unknown
>  
>      >>> bugtasks = find_tag_by_id(anon_browser.contents, 'milestone_bugtasks')
>      >>> print extract_text(bugtasks)
> -    Bug report Product Importance Assignee Status ...
> +    Bug report Project Importance Assignee Status ...
>      Milestone test bug for evolution  Evolution  Undecided Confirmed ...
>      Milestone test bug for gnomebaker gnomebaker Undecided Confirmed ...
>      Milestone test bug for evolution series trunk Undecided Confirmed
> 
> === modified file 'lib/lp/registry/templates/milestone-index.pt'
> --- lib/lp/registry/templates/milestone-index.pt	2009-09-14 19:28:26 +0000
> +++ lib/lp/registry/templates/milestone-index.pt	2009-09-17 07:26:10 +0000
> @@ -6,6 +6,15 @@
>    metal:use-macro="view/macro:page/main_side"
>    i18n:domain="launchpad">
>  
> +  <tal:css metal:fill-slot="head_epilogue"
> +    condition="view/is_project_milestone">
> +    <style id="hide-side-portlets" type="text/css">
> +        .side {
> +            background: #fff;
> +            }
> +    </style>
> +  </tal:css>

The link above to the zope page about conditional macros *may* help - but
otherwise, this is fine.

> +
>    <body>
>      <tal:heading metal:fill-slot="heading">
>        <h1 tal:content="view/milestone/title" >project 1.x series</h1>
> @@ -183,11 +192,11 @@
>          </div>
>        </div>
>  
> -    <tal:release
> -      condition="view/release"
> -      replace="structure view/milestone/@@+productrelease-data" />
> +    <div class="wide"
> +      tal:condition="view/release"
> +      tal:content="structure view/milestone/@@+productrelease-data" />
>  
> -    <div class="portlet">
> +    <div class="portlet wide">
>          <h2>
>            <span id="specification-count" style="color: #3594bb;"
>              tal:content="structure view/specification_count_text">2</span>
> @@ -205,7 +214,7 @@
>                  <th colspan="2">Blueprint</th>
>                  <th style="width: 14em"
>                    tal:condition="view/is_project_milestone">
> -                  Product
> +                  Project
>                  </th>
>                  <th style="width: 9em;">Priority</th>
>                  <th style="width: 14em;">Assignee</th>
> @@ -261,7 +270,7 @@
>                  <th colspan="4">Bug report</th>
>                  <th style="width: 14em"
>                    tal:condition="view/is_project_milestone">
> -                  Product
> +                  Project
>                  </th>
>                  <th style="width: 9em;">Importance</th>
>                  <th style="width: 14em;">Assignee</th>
> @@ -280,20 +289,6 @@
>                       tal:content="bugtask/bug/title/fmt:shorten/65">
>                       Bug Title Here
>                    </a>
> -                  <tal:has-tags
> -                    condition="bugtask/bug/tags|nothing">
> -                    <br />
> -                    <span id="tag-list">
> -                      <a tal:repeat="tag bugtask/@@+index/official_tags"
> -                         tal:content="tag"
> -                         class="official-tag"
> -                         tal:attributes="href string:${context/target/fmt:url:bugs}/+bugs?field.tag=${tag}">tag</a>
> -                      <a tal:repeat="tag bugtask/@@+index/unofficial_tags"
> -                         tal:content="tag"
> -                         class="unofficial-tag"
> -                         tal:attributes="href string:${context/target/fmt:url:bugs}/+bugs?field.tag=${tag}">tag</a>
> -                    </span>
> -                  </tal:has-tags>
>                  </td>
>                  <td style="padding-right: 5px; text-align:right">
>                    <img src="/@@/info" tal:replace="structure bugtask/image:badges" />
> @@ -355,8 +350,7 @@
>        </div>
>      </div>
>  
> -    <tal:side metal:fill-slot="side"
> -      define="overview_menu context/menu:overview">
> +    <tal:side metal:fill-slot="side">
>        <tal:menu replace="structure view/milestone/@@+global-actions" />
>      </tal:side>
>    </body>

Great! Thanks Curtis.