Code review comment for lp:~julian-edwards/launchpad/related-software-30

On Sep 8, 2009, at 10:07 , Curtis Hovey wrote:

>>
>> I'm a bit bothered by the way the horizontal links hop around and are
>> inconsistently ordered as you page through them. If you used the
>> following code (moved to a macro) for all pages then the link display
>> would be fixed in the same order and the active page would have a
>> disabled link, which is the way +global-action menus work.
>>
>> <div class="top-portlet" id="navlinks">
>> <ul class="horizontal">
>> <li>
>> <a tal:replace="structure view/menu:navigation/summary/
>> render"/></li>
>> <li>
>> <a tal:replace="structure
>> view/menu:navigation/maintained/render"/></li>
>> <li>
>> <a tal:replace="structure view/menu:navigation/uploaded/
>> render"/></li>
>> <li>
>> <a tal:replace="structure view/menu:navigation/ppa/render"/
>> ></li>
>> <li>
>> <a tal:replace="structure view/menu:navigation/projects/
>> render"/></li>
>> </ul>
>> </div>
>
> I agree with Brad, but I think we need another change. This approach
> creates empty <li> elements. Each <li> needs condition="view/
> menu:navigation/uploaded/enabled"

The snipped I suggested does not produce empty <li>. Here is what it
produces for the +related-projects page:

   <div class="top-portlet" id="navlinks">
     <ul class="horizontal">
       <li>
         <a href="https://launchpad.dev/~mark/+related-software"
      class="menu-link-summary sprite info">Summary</a></li>
       <li>
         <a href="https://launchpad.dev/~mark/+maintained-packages"
      class="menu-link-maintained sprite info">Maintained Packages</
a></li>
       <li>
         <a href="https://launchpad.dev/~mark/+uploaded-packages"
      class="menu-link-uploaded sprite info">Uploaded Packages</a></li>
       <li>
         <a href="https://launchpad.dev/~mark/+ppa-packages"
      class="menu-link-ppa sprite info">PPA Packages</a></li>
       <li>
         <span class="menu-link-projects nolink
               sprite info">Related Projects</span></li>
     </ul>
   </div>

>
> Since this is already a NavMenu, you could just iterate over
> the .enabled_links. I recall that the list needs to be gotten from
> the view, but it may be possible to make the menu do the work.

Leaving out the enabled links will cause the horizontal menu to leave
out the page we're on. What I have renders it as text, not a link,
which is what we do for +global-actions. Leaving it out may be your
preference.