-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Данило Шеган wrote: > Hi Michael, > > Thanks a lot for the review. Comments inline. > Thanks for the changes! r=me - some comments below. Cheers, Michael. > >> 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. OK. It's up to you, but I'm guessing: table.listing.translation-stats tbody td {...} would take precedence (ie. selecting both classes - as it's more specific). I'm not fussed either way - what you've got works, and it's not a huge improvement to remove one class. > >>> +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. :( Really? That is very strange. Try the more specific one I mentioned above - selecting both classes - I'd be interested to know why it was been over-ruled... is there something like firebug for epiphany? > >>> +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. Great. > >>> === 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. Well *sorrrry* - I'd forgotten it was you helping barry with the rules - in previous reviews people haven't been aware. Next time I'll just keep my mouth shut ;) > >>> @@ -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. Great. > >>> === 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? I think a comment would be fine - it would have cleared up my confusion. > >>> === 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. Great. > >>> + >>> + @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! Ah nice. > >>> === 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 :) heh - ok, I missed the zcml removal. > > 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. :) Great. > >>> - <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! No problem - I hope you didn't feel patronized by me pointing out things that were already obvious to you. > > Incremental diff attached. > > > - -- Michael -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkqp+RgACgkQGSan/irvan2hiQCdF25eYY0tTqxnfvLFuoJJl/G1 whoAn1rxAwJmN/u+vKtNQ/K+12+yA7g/ =LBxa -----END PGP SIGNATURE-----