Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1347)

Issue 29599635: Issue 5717 - Update footer contents (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by juliandoucette
Modified:
1 year, 5 months ago
Reviewers:
ire
Base URL:
https://bitbucket.org/adblockplus/adblockplus.org
Visibility:
Public.

Description

Issue 5717 - Update footer contents

Patch Set 1 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -51 lines) Patch
M locales/en/menu.json View 1 chunk +36 lines, -30 lines 6 comments Download
M templates/default.tmpl View 3 chunks +35 lines, -21 lines 6 comments Download

Messages

Total messages: 4
juliandoucette
1 year, 8 months ago (2017-11-06 17:16:21 UTC) #1
juliandoucette
On 2017/11/06 17:16:21, juliandoucette wrote: Relevant: https://bitbucket.org/adblockplus/spec/pull-requests/76/added-abp-website-specifications/diff
1 year, 8 months ago (2017-11-06 17:42:23 UTC) #2
ire
Thanks Julian. Initial comments. https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.json File locales/en/menu.json (right): https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.json#newcode32 locales/en/menu.json:32: "help-centre-link": { help-centre or help-center? ...
1 year, 8 months ago (2017-11-07 08:36:16 UTC) #3
juliandoucette
1 year, 8 months ago (2017-11-07 13:08:56 UTC) #4
Thanks Ire!

I'll patch this up in the afternoon.

https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.json
File locales/en/menu.json (right):

https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.jso...
locales/en/menu.json:32: "help-centre-link": {
On 2017/11/07 08:36:15, ire wrote:
> help-centre or help-center?

We're supposed to use the US English spelling.

(Canadian English is closer to UK English in spelling.)

https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.jso...
locales/en/menu.json:35: "contribute-link": {
On 2017/11/07 08:36:15, ire wrote:
> NIT: Why suffix these with "-link"? 
> 
> Also "Contribute" is already translated above

Acknowledged.  This could be translated differently depending on the context
"contribute" alone vs "contribute" as a link in a list with a heading. But you
are right, it was translated for this context above. And the same applies
elsewhere.

https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.jso...
locales/en/menu.json:42: "message": "Report a bug"
On 2017/11/07 08:36:15, ire wrote:
> SuperNIT: The casing of the titles is a inconsistent. e.g. "Report a bug" vs
> "Source Code". I think title case for all would work best

Agreed. Good catch.

https://codereview.adblockplus.org/29599635/diff/29599636/templates/default.tmpl
File templates/default.tmpl (left):

https://codereview.adblockplus.org/29599635/diff/29599636/templates/default.t...
templates/default.tmpl:17: 
On 2017/11/07 08:36:16, ire wrote:
> NIT: Why remove this space?

Mistake.

https://codereview.adblockplus.org/29599635/diff/29599636/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29599635/diff/29599636/templates/default.t...
templates/default.tmpl:77: {{ 'target="_blank"' if href }}>
On 2017/11/07 08:36:16, ire wrote:
> This is applied to all links because all of them are passing a `href`

Good catch.

https://codereview.adblockplus.org/29599635/diff/29599636/templates/default.t...
templates/default.tmpl:156: <nav class="column one-fourth">
On 2017/11/07 08:36:16, ire wrote:
> NIT: I think this whole <nav> could also be a macro. Or you could make just
the
> <nav> a macro and not the <li> within. That way the whole navigation
links/names
> (excluding the contact-nav with the social links) could be placed above in a
set
> and easily changed without going into the markup.

Detail: I chose to place *just* the `<li>` in the macro so that I could use it
elsewhere (e.g. the navbar) separately. Of course, I could create two macros
[one for the <li>, one for the <nav>]. Or I could just use template features
e.g.

{% for heading, links in [
  'developers', ('source', ...),
  ...
] %}
<nav ...>
  <h ...>{{ heading }}
   <ul>
   {% for sid, href in links  %}
     {{ menuitem(sid, href) }}
   ...

---

It makes little difference.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5