review approve code ui
 status approve

On Aug 21, 2009, at 04:05 PM, Julian Edwards wrote:

>https://launchpad.dev/ubuntu/hoary/i386/

Hi Julian,

As mentioned on IRC, I have a few ui comments.  My UI review must be seconded
by a ui mentor.  Here are the things I noticed:

 * Wrapping the "1" in "This archive contains 1 software packages" in a
   <bold class="registry-stats"> for consistency
 * Removing the info icon and "Show" prefix from the navmenu.  However,
   because it is too expensive at this late date to retrofit all the already
   converted pages, we've decided to leave this the way it is.
 * The +builds page batch navigation shouldn't show the navbar when there is
   only one page of results, but this is out-of-scope for this branch
 * The placement of the "This archive contains" text to above the search box
   instead of below it, in normal narrative text

Other than these small issues, the ui looks great.  I have only minor comments
about the code, so r=me, ui=me.  merge-conditional

=== modified file 'lib/lp/soyuz/browser/configure.zcml'
--- lib/lp/soyuz/browser/configure.zcml	2009-08-19 02:58:06 +0000
+++ lib/lp/soyuz/browser/configure.zcml	2009-08-21 15:29:49 +0000
> @@ -591,7 +591,7 @@
>          <browser:menus
>              module="lp.soyuz.browser.distroarchseries"
>              classes="
> -                DistroArchSeriesContextMenu"/>
> +                DistroArchSeriesActionMenu"/>

When I was making similar changes, I ended up putting the "/> on a separate
line underneath the first letter of the menu name.  It made it much easier to
sort the lines alphabetically later.  You only have one class in this case, so
I'll leave it up to you, but I just wanted to share that :).

>          <browser:url
>              for="lp.soyuz.interfaces.distroarchseries.IDistroArchSeries"
>              path_expression="architecturetag"

=== modified file 'lib/lp/soyuz/browser/distroarchseries.py'
--- lib/lp/soyuz/browser/distroarchseries.py	2009-08-18 18:17:25 +0000
+++ lib/lp/soyuz/browser/distroarchseries.py	2009-08-21 15:29:49 +0000
> @@ -4,14 +4,17 @@
>  __metaclass__ = type
>  
>  __all__ = [
> +    'DistroArchSeriesActionMenu',
>      'DistroArchSeriesAddView',
>      'DistroArchSeriesAdminView',
>      'DistroArchSeriesPackageSearchView',
> -    'DistroArchSeriesContextMenu',
>      'DistroArchSeriesNavigation',
>      'DistroArchSeriesView',
>      ]
>  
> +from zope.component import provideAdapter
> +from zope.interface import implements, Interface
> +
>  from canonical.launchpad import _
>  from lp.soyuz.browser.build import BuildRecordsView
>  from canonical.launchpad.browser.packagesearch import PackageSearchViewBase
> @@ -20,8 +23,9 @@
>      GetitemNavigation, LaunchpadEditFormView)
>  from canonical.launchpad.webapp.launchpadform import (
>      action, LaunchpadFormView)
> +from canonical.launchpad.webapp.interfaces import INavigationMenu
>  from canonical.launchpad.webapp.menu import (
> -    ContextMenu, enabled_with_permission, Link)
> +    ContextMenu, enabled_with_permission, Link, NavigationMenu)
>  from canonical.launchpad.webapp.publisher import canonical_url
>  from canonical.lazr.utils import smartquote
>  
> @@ -31,9 +35,15 @@
>      usedfor = IDistroArchSeries
>  
>  
> -class DistroArchSeriesContextMenu(ContextMenu):
> -
> -    usedfor = IDistroArchSeries
> +class IDistroArchSeriesActionMenu(Interface):
> +    """Marker interface for the action menu."""
> +
> +
> +class DistroArchSeriesActionMenu(NavigationMenu):
> +    """Action menu for distro arch series."""
> +    usedfor = IDistroArchSeriesActionMenu
> +    facet = "overview"
> +    title = "Actions"

I don't think the title actually shows up anywhere currently.  Maybe remove
this so it doesn't magically show up later?  (I think it would do little more
than add clutter if it did show up.)

>      links = ['admin', 'builds']
>  
>      @enabled_with_permission('launchpad.Admin')

=== modified file 'lib/lp/soyuz/templates/distroarchseries-index.pt'
--- lib/lp/soyuz/templates/distroarchseries-index.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/soyuz/templates/distroarchseries-index.pt	2009-08-21 15:29:49 +0000
> @@ -25,26 +19,42 @@
>        <tal:series replace="distroseries/displayname">3.1</tal:series> for
>        <tal:arch replace="context/architecturetag">i386</tal:arch>
>    </h1>
> -
> -  <h2>Search binary packages</h2>
> -
> -  <form name="search" action="+search" method="GET">
> -
> -    <input type="text"
> -      name="text"
> -      size="35"
> -      tal:attributes="value request/text|nothing" />
> -
> -    <input type="submit" value="Search Packages"/>
> -
> -  </form>
> -
> -  <p class="discreet">
> -    This archive contains
> -    <tal:count replace="context/package_count">bazillions of</tal:count> software packages.
> -  </p>
> -
> -</div>
> +</div>
> +
> +<div metal:fill-slot="side">
> +  <tal:menu replace="structure view/@@+global-actions" />
> +</div>
> +
> +<div metal:fill-slot="main">
> +
> +    <div class="top-portlet">
> +      <h2>Search binary packages</h2>

I'm not sure this h2 is needed.  When you move the "This archive..." text to
above the search box, I think this text will be clutter.  I'd rather the
search button say "Search Binary Packages", and then this h2 is redundant and
can be removed.

> +
> +      <form name="search" method="GET">
> +
> +        <input type="text"
> +          name="text"
> +          size="35"
> +          tal:attributes="value request/text|nothing" />
> +
> +        <input type="submit" value="Search Packages"/>
> +
> +      </form>
> +
> +      <p class="discreet">
> +        This archive contains
> +        <tal:count replace="context/package_count">bazillions of</tal:count> software packages.
> +      </p>
> +    </div><!--portlet-->
> +
> +    <metal:package-search use-macro="context/@@+macros/search-results" />
> +
> +    <tal:details condition="not: view/search_requested"
> +                 replace="structure context/@@+portlet-details" />
> +
> +
> +
> +</div><!--metal-->
>  
>  </body>
>  </html>