Launchpad itself

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

related-software-30
Merge into devel

Revision history for this message

Brad Crittenden (bac) wrote on 2009-09-08:

Hi Julian,

Thanks for the nice fixes on this branch.

I have one big suggestion for the way the links are put on the page.

Other than that the branch looks good.

> === modified file 'lib/canonical/launchpad/pagetitles.py'
--- lib/canonical/launchpad/pagetitles.py 2009-09-05 07:03:47 +0000
> +++ lib/canonical/launchpad/pagetitles.py 2009-09-07 20:32:50 +0000
> @@ -747,8 +747,6 @@
>
> person_karma = ContextDisplayName(smartquote("%s's karma in Launchpad"))
>
> -person_maintained_packages = ContextDisplayName('Software maintained by %s')
> -
> person_mentoringoffers = ContextTitle('Mentoring offered by %s')
>
> def person_mergeproposals(context, view):
> @@ -765,12 +763,6 @@
>
> person_participation = ContextTitle("Team participation by %s")
>
> -person_ppa_packages = ContextDisplayName('PPA packages related to %s')
> -
> -person_related_projects = ContextDisplayName('Projects related to %s')
> -
> -person_related_software = ContextDisplayName('Software related to %s')
> -
> person_review = ContextDisplayName("Review %s")
>
> person_specfeedback = ContextDisplayName('Feature feedback requests for %s')
> @@ -786,8 +778,6 @@
>
> person_teamhierarchy = ContextDisplayName('Team hierarchy for %s')
>
> -person_uploaded_packages = ContextDisplayName('Software uploaded by %s')
> -
> pofile_filter = FilteredTranslationsTitle(
> smartquote('Translations by %(person)s in "%(title)s"'))
>

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2009-09-05 03:33:24 +0000
> +++ lib/lp/registry/browser/person.py 2009-09-07 20:32:50 +0000
> @@ -4834,6 +4834,10 @@
>
> max_results_to_display = config.launchpad.default_batch_size
>
> + @property
> + def page_title(self):
> + return "Software related to " + self.context.title

Minor since they are synonyms but I'd use displayname here as it is
more familiar for a person. Will apply to all.

