On Tue, 2009-08-11 at 20:37 +0000, Curtis Hovey wrote:
> Review: Needs Fixing code
> Hi Guilherme.
> 
> This is a very promising start. I have several suggestions to improve
> this branch based on code that final revision of the project page. I
> stole some code to express my ideas. I am certain none of it works.
> 
> 
> > === modified file 'lib/lp/code/templates/project-branches.pt'
> > --- lib/lp/code/templates/project-branches.pt	2009-07-17 17:59:07 +0000
> > +++ lib/lp/code/templates/project-branches.pt	2009-08-10 19:04:22 +0000
> > @@ -6,24 +6,22 @@
> >    xml:lang="en"
> >    lang="en"
> >    dir="ltr"
> > -  metal:use-macro="context/@@main_template/master"
> > +  metal:use-macro="view/macro:page/onecolumn"
> 
> onecolumn is 2.0. use the replacement macro
>   metal:use-macro="view/macro:page/main_only"

Right, this is because I didn't mean to upgrade this to 3.0 -- I just
wanted to make it look like product-branches.pt so that I could float
the 'subscribe to branches' link to the top right. For projects, this
link is currently shown in the action menu of the +index page and I had
to move it somewhere else.

> 
> ...
> 
> > +<div metal:fill-slot="main" tal:define="branches view/branches">
> > +
> > +  <div style="float:right" id="floating-links"
> > +       tal:define="menu context/menu:overview">
> > +    <div tal:define="link menu/branch_visibility"
> > +         tal:condition="link/enabled"
> > +         tal:content="structure link/render" />
> > +  </div>
> 
> This is 2.0 style. We do not want to float admin links the to side. They
> may be in the content or in the side portlet as an action menu. Since this
> page does not have side portlet, I think we can work this into the content
> above the branch listing...
> 
> >    <h1>Bazaar branches for <tal:project-name replace="context/displayname"/></h1>
> 
> ...Something like this before the listing.
> 
>   <p>
>     Branches are <tal:visibility replace="magic command to get value" />
>     by default.
>     <a tal:replace="structure context/menu:overview/branch_visibility/fmt:link" />
>   </p>

I could do that, but I think it's important that product-branches.pt is
updated at the same time, to make sure they're consistent. I'd also have
to get this past UI review, which might delay this branch -- that's the
reason why I didn't try converting it to 3.0

>   
> ...
>  
> >    <tal:branchlisting content="structure branches/@@+branch-listing" />
>  
> ...
> 
> > === modified file 'lib/lp/registry/browser/project.py'
> > --- lib/lp/registry/browser/project.py	2009-08-05 01:49:41 +0000
> > +++ lib/lp/registry/browser/project.py	2009-08-10 19:53:47 +0000
> 
> ...
> 
> @@ -220,19 +215,39 @@
> 
> ...
> 
> > +class ProjectActionMenu(NavigationMenu):
> > +
> > +    usedfor = IProjectActionMenu
> > +    facet = 'overview'
> > +    title = 'Action menu'
> > +    links = ('subscribe', 'edit', 'administer')
> > +
> > +    # XXX: salgado, 2009-08-10: This should be shown in the +index page of the
> > +    # project's bugs facet, but that would require too much work and I just
> > +    # want to convert this page to 3.0, so I'll leave it here for now.
> >      def subscribe(self):
> >          text = 'Subscribe to bug mail'
> >          return Link('+subscribe', text, icon='edit')
> 
> Do we need a bug number for this XXX. I am tempted to say we want to do to

Yes, I think we do.
https://bugs.edge.launchpad.net/malone/+bug/412178

> define two clean up tasks for links and menus:
> 
>     1. Define all links in mixins so that we are certain there is a single
>        definition. Menus *must* use the mixin instead of define their own
>        links.

Even if that means having a menu which just inherits from a mixin and
don't define any other links?  In the case of project group, I think
that'd be the case for most of them.

>     2. Make menu mixins for interfaces such as IPillar and
>        IHasStructuralSubscriptions so that we share common links.

Do they have enough common links to justify doing this? I know there are
plenty of pages that have the same purpose, but some (most?) of them
have different names/URLs, don't they?

