Code review comment for lp:~michael.nelson/launchpad/3-0-menu

Revision history for this message
Michael Nelson (michael.nelson) wrote :

OK, I've updated this branch after talking with both Curtis and Martin.

The complete diff is: Diff: http://pastebin.ubuntu.com/247929/ (easier to read).

Changes
=======

1. I added a location_title formatter which, similar to the logo formatter, checks for the nearest IHasLogo implementer in the canonical url, and displays returns the title. This allows the location portlet to display the *person* who owns the PPA currently being browsed, like this:

http://people.canonical.com/~michaeln/menu_3-0/5-default-h2-location-heading.png

Note: when discussing this with Curtis, we had planned for this to be 'pillar_title', but I also understood that when viewing a PPA, the location should be the person, as shown in the image. Other options for the formatter would be 'piller_or_person_title' depending on what Curtis thinks.

2. I've ensured that if we cannot display a piller/person title (because there is no pillar or person present) then we display 'Launchpad' like this:

http://people.canonical.com/~michaeln/menu_3-0/6-no-location-default-launchpad.png

Note1: I assume 'Launchpad Home' will be disappearing, according to beuno' design?
Note2: The logo formatter already does this by default - so that didn't require any change.

Questions
=========

Now I'm left with the following question:

1. Why is the default heading used in the "heading" metal slot h2? Shouldn't it always be h1 (given that the heading in the location is always h2).

Note: I did do a revision where the location_title marked up the title with h1 or h2 depending on whether the current context itself implemented IHasLogo, but that didn't work for pages like:

http://people.canonical.com/~michaeln/menu_3-0/7-ubuntu-search.png

where the context *is* the ubuntu distribution, but we have an h1 heading related to search.

Thanks for your time!

« Back to merge proposal