launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Henning, thanks again for the review.  I didn't apply all of your
comments though, so see below.

У сре, 29. 12 2010. у 16:08 +0000, Henning Eggers пише:
> 
> This may be missing a site="translations" parameter but I just checked
> the other links on the project page and they all go to the root site.
> In fact they seem to be available on all sites. All other links to
> +configure-translations go to the translations site. I don't know if
> there is some kind of concept behind this setup or if it is just an
> oversight.
> 
> For a distribution, +configure-translations is only available on the
> translations site. This adds to my confusion here.

Indeed.  As explained on IRC, all this is intentional, but just because
I touch on work done by others.  Ideally, I think we should just let
them live on the translations site, but that should be made a general
policy.  I didn't change distribution one because that one is basically
entirely different, and I brought them only slightly closer together (by
using the same page ID and narrative).

> > +            "The release series translators should focus on."),
> 
> "The release series *that* translators should focus on." is much
> better to read. 

I disagree, and looking online, so do many others (especially in
writing).  I've still adjusted it (in both places) so as to make my nice
reviewer happy :)

As for the facet attribute, entire section is included in <facet
facet="translations"> tag so I wanted to cancel it out explicitely.
Still, considering it's useless, I removed it.


> >  class DistributionSettingsView(TranslationsMixin,
> DistributionEditView):
> > -    label = "Set permissions and policies"
> > -    field_names = ["translationgroup", "translationpermission"]
> > -
> > -    @property
> > -    def page_title(self):
> > -        return "Set translation permissions for %s" % (
> > -            self.context.displayname)
> > +    label = "Translations settings"
> 
> Yeah! Finally this page is converted to 3.0! ;) I think, though, that
> it is custom to include the displayname of the context in the label.

I believe that we (LP team) have had a long discussion about this in the
past, and agreed that it isn't.  The result of not including it makes
the following critical elements as such:

| Settings : Translations : Ubuntu (Browser title)
+-------------------------------------------------
|  __
| /  \    Ubuntu Linux
| \  /
|  ^^     Overview  Code  Bugs  *Translations*  Answers
|
| <h1>Translation settings</h1>
| Ubuntu > Translations > Settings


And I hold a very strong opinion that including context is not necessary
considering[*] context is mentioned in the page title, at the top of the
page and in breadcrumbs right after the heading.  The only decent
argument in favor of including it I could ever hear is that it is better
for search engines.  But, with context being so prominent anyway, I
doubt it will make much of a difference.

[*] "...considering *that* context...", if you please :)


-- 
https://code.launchpad.net/~danilo/launchpad/bug-516317/+merge/44746
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-516317 into lp:launchpad.