> 
> ...
> 
> > === modified file 'lib/lp/registry/templates/project-details.pt'
> > --- lib/lp/registry/templates/project-details.pt	2009-07-17 17:59:07 +0000
> > +++ lib/lp/registry/templates/project-details.pt	2009-08-10 19:04:22 +0000
> 
> ...
> 
> > +<div id="portlet-details">
> > +  <table class="summary" tal:define="overview_menu context/menu:overview">
> > +    <tbody>
> > +      <tr tal:condition="context/required:launchpad.Admin">
> > +        <th>Status:</th>
> > +        <td>
> > +          <tal:block condition="context/active">Active</tal:block>
> > +          <tal:block condition="not: context/active">Disabled</tal:block>
> > +          <tal:block condition="context/reviewed"><br />Reviewed</tal:block>
> > +          <tal:block condition="context/icon"><br />Branded
> > +            <tal:icon replace="structure context/image:icon" />
> > +          </tal:block>
> > +        </td>
> > +      </tr>
> 
> ...
> 
> 
> This should become a set of definition lists so that it uses the 3.0
> layout and style correctly.
> 
> /me steals example and hacks a possible solution.
> 
>   <div id="project-info" class="portlet">
>     <h2>Project information</h2>
> 
>     <div class="two-column-list">
>       <dl id="owner" tal:condition="context/owner">
>         <dt>Maintainer:</dt>
>         <dd><a tal:replace="structure context/owner/fmt:link" /></dd>
>       </dl>
> 
>       <dl id="driver">
>         <dt>Driver:</dt>
>         <dd tal:condition="view/effective_driver">
>           <a tal:replace="structure view/effective_driver/fmt:link" /></dd>
>         <dd tal:condition="not: view/effective_driver">
>           Not yet appointed.</dd>
>       </dl>
> 
>       <dl id="bug-tracker">
>         <tal:uses_launchpad condition="uses_launchpad">
>           <dt>Bug tracker:</dt>
>           <dd>
>           <a
>             tal:condition="context/bugtracker"
>             tal:content="context/bugtracker/title"
>             tal:attributes="href context/bugtracker/fmt:url">tracker title</a>
>           <tal:none condition="not:context/bugtracker">None specified</tal:none>
>           </dd>
>         </tal:uses_launchpad>
>       </dl>
>     </div>

It looks much nicer now.

> 
>     <!--
>       projects has a  review white board here project groups may want
>       statuses.
>     -->

Do you mean to have the status below the others instead of above them as
it currently is?

> 
>     <!-- Hack because the page header is missing the registrant. -->
>     <p id="registrant">
>       Registered
>       <tal:created replace="context/datecreated/fmt:approximatedate" />
>       by
>       <tal:registrant replace="structure context/registrant/fmt:link" />
>     </p>
>   </div>
> 
> 
> > === modified file 'lib/lp/registry/templates/project-index.pt'
> > --- lib/lp/registry/templates/project-index.pt	2009-07-17 17:59:07 +0000
> > +++ lib/lp/registry/templates/project-index.pt	2009-08-11 14:26:36 +0000
> 
> ...
> 
> > +    <tal:heading metal:fill-slot="heading">
> > +      <h1 tal:content="context/title">Mozilla</h1>
> > +    </tal:heading>
> > +
> > +    <tal:main metal:fill-slot="main">
> > +
> > +    <div class="top-portlet">
> > +      <p id="project-inactive" tal:condition="not: context/active" class="warning message"> 
> > +        This project is currently inactive <a href="+review">(change this)</a>
> >        </p>
> 
> The link should come from the menu: maybe
> 
>     <a tal:replace="structure overview_menu/review/fmt:icon"/>

To keep the text of the link I need to do it slightly different, but it
achieves the point -- that the link's URL comes from a menu.

        This project is currently inactive
        <a tal:attributes="href context/menu:overview/administer/url"
          >(change this)</a>

> 
> ...
> 
> > +      <tal:block condition="view/required:launchpad.Edit">
> > +        <p tal:condition="not: context/products" class="warning message"> 
> 
> I see trailing whitespace.

Remove them all around.

> 
> > +          There are no projects registered for
> > +          <span tal:replace="context/displayname">project displayname</span>.
> > +          <br />
> > +          You need to <a href="+newproduct">register another project that is
> > +          part of <tal:project replace="context/displayname" /></a> or associate
> > +          an existing project with it.
> > +        </p>
> > +      </tal:block>
> 
> This information should be moved into the project portlet below so that
> we show it once. I think the new +newproduct link should always be at the
> bottom of projects portlet.

The link is always shown there, as well as the notice, although it's not
a warning like it is here.  I don't quite like having this as a warning
but on the other hand, having a project group with no projects is
meaningless, so we must try and make sure users see something is wrong.

I'm not sure what to do in this case.

> 
>        <div class="description" tal:content="context/summary">
>          This is the project group Summary, which should be a single paragraph
> 
> Use class="summary" to present the summary as a summary.

Wow!  Now I can even distinguish the summary from the description in the
page. :)

> ...
> 
> > +      <div class="discreet" style="text-align: right"
> > +            tal:content="structure context/menu:overview/rdf/fmt:link-icon" />
> 
> The rdf goes into the side portlets like the project. This is awkward
> since there are no downloads to pair with it.

Right, that's why I left it right below the project's description. I'd
prefer it as close as possible to the project's info, but I could move
it to the side portlet if you want.

> 
> > +      <div class="yui-g">
> > +        <div class="yui-u first">
> > +          <div class="portlet">
> > +            <h2>Project group information</h2>
> > +            <tal:details replace="structure context/@@+details" />
> 
> Are the details used else where? if not, I think the
> 
>   <div class="portlet">
>     <h2>Project group information</h2>
> 
> should be moved into the portlet so that it is self-contained.

