Issue 5717 - Update footer contents

Issue 5717 - Update footer contents

[juliandoucette, 2017-11-06 17:42:23]

On 2017/11/06 17:16:21, juliandoucette wrote:

Relevant:
https://bitbucket.org/adblockplus/spec/pull-requests/76/added-abp-website-spe...

[ire, 2017-11-07 08:36:16]

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.jso...
locales/en/menu.json:32: "help-centre-link": {
help-centre or help-center?

https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.jso...
locales/en/menu.json:35: "contribute-link": {
NIT: Why suffix these with "-link"? 

Also "Contribute" is already translated above

https://codereview.adblockplus.org/29599635/diff/29599636/locales/en/menu.jso...
locales/en/menu.json:42: "message": "Report a bug"
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

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: 
NIT: Why remove this space?

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 }}>
This is applied to all links because all of them are passing a `href`

https://codereview.adblockplus.org/29599635/diff/29599636/templates/default.t...
templates/default.tmpl:156: <nav class="column one-fourth">
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.

[juliandoucette, 2017-11-07 13:08:56]

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.