-----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
>>> -
>>> - ...
>>> - ...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.
>
>>> -
>>> +
>> 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-----