Wow - a huge amount of work has gone into this Danilo!

I've mainly got a few questions below, but there was one thing that needs fixing - a CSS rule that I think you meant to apply just to your table, but applies globally to all th's.

Phew! I'm glad you managed to split them up now - I don't think I could sensibly do another review today :)

Cheers,
-Michael


> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> --- lib/canonical/launchpad/icing/style-3-0.css	2009-09-05 03:17:24 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css	2009-09-09 18:52:48 +0000
> @@ -748,6 +748,38 @@
>    padding: 2px 1em 2px 2px;
>  }
>  
> +/* Translations statistics and legend.
> + *
> + * Examples:
> + *   https://translations.launchpad.dev/ubuntu/hoary/+lang/es
> + *   https://translations.launchpad.dev/evolution/trunk/+lang/es
> + */

Great to see examples in there as recommended the other week!

> +
> +div.translations-legend {
> +  padding-top: 2em;
> +  padding-bottom: 1em;
> +}

Note: although we didn't get to it in the reviewers meeting on 09/09/09,
I saw that barry had an agenda item:

* 4-space indents for CSS styles [barry]

Also, if that table can get large, and you want to do easy
alternate-row-shading, I just found the other day that you can do things like:

tbody.translation-stats tr:nth-child(odd) {...}

which works in FF3.5, webkit/chromium etc., without having to add markup.
See http://dev.opera.com/articles/view/zebra-striping-tables-with-css3/
if you're interested.

This CSS could possibly be much simpler if you put
the 'translation-stats' class on the table element (instead of on the
tbody). Then you could remove the overall-translation-stats class and do:

> +tbody.translation-stats td {

table.translation-stats td {...

(that way you don't need a separate rule for the tfoot below.)

> +  text-align:center;
> +}
> +tbody.translation-stats td.template-name {

table.translation-stats td.template-name {...

(no difference there).

> +  text-align:left;
> +}
> +tfoot tr.overall-translation-stats td, th {

Yikes - careful there. This actually reads currently as:

"Apply the following styles to any td's inside a tr...etc., or *any* th."

So, for example, you can see this is being applied to th's at:

https://launchpad.dev/builders

Anyway, if you updated the class as suggested above, you could write it
as:
table.translation-stats tfoot td, table.translation-stats tfoot th {...

> +  background-color: #f7f7f7;
> +  border: 0px;
> +  border-top: 2px solid #d2d2d2;
> +  border-bottom: 2px solid #d2d2d2;
> +  padding-top: 5px;
> +  padding-bottom: 5px;
> +  font-weight: bold;
> +}
> +tfoot tr.overall-translation-stats th {

table.translation-stats tfoot th { ...

> +  text-align:left;
> +}
> +tfoot tr.overall-translation-stats td {

table.translation-stats tfoot td { ...

> +  text-align:center;
> +}
>  table.narrow-listing {
>    width: 45em;
>  }
> 

[snip]
> === modified file 'lib/lp/translations/browser/distroserieslanguage.py'
> --- lib/lp/translations/browser/distroserieslanguage.py	2009-07-17 00:26:05 +0000
> +++ lib/lp/translations/browser/distroserieslanguage.py	2009-09-10 09:45:00 +0000
> @@ -5,17 +5,26 @@
>  
>  __metaclass__ = type
>  
> -__all__ = ['DistroSeriesLanguageView']
> +__all__ = [
> +    'DistroSeriesLanguageNavigation',
> +    'DistroSeriesLanguageView',
> +    ]
>  
>  from canonical.launchpad.webapp import LaunchpadView
>  from canonical.launchpad.webapp.batching import BatchNavigator
> -
> +from canonical.launchpad.webapp.publisher import Navigation
> +from lp.translations.interfaces.distroserieslanguage import (
> +    IDistroSeriesLanguage)
>  
>  class DistroSeriesLanguageView(LaunchpadView):
>      """View class to render translation status for an `IDistroSeries`."""
>  
>      pofiles = None
>  
> +    @property
> +    def page_title(self):
> +        return self.context.title
> +

Just note that you will probably need to remove this soon - when barry's
branch lands that calculates the <title> automatically. It should only
be overridden here in special cases (yes, this changed... see the points
under "Here are the specific coding recommendations for your templates"
at: https://dev.launchpad.net/VersionThreeDotO/UI/Conversion#Heading%20rules

>      def initialize(self):
>          self.form = self.request.form
>  
> @@ -25,3 +34,23 @@
>  
>          self.pofiles = self.context.getPOFilesFor(
>              self.batchnav.currentBatch())
> +        self.parent = self.context.distroseries.distribution
> +
> +    @property
> +    def translation_group(self):
> +        return self.context.distroseries.distribution.translationgroup
> +
> +    @property
> +    def translation_team(self):
> +        """Is there a translation team for this translation."""
> +        if self.translation_group is not None:
> +            team = self.translation_group.query_translator(
> +                self.context.language)
> +        else:
> +            team = None
> +        return team

Both of the above methods are currently used in your template,
but there's no view test that I can see for them. I'll leave it up to
you - easy to add.

> +
> +
> +class DistroSeriesLanguageNavigation(Navigation):
> +    """Navigation for `IDistroSeriesLanguage`."""
> +    usedfor = IDistroSeriesLanguage
> 

> === modified file 'lib/lp/translations/browser/tests/test_breadcrumbs.py'
> --- lib/lp/translations/browser/tests/test_breadcrumbs.py	2009-09-10 09:58:30 +0000
> +++ lib/lp/translations/browser/tests/test_breadcrumbs.py	2009-09-10 10:27:20 +0000
> @@ -98,6 +98,46 @@
>              ["Crumb Tester", "Translations"])
>  
>  
> +class TestTranslationGroupsBreadcrumbs(BaseTranslationsBreadcrumbTestCase):
> +
> +    def test_translationgroupset(self):
> +        group_set = getUtility(ITranslationGroupSet)
> +        url = canonical_url(group_set, rootsite='translations')
> +        # Translation group listing is top-level, so no breadcrumbs show up.
> +        self._testContextBreadcrumbs(
> +            [], [], [],

Shouldn't this be:
               [group_set], [], [],
?
But this then fails, with the breadcrumb urls being:

[u'http://translations.launchpad.dev/+groups', 'http://translations.launchpad.dev/+groups']

even though there are correctly no breadcrumbs displayed at:
https://translations.launchpad.dev/+groups

Maybe I missed something, but I'm confused by that test as it is.

> +            url=url)
> +
> +    def test_translationgroup(self):
> +        group_set = getUtility(ITranslationGroupSet)
> +        group = self.factory.makeTranslationGroup(
> +            name='test-translators', title='Test translators')
> +        self._testContextBreadcrumbs(
> +            [group_set, group],
> +            ["http://translations.launchpad.dev/+groups",
> +             "http://translations.launchpad.dev/+groups/test-translators"],
> +            ["Translation groups", "Test translators"])
> +
> +
> +class TestSeriesLanguageBreadcrumbs(BaseTranslationsBreadcrumbTestCase):
> +    def setUp(self):
> +        super(TestSeriesLanguageBreadcrumbs, self).setUp()
> +        self.language = getUtility(ILanguageSet)['sr']
> +
> +    def test_distroserieslanguage(self):
> +        distribution = self.factory.makeDistribution(
> +            name='crumb-tester', displayname="Crumb Tester")
> +        series = self.factory.makeDistroRelease(
> +            name="test", version="1.0", distribution=distribution)
> +        serieslanguage = getUtility(IDistroSeriesLanguageSet).getDummy(
> +            series, self.language)
> +        self._testContextBreadcrumbs(
> +            [distribution, series, serieslanguage],
> +            ["http://launchpad.dev/crumb-tester",
> +             "http://launchpad.dev/crumb-tester/test",
> +             "http://translations.launchpad.dev/crumb-tester/test",
> +             "http://translations.launchpad.dev/crumb-tester/test/+lang/sr"],
> +            ["Crumb Tester", "1.0", "Translations", "Serbian (sr)"])
>  
>  
>  def test_suite():
> 

[snip]

> === modified file 'lib/lp/translations/doc/canonical_url_examples.txt'
> --- lib/lp/translations/doc/canonical_url_examples.txt	2009-09-10 09:58:30 +0000
> +++ lib/lp/translations/doc/canonical_url_examples.txt	2009-09-10 10:27:20 +0000
> @@ -151,7 +151,7 @@
>      >>> boo_bah_serbian = getUtility(IDistroSeriesLanguageSet).getDummy(
>      ...     distroseries, serbian)
>      >>> canonical_url(boo_bah_serbian)
> -    u'http://launchpad.dev/boo/bah/+lang/sr'
> +    u'http://translations.launchpad.dev/boo/bah/+lang/sr'

Great, that's what I was expecting in the last one :)
>  
>  Product, ProductSeries and ProductSeriesLanguage
>  ---------------------------------------------------
> 
> === modified file 'lib/lp/translations/model/distroserieslanguage.py'
> --- lib/lp/translations/model/distroserieslanguage.py	2009-07-17 00:26:05 +0000
> +++ lib/lp/translations/model/distroserieslanguage.py	2009-09-09 11:57:25 +0000
> @@ -52,10 +52,14 @@
>  
>      @property
>      def title(self):
> -        return '%s translations of applications in %s, %s' % (
> +        return '%s translations of %s %s' % (
>              self.language.englishname,
>              self.distroseries.distribution.displayname,
> -            self.distroseries.title)
> +            self.distroseries.displayname)

Again, it'd be nice to see a matching model test or doc updated with
this change, but I understand if it's not within the scope here.

> +
> +    @property
> +    def text(self):
> +        return self.language.englishname

And again.

>  
>      @property
>      def pofiles(self):
> @@ -158,10 +162,10 @@
>          self.dateupdated = datetime.now(tz=pytz.timezone('UTC'))
>          self.translator_count = 0
>          self.contributor_count = 0
> -        self.title = '%s translations of applications in %s, %s' % (
> +        self.title = '%s translations of %s %s' % (
>              self.language.englishname,
>              self.distroseries.distribution.displayname,
> -            self.distroseries.title)
> +            self.distroseries.displayname)
>  
>      @property
>      def pofiles(self):
> 
> === modified file 'lib/lp/translations/stories/standalone/xx-test-potlists.txt'
> --- lib/lp/translations/stories/standalone/xx-test-potlists.txt	2009-07-01 20:45:39 +0000
> +++ lib/lp/translations/stories/standalone/xx-test-potlists.txt	2009-09-09 12:41:48 +0000
> @@ -11,17 +11,3 @@
>    ...
>    ...Listing of FEW templates...
>    ...
> -
> -Check that we can get a potlist for a distroseries:
> -
> -  >>> print http(r"""
> -  ... GET /ubuntu/hoary/+potlist HTTP/1.1
> -  ... Host: translations.launchpad.dev
> -  ... """)
> -  HTTP/1.1 200 Ok
> -  Content-Length: ...
> -  Content-Type: text/html;charset=utf-8
> -  <BLANKLINE>
> -  ...
> -  ...Listing of FEW templates...
> -  ...
> 

I'm not sure why this was just removed - but assume it's because it's better
tested elsewhere?

[snip]
> === renamed file 'lib/lp/translations/templates/distroserieslanguage-index.pt' => 'lib/lp/translations/templates/serieslanguage-index.pt'
> --- lib/lp/translations/templates/distroserieslanguage-index.pt	2009-07-17 17:59:07 +0000
> +++ lib/lp/translations/templates/serieslanguage-index.pt	2009-09-09 18:52:48 +0000

Any reason for not renaming the browser/distroserieslanguage.py at the same
time?

> @@ -2,27 +2,11 @@
>    xmlns="http://www.w3.org/1999/xhtml"
>    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="context/@@main_template/master"
> -  i18n:domain="rosetta"
> +  metal:use-macro="view/macro:page/main_only"
>  >
>  
>  <body>
>  
> -<metal:rightportlets fill-slot="portlets_one">
> -  <div tal:replace="structure context/@@+portlet-stats" />
> -</metal:rightportlets>
> -
> -<metal:heading fill-slot="pageheading">
> -  <h1>
> -    Programs you can translate into
> -    <tal:language replace="context/language/englishname">Catalan</tal:language>
> -  </h1>
> -</metal:heading>
> -
>  <div metal:fill-slot="main">
>    <tal:block condition="not:view/pofiles">
>      <div class="documentDescription">
> @@ -31,15 +15,43 @@
>    </tal:block>
>  
>    <tal:block condition="view/pofiles">
> -
> -    <p>
> -      Choose a template name to begin translating. Templates that are
> -      considered more important for translation are listed first.
> -    </p>
> -
> +    <h1>Translatable templates</h1>
> +    <div class="top-portlet">
> +      <tal:with_group condition="view/translation_group">
> +        <p tal:condition="view/translation_team">
> +          <tal:product replace="structure
> +            view/parent/fmt:link">Evolution</tal:product>
> +          is translated by
> +          <tal:team replace="structure
> +                             view/translation_team/translator/fmt:link">
> +            Serbian translators</tal:team> — if you need help, or your
> +          translations are not being reviewed, please get in touch with them.
> +        </p>
> +
> +        <p tal:condition="not:view/translation_team">

> +          There is no team to translate
> +          <tal:product replace="structure
> +            view/parent/fmt:link">Evolution</tal:product>
> +          to
> +          <a tal:attributes="href context/language/fmt:url"
> +             tal:content="context/language/englishname">Serbian</a>.
> +          To set one up, please get in touch with
> +          <a tal:replace="structure view/translation_group/fmt:link"
> +             >Launchpad Translators</a>
> +          coordinator.
> +        </p>
> +      </tal:with_group>
> +
> +      <p>
> +        Choose a template name to begin translating.
> +        Templates which are more important to translate are listed first.
> +      </p>
> +    </div>
> +
> +    <div style="max-width:840px;">
>      <tal:navigation replace="structure view/batchnav/@@+navigation-links-upper" />
>  
> -    <table class="listing sortable" width="100%" id="translationstatuses">
> +    <table class="listing sortable" id="translationstatuses">

As suggested above, adding a class 'translation-stats' here rather than on
the tbody allows you to remove the class on the tfoot too. Also, I can't
find the id 'translationstatuses' used anywhere in LP? Maybe it was for an
old test?

>        <thead>
>          <tr>
>            <th>Template Name</th>
> @@ -52,10 +64,10 @@
>            <th>By</th>
>          </tr>
>        </thead>
> -      <tbody>
> +      <tbody class="translation-stats">

As above.

>          <tr tal:repeat="entry view/pofiles"
>              tal:attributes="id string:${entry/potemplate/name}">
> -          <td>
> +          <td class="template-name">
>              <a tal:attributes="href string:${entry/fmt:url}/+translate"
>                 tal:content="entry/potemplate/name">
>                 apache2-dev
> @@ -146,34 +158,31 @@
>              </tal:block>
>            </td>
>          </tr>
> +        <tfoot>
> +          <tr class="overall-translation-stats">

As above - this class could be removed.

> +            <th>Overall statistics:</th>
> +            <td><span tal:replace="context/messageCount">N</span></td>
> +            <td>
> +              <span style="white-space: nowrap"
> +                    tal:content="structure context/@@+barchart">--</span>
> +            </td>
> +            <td><span tal:replace="context/untranslatedCount">N</span></td>
> +            <td><span tal:replace="context/unreviewedCount">N</span></td>
> +            <td><span tal:replace="context/updatesCount">N</span></td>
> +            <td colspan="2"></td>
> +          </tr>
> +        </tfoot>
>        </tbody>
>      </table>
> -
>      <tal:navigation replace="structure view/batchnav/@@+navigation-links-lower" />
>  
> -    <div tal:replace="structure context/@@+rosetta-status-legend" />
> -
> +    <div class="translations-legend">
> +      <div tal:replace="structure context/@@+rosetta-status-legend" />
> +    </div>
> +    </div><!-- max-width -->
>    </tal:block>
>  
>  </div>
>  
> -<div metal:fill-slot="help">
> -  <p>
> -    This page shows each of the translation templates in each of the source
> -    packages in the
> -    <span tal:replace="context/distroseries/displayname">hoary</span>
> -    series of
> -    <span tal:replace="context/distroseries/distribution/displayname">Ubuntu
> -    </span>. Select a template to start translating.
> -  </p>
> -  <ul class="info">
> -    <li>
> -      <a href="https://help.launchpad.net/RosettaFAQ"
> -      >Frequently asked questions</a>
> -    </li>
> -  </ul>
> -</div>
> -
> -
>  </body>
>  </html>
>