Hi Michael,
Thanks a lot for the review. Comments inline.
У чет, 10. 09 2009. у 14:44 +0000, Michael Nelson пише:
> 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.
Indeed, this is something I forgot to fix up from the 'prototyping'
phase.
> > === 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
...
> > +
> > +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]
Fixed.
> 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) {...}
Oh, I know about that CSS3 feature. However, considering my main
browser is Epiphany which still uses xulrunner 1.9 (I believe shared
with Firefox 2 or 3), I am not personally interested :)
Just kidding, I am not going to change styles on existing table too
much, if at all. I am trying to make this mostly mechanical.
> 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:
That's actually what I started with. However, fighting legacy CSS code
is hell. 'table.listing tbody td' takes precedence over
'table.translation-stats tbody td'. I am not yet ready to do away with
class='listing' on these tables, though, simply because I am not trying
to solve all the problems with these pages. It's mostly mechanical
changes, and no real redesign.
> > +tbody.translation-stats td {
>
> table.translation-stats td {...
>
> (that way you don't need a separate rule for the tfoot below.)
Actually, the 'cascading' of CSS seems to cause me problems in at least
Epiphany (xulrunner 1.9): if I set the rule up like that (that's what I
started with), it doesn't get picked up. I'll play a bit more with CSS
and will follow up.
It's good to know that even CSS precedence rules differ between
browsers. :(
> > +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
Right, fixed.
> > === modified file 'lib/lp/translations/browser/distroserieslanguage.py'
...
> >
> > + @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
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
At the moment, I either can't remove it from pagetitles.py or I have to
define it like this. And yes, I am completely aware of the work Barry's
doing, and I am completely aware that I'll have to go through all the
translations pages to make sure that breadcrumbs, page titles, h2's and
h1's make sense on them. And that's exactly what I was planning to do
anyway.
> > @@ -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.
Yep, added.
> > === 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.
The way request.traversed_objects behaves is strange. Items are added
only if they provide Navigation interface and
Navigation.publishTraverse() is called. However, for leafs of top-level
object (root), they don't need the Navigation interface because they get
a breadcrumb even if the object doesn't appear in the
request.traversed_objects. And for top level objects, we don't display
any breadcrumb anyway.
Perhaps it'd make some sense for me to provide Navigation class for
ITranslationGroupSet, just so it's more consistent and so no confusion
arises in the future. Or perhaps just comment that this doesn't show up
in the request.traversed_objects because it doesn't provide Navigation
class.
What do you think?
> > === 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.
Oh, indeed: I added the appropriate doctest for this one.
> > +
> > + @property
> > + def text(self):
> > + return self.language.englishname
>
> And again.
This was not really used, so I've removed it (a left over from some
earlier ideas when constructing breadcrumbs): good catch!
> > === 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
> > -
> > - ...
> > - ...Listing of FEW templates...
> > - ...
> >
>
> I'm not sure why this was just removed - but assume it's because it's better
> tested elsewhere?
No, it's just removed. DistroSeries:+potlist registration is removed in
ZCML because it's not used anywhere, so removing a test for removed
features helps the test suite pass :)
The template is still used on SourcePackage:+potlist, so I can't fully
remove it, but... some day :)
> > === 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?
Of course. Template is to be shared with
browser/productserieslanguage.py (and in the future, I hope
projectserieslanguage and sourcepackagelanguage), but views are far from
being able to use the same code just yet.
That's actually what the next part of the branch is about — using
serieslanguage-index.pt for ProductSeriesLanguage. :)
> > -
> > +
>
> 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?
As I said above, I'll have to look into this some more and see what
works best. Just doing the above doesn't, at least not in Epiphany.
And while I do test in a bunch of browser, nobody can make me write code
that doesn't work in browser of my choice :)
Thanks for the review, much appreciated!
Incremental diff attached.