It's not, so this is done.

> 
> ...
> 
> > +        <div class="yui-u" id="products">
> > +          <div class="portlet">
> > +            <h2>Projects</h2>
> > +            <p tal:condition="not: context/products"> 
> 
> I see trailing whitespace.

Removed too

> 
> > +              There are no projects registered for
> > +              <span tal:replace="context/displayname">project displayname</span>.
> > +            </p>
> > +            <div
> > +              tal:condition="context/products"
> > +              tal:repeat="product context/products">
> > +              <tal:link replace="structure product/fmt:link" />
> > +              <div tal:condition="product/releases/count">
> > +                Releases:
> > +                <tal:release repeat="release product/releases">
> > +                  <a
> > +                    tal:attributes="href release/fmt:url"
> > +                    tal:content="release/version"
> > +                  >version</a><tal:comma
> > +                  condition="not:repeat/release/end">,</tal:comma>
> > +                </tal:release>
> > +              </div>
> > +              <div tal:condition="product/milestones">
> > +                Milestones:
> > +                <tal:milestone repeat="milestone product/milestones">
> > +                  <a
> > +                    tal:attributes="href milestone/fmt:url"
> > +                    tal:content="milestone/name"
> > +                  >name</a><tal:comma
> > +                  condition="not:repeat/milestone/end">,</tal:comma>
> > +                </tal:milestone>
> > +              </div>
> > +            </div>
> 
> This looks familiar. I know I hacked in this recently. This is fine to
> land but I think we need to rethink this. The release is now presented
> as a state of a milestone, so we want a single list that  shows releases
> differently than milestones.
> 
> Well, why is the list of releases and milestones here? We do not list this
> much information in the project page. We are only show the current
> release and the current series. This should be the same. Maybe

This would make a lot more sense, I think.

> 
>  <dl>
>    <dt><tal:link replace="structure product/fmt:link" /></td>
>    <dd>
>      <a tal:replace="structure product/development_focus/fmt:link" /> series
>      <strong>_release_</strong> ,<strong>_release_</strong>,
>      milestone, milestone
>    </dd>
>  

This is what I ended up with:

          <dl tal:condition="context/products"
              tal:repeat="product context/products">
            <dt><a tal:replace="structure product/fmt:link">product</a></dt>
            <dd tal:define="dev_focus product/development_focus">
              <a tal:attributes="href dev_focus/fmt:url"
                 tal:content="dev_focus/name">trunk</a>
              series:
              <tal:releases repeat="release dev_focus/releases">
                <strong>
                  <a tal:attributes="href release/fmt:url"
                     tal:content="release/version">release
                </a></strong>,
              </tal:releases>
              <tal:releases repeat="milestone dev_focus/milestones">
                <strong>
                  <a tal:attributes="href milestone/fmt:url"
                     tal:content="milestone/name">milestone
                  </a></strong><tal:comma
                    condition="not:repeat/milestone/end">,</tal:comma>
              </tal:releases>
            </dd>
          </dl>

Is this what you had in mind?

> 
> 
> ...
> 
> > +            <tal:block condition="context/required:launchpad.Edit">
> > +              <a href="+newproduct">&raquo; Register another project that is
> > +              part of <tal:project replace="context/displayname" /></a>
> > +            </tal:block>
> 
> This should be a standard link presented as a list in case we get more than
> one. Something like this.
> 
>             <ul class="horizontal"
>               tal:condition="overview_menu/project_add/enabled">
>               <li>
>                 <a tal:replace="structure overview_menu/project_add/fmt:link" />
>               </li>
>             </ul>
> 

Martin suggested this and floating the link to the right, so I added a
float:right to the <ul>.

> ...
> 
> > +    <tal:side metal:fill-slot="side">
> > +      <div id="object-actions" class="top-portlet">
> > +        <tal:menu replace="structure view/@@+global-actions" /> 
> 
> I see trailing whitespace.

Removed

> 
> ...
> 
> > +      <div id="involvement" class="portlet involvement"
> > +           tal:condition="context/products">
> > +        <h2>Get Involved</h2>
> > +        <ul>
> > +          <li>
> > +            <a class="bugs"
> > +              tal:attributes="href context/menu:bugs/new/url">Report a Bug</a>
> > +          </li>
> > +          <li>
> > +            <a class="question"
> > +              tal:attributes="href context/menu:answers/new/url">Ask a question</a>
> > +          </li>
> > +          <li>
> > +            <a class="translate"
> > +              tal:attributes="href context/menu:translations/overview/url">Help translate</a>
> > +          </li>
> > +        </ul>
> > +      </div>
> 
> Use this, though you may need to fix the view in browser/pillar.py
>       <div tal:replace="structure context/@@+get-involved" />
> 

IProject doesn't provide any of the official_* attributes used in the
view/template and we also don't want 'Submit code' and 'Register a
Blueprint' links, so I'd have to special case everything in the
view/template to make it possible to use it here.  I'd rather not do
that.


-- 
Guilherme Salgado <email address hidden>