> @cachedproperty
> def related_projects(self):
> """Return a list of project dicts owned or driven by this person.
> === modified file 'lib/lp/registry/stories/foaf/xx-person-packages.txt'
> --- lib/lp/registry/stories/foaf/xx-person-packages.txt 2009-08-13 15:12:16 +0000
> +++ lib/lp/registry/stories/foaf/xx-person-packages.txt 2009-09-08 10:49:20 +0000
> @@ -5,18 +5,10 @@
>
> >>> anon_browser.open('http://launchpad.dev/~mark')
> >>> anon_browser.getLink('Related Software').click()
> - >>> print_navigation_links(anon_browser.contents)
> - Profile: ...
> - Related Software
> - Karma: ...
>
> >>> print anon_browser.title
> Software related to Mark Shuttleworth
> >>> print anon_browser.url
> http://launchpad.dev/~mark/+related-software
>
> -That page displays a table, the with list of 'Maintained'packages as
> -well as the list of 'Uploaded' packages. The table also shows the
> -number of open bugs and questions filed against each package.

Spotting typos in deleted code probably isn't helpful, is it? :)

> Please see pagetests/soyuz/xx-person-packages.txt for details.

> === modified file 'lib/lp/registry/templates/person-related-projects.pt'
> --- lib/lp/registry/templates/person-related-projects.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/person-related-projects.pt 2009-09-08 11:49:02 +0000
> @@ -3,20 +3,33 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
> xmlns:metal="http://xml.zope.org/namespaces/metal"
> xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> - xml:lang="en"
> - lang="en"
> - dir="ltr"
> - metal:use-macro="view/macro:page/onecolumn"
> + metal:use-macro="view/macro:page/main_only"
> i18n:domain="launchpad"
> >
>
> <body>
>
> +<div metal:fill-slot="heading">
> + <h1 tal:content="view/page_title"/>
> +</div>
> +
> <div metal:fill-slot="main">
> -
> - <div id="projects">
> + <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>
> + </ul>
> + </div>

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 id="projects" class="top-portlet">
> <a name="projects" />
> - <h1>Related projects</h1>
>
> <tal:navigation_top
> replace="structure view/batchnav/@@+navigation-links-upper" />

Hi Julian,

Thanks for the nice fixes on this branch.

I have one big suggestion for the way the links are put on the page.

Other than that the branch looks good.

> === modified file 'lib/canonical/launchpad/pagetitles.py'
--- lib/canonical/launchpad/pagetitles.py	2009-09-05 07:03:47 +0000
> +++ lib/canonical/launchpad/pagetitles.py	2009-09-07 20:32:50 +0000
> @@ -747,8 +747,6 @@
>  
>  person_karma = ContextDisplayName(smartquote("%s's karma in Launchpad"))
>  
> -person_maintained_packages = ContextDisplayName('Software maintained by %s')
> -
>  person_mentoringoffers = ContextTitle('Mentoring offered by %s')
>  
>  def person_mergeproposals(context, view):
> @@ -765,12 +763,6 @@
>  
>  person_participation = ContextTitle("Team participation by %s")
>  
> -person_ppa_packages = ContextDisplayName('PPA packages related to %s')
> -
> -person_related_projects = ContextDisplayName('Projects related to %s')
> -
> -person_related_software = ContextDisplayName('Software related to %s')
> -
>  person_review = ContextDisplayName("Review %s")
>  
>  person_specfeedback = ContextDisplayName('Feature feedback requests for %s')
> @@ -786,8 +778,6 @@
>  
>  person_teamhierarchy = ContextDisplayName('Team hierarchy for %s')
>  
> -person_uploaded_packages = ContextDisplayName('Software uploaded by %s')
> -
>  pofile_filter = FilteredTranslationsTitle(
>      smartquote('Translations by %(person)s in "%(title)s"'))
>

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py	2009-09-05 03:33:24 +0000
> +++ lib/lp/registry/browser/person.py	2009-09-07 20:32:50 +0000
> @@ -4834,6 +4834,10 @@
>  
>      max_results_to_display = config.launchpad.default_batch_size
>  
> +    @property
> +    def page_title(self):
> +        return "Software related to " + self.context.title

Minor since they are synonyms but I'd use displayname here as it is
more familiar for a person.  Will apply to all.

>      @cachedproperty
>      def related_projects(self):
>          """Return a list of project dicts owned or driven by this person.
> === modified file 'lib/lp/registry/stories/foaf/xx-person-packages.txt'
> --- lib/lp/registry/stories/foaf/xx-person-packages.txt	2009-08-13 15:12:16 +0000
> +++ lib/lp/registry/stories/foaf/xx-person-packages.txt	2009-09-08 10:49:20 +0000
> @@ -5,18 +5,10 @@
>  
>      >>> anon_browser.open('http://launchpad.dev/~mark')
>      >>> anon_browser.getLink('Related Software').click()
> -    >>> print_navigation_links(anon_browser.contents)
> -    Profile: ...
> -    Related Software
> -    Karma: ...
>  
>      >>> print anon_browser.title
>      Software related to Mark Shuttleworth
>      >>> print anon_browser.url
>      http://launchpad.dev/~mark/+related-software
>  
> -That page displays a table, the with list of 'Maintained'packages as
> -well as the list of 'Uploaded' packages. The table also shows the
> -number of open bugs and questions filed against each package.

Spotting typos in deleted code probably isn't helpful, is it?  :)

>  Please see pagetests/soyuz/xx-person-packages.txt for details.

> === modified file 'lib/lp/registry/templates/person-related-projects.pt'
> --- lib/lp/registry/templates/person-related-projects.pt	2009-07-17 17:59:07 +0000
> +++ lib/lp/registry/templates/person-related-projects.pt	2009-09-08 11:49:02 +0000
> @@ -3,20 +3,33 @@
>    xmlns:tal="http://xml.zope.org/namespaces/tal"
>    xmlns:metal="http://xml.zope.org/namespaces/metal"
>    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> -  xml:lang="en"
> -  lang="en"
> -  dir="ltr"
> -  metal:use-macro="view/macro:page/onecolumn"
> +  metal:use-macro="view/macro:page/main_only"
>    i18n:domain="launchpad"
>  >
>  
>  <body>
>  
> +<div metal:fill-slot="heading">
> +  <h1 tal:content="view/page_title"/>
> +</div>
> +
>  <div metal:fill-slot="main">
> -
> -  <div id="projects">
> +  <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>
> +    </ul>
> +  </div>

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 id="projects" class="top-portlet">
>      <a name="projects" />
> -    <h1>Related projects</h1>
>  
>      <tal:navigation_top
>           replace="structure view/batchnav/@@+navigation-links-upper" />

review: Approve (code)

« Back to merge proposal