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 > > - <BLANKLINE> > > - ... > > - ...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. :) > > - <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? 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.