Launchpad itself

Code review comment for lp:~danilo/launchpad/bug-516317

bug-516317
Merge into devel

Revision history for this message

Данило Шеган (danilo) wrote on 2010-12-30:

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:

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 :)

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:

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 :)

« Back to merge proposal