Issue 5740 - Implemented new abp.org footer styles

Issue 5740 - Implemented new abp.org footer styles

[juliandoucette, 2017-09-28 16:33:22]

First draft. This is pretty messy. I'll review myself too.

[juliandoucette, 2017-10-05 16:04:21]

More updates coming. Sorry for the delay.

https://codereview.adblockplus.org/29558641/diff/29565736/includes/index.tmpl
File includes/index.tmpl (left):

https://codereview.adblockplus.org/29558641/diff/29565736/includes/index.tmpl...
includes/index.tmpl:188: <section class="hidden" id="more-container">
Note: I will remove this change from the next patchset. I have removed this
section separately in the following review
https://codereview.adblockplus.org/29565748.

[juliandoucette, 2017-10-05 17:09:57]

Added some notes.

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/index-de...
File static/css/index-desktop.css (left):

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/index-de...
static/css/index-desktop.css:4: margin-bottom: 290px;
Because #footer-main was absolutely positioned :/

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/index-de...
static/css/index-desktop.css:190: }
Dono what's up with this.

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css
File static/css/main.css (left):

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css...
static/css/main.css:141: nav ul
Note: Both the header and the footer contain navs. This change doesn't seem to
break the header. But I'm not too concerned about breaking the header because I
almost completely reimplemented it in https://codereview.adblockplus.org/

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css...
static/css/main.css:72: box-sizing: border-box; }
Oops! I fixed an issue here
https://codereview.adblockplus.org/29564718/diff/29564719/static/css/main.css...
- I'll update the other patchset and rebase.

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css...
static/css/main.css:348: #site-footer #social-list
This is suspicious.

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css...
static/css/main.css:359: #site-footer
I should probably order and/or separate (comment) this better (everything
#site-footer related).

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css...
static/css/main.css:375: #site-footer h5::after
Note: I took this from https://codereview.adblockplus.org/29536654/

https://codereview.adblockplus.org/29558641/diff/29565736/static/css/main.css...
static/css/main.css:380: height: 3px;
This is probably too wide.

[juliandoucette, 2017-10-06 11:48:01]

Properly rebased. I'm updating this implementation now.

[juliandoucette, 2017-10-06 12:58:01]

Ready for review.

Note: I changed font-sizes from px to em according to the sizes in the
styleguide (https://eyeogmbh.invisionapp.com/share/AUCYJCC8B#/screens). But I
didn't change the sizes in which these ems are based yet (I will here
https://issues.adblockplus.org/ticket/5723). As a result [these sizes will not
look right yet, these sizes will be future-proof if we change the base sizes]. 

(I expect that we will make the px sizes in the sytleguide smaller because they
are currently huge.)

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:330: #site-footer h5
Note: h5 will inherit color from #site-footer and margin/font-size from global
styles. As a result, I will address this (the site-wide h5 style) separately.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:341: width: 30px;
Note: I used em for this width in the new implementation. This is a little
unintuitive because you would think that 2em = 2 letters width. But this is not
true because this is not a monospace font. I eyeballed this width based on an h5
font-size of 22px.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:348: #site-footer p
Note: I will inherit this font-size from #site-footer instead.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:353: #site-footer li
Note: I would like to inherit this style from reset.css. But I think that is
out-of-scope for this change.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:365: font-size: 18px;
Note: I will inherit this font-size from #site-footer.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:374: color: #fff;
Note: This is indistinguishable. But it's better than nothing IMO (this change
was improvised/unspecified).

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:380: margin-top: 3em;
Note: This was improvised (Paul said that he made a similar design, and approved
my spacing). Anyway, I thought 3em was a little too much. So I changed it to
2em.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:400: #social-list li,
Note: I separated these because they are separate ~components. I am choosing
readability over performance in that way. I don't have a strong opinion about
this case in particular.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:404: float: left;
Note: I forgot to support rtl here. I'm not really sure of the value of
reversing the order when rtl. But I will anyway.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:409: margin-right: 10px;
Note: This width wasn't specified, but it looks larger in the mockups. I made it
smaller so that I could fit all of these glyphs on one line on smaller screens.
Of course, I could use a media query or a % width instead. But I won't unless
you think it's necessary.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:419: height: 40px;
Note: 50px was specified, but 50px made it too hard to fit these all on one line
(especially on smaller screens). I chose 40px (2em) instead.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:428: #legal-list li:last-of-type::after
Note: I'm being a little tricky here using ::after (IE9+) and :last-of-type
(IE9+) for progressive enhancement (the "|" will not show at all lt IE9)

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:433: @media(min-width: 1024px)
Note: I made this breakpoint larger [to match the largest grid breakpoint, to
avoid clearing if the copyright notice is longer when translated]

[juliandoucette, 2017-10-06 13:03:41]

Added afterthoughts / things I noticed at a glance.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:254: #social-list ul
Forgot about this style :D - It should probably be moved or removed.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:303:
/*******************************************************************************
What do you think of this comment style?

/**
 * Category
 */

/* Subcategory
 */

(except lines are 80 char width)

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:420: 
Maybe I should put a #site-footer comment here?

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:444: 
Maybe I should move this up? e.g. under #site-footer or #site-footer
#social-list? What do you think?

[ire, 2017-10-09 10:37:21]

On 2017/10/06 12:58:01, juliandoucette wrote:
> Ready for review.
> 
> Note: I changed font-sizes from px to em according to the sizes in the
> styleguide (https://eyeogmbh.invisionapp.com/share/AUCYJCC8B#/screens). But I
> didn't change the sizes in which these ems are based yet (I will here
> https://issues.adblockplus.org/ticket/5723). As a result [these sizes will not
> look right yet, these sizes will be future-proof if we change the base sizes].

> 
> (I expect that we will make the px sizes in the sytleguide smaller because
they
> are currently huge.)

Ack.

[ire, 2017-10-09 10:38:05]

Thanks Julian! Comments below.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:330: #site-footer h5
On 2017/10/06 12:57:58, juliandoucette wrote:
> Note: h5 will inherit color from #site-footer and margin/font-size from global
> styles. As a result, I will address this (the site-wide h5 style) separately.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:341: width: 30px;
On 2017/10/06 12:58:01, juliandoucette wrote:
> Note: I used em for this width in the new implementation. This is a little
> unintuitive because you would think that 2em = 2 letters width. But this is
not
> true because this is not a monospace font. I eyeballed this width based on an
h5
> font-size of 22px.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:348: #site-footer p
On 2017/10/06 12:57:58, juliandoucette wrote:
> Note: I will inherit this font-size from #site-footer instead.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:353: #site-footer li
On 2017/10/06 12:57:58, juliandoucette wrote:
> Note: I would like to inherit this style from reset.css. But I think that is
> out-of-scope for this change.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:365: font-size: 18px;
On 2017/10/06 12:57:59, juliandoucette wrote:
> Note: I will inherit this font-size from #site-footer.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:374: color: #fff;
On 2017/10/06 12:57:59, juliandoucette wrote:
> Note: This is indistinguishable. But it's better than nothing IMO (this change
> was improvised/unspecified).

I agree

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:380: margin-top: 3em;
On 2017/10/06 12:57:58, juliandoucette wrote:
> Note: This was improvised (Paul said that he made a similar design, and
approved
> my spacing). Anyway, I thought 3em was a little too much. So I changed it to
> 2em.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:404: float: left;
On 2017/10/06 12:58:00, juliandoucette wrote:
> Note: I forgot to support rtl here. I'm not really sure of the value of
> reversing the order when rtl. But I will anyway.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:409: margin-right: 10px;
On 2017/10/06 12:57:58, juliandoucette wrote:
> Note: This width wasn't specified, but it looks larger in the mockups. I made
it
> smaller so that I could fit all of these glyphs on one line on smaller
screens.
> Of course, I could use a media query or a % width instead. But I won't unless
> you think it's necessary.

I don't think it is necessary. IMO the spacing is good and readable

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:419: height: 40px;
On 2017/10/06 12:58:00, juliandoucette wrote:
> Note: 50px was specified, but 50px made it too hard to fit these all on one
line
> (especially on smaller screens). I chose 40px (2em) instead.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:428: #legal-list li:last-of-type::after
On 2017/10/06 12:58:00, juliandoucette wrote:
> Note: I'm being a little tricky here using ::after (IE9+) and :last-of-type
> (IE9+) for progressive enhancement (the "|" will not show at all lt IE9)

Love it!

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:433: @media(min-width: 1024px)
On 2017/10/06 12:57:58, juliandoucette wrote:
> Note: I made this breakpoint larger [to match the largest grid breakpoint, to
> avoid clearing if the copyright notice is longer when translated]

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:254: #social-list ul
On 2017/10/06 13:03:40, juliandoucette wrote:
> Forgot about this style :D - It should probably be moved or removed.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:303:
/*******************************************************************************
On 2017/10/06 13:03:40, juliandoucette wrote:
> What do you think of this comment style?
> 
> /**
>  * Category
>  */
> 
> /* Subcategory
>  */
> 
> (except lines are 80 char width)

I don't mind it being used for subcategories, but I prefer the full-length lines
for the headings of each category. I's much easier to read for me

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:321: #site-footer h5::after
This should be `:after`

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:326: height: 3px;
If the width changes with the font size, I think the height should too

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:327: margin: 1em 0em 0em 0em;
NIT: I don't think you need to set the margins for all the sides since the only
change you're making is with the top margin

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:328: border: none;
I don't think this style is needed. There isn't a border applied by default
(unless this happens in some browser I'm not aware of)

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:334: list-style-type: none;
NIT: I don't know if it makes a difference, but I prefer to add the `list-style:
none` to the `ul` element, and it'll be applied to the `li`s. In my mind (no
evidence for/against) it's more performant. I'll have to do some research on if
this is actually the case or not..

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:359: #social-list li
I think your trick of using negative margins on the parent and positive margins
on the children will work perfectly here, so you don't need the floating or the
rtl styles. e.g.:

```
#social-list ul
{
  margin-right: -0.5em;
  margin-left: -0.5em;
}

#social-list li
{
  display: inline-block;
  margin-right: 0.5em;
  margin-left: 0.5em;
}

```

And from my tests everything still fits on the one line even with the additional
space between the icons

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:390: overflow: auto;
NIT: I was a bit confused about why this style was here, then I realised it was
to stop the margins collapsing. To my eyes, it looks as though the non-collapsed
margins are bigger (therefore uneven) than the space between the other list
sections, so I'm not sure it's actually necessary to have.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:412: margin-right: 0.25em;
Since the margin i sonly applied to the `::after` element, it doesn't apply to
lt IE 9 so the list items have no spacing between them.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:420: 
On 2017/10/06 13:03:40, juliandoucette wrote:
> Maybe I should put a #site-footer comment here?

Sure. Or you can group all the `#site-footer #footer-legal` under that one
comment instead of further diving them. I'm fine either way.

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:166: <li><a href="/releases/"
hreflang="en">{{get_string("releases", "menu")}}</a></li>
NIT: There's no whitespace around the "{{" and content here (and a few other
places in this file)

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:185: <a rel="nofollow"
href="https://twitter.com/AdblockPlus" target="_blank">
I thought we agreed to not include the `rel=nofollow` manually?

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:201: src="/img/footer-facebook-glyphicon.png"
The images need to be optimised

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:216: <p id="copyright-notice">{{ "Copyright © 2017 All
rights reserved. Adblock Plus® is a registered trademark of <a
href='https://eyeo.com'>eyeo GmbH</a>." | translate("copyright-notice", "Footer
copyright notice") }}</p>
The ® could be wrapped in a <sup> and made a but smaller relative to the size of
the rest of the text

[juliandoucette, 2017-10-09 16:44:13]

Quick response.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:303:
/*******************************************************************************
On 2017/10/09 10:38:03, ire wrote:
> On 2017/10/06 13:03:40, juliandoucette wrote:
> > What do you think of this comment style?
> > 
> > /**
> >  * Category
> >  */
> > 
> > /* Subcategory
> >  */
> > 
> > (except lines are 80 char width)
> 
> I don't mind it being used for subcategories, but I prefer the full-length
lines
> for the headings of each category. I's much easier to read for me

That's what I meant by "(except lines are 80 char width)"... as seen
above/below.

[ire, 2017-10-09 17:01:14]

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:303:
/*******************************************************************************
On 2017/10/09 16:44:12, juliandoucette wrote:
> On 2017/10/09 10:38:03, ire wrote:
> > On 2017/10/06 13:03:40, juliandoucette wrote:
> > > What do you think of this comment style?
> > > 
> > > /**
> > >  * Category
> > >  */
> > > 
> > > /* Subcategory
> > >  */
> > > 
> > > (except lines are 80 char width)
> > 
> > I don't mind it being used for subcategories, but I prefer the full-length
> lines
> > for the headings of each category. I's much easier to read for me
> 
> That's what I meant by "(except lines are 80 char width)"... as seen
> above/below.

Oh alright. I thought you meant literally what you wrote. I like the style then.

[juliandoucette, 2017-10-11 12:11:00]

Rebased

[juliandoucette, 2017-10-11 13:11:32]

Ready for review

[juliandoucette, 2017-10-11 13:12:10]

Added notes, Ready for review.

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567717/static/css/main.css...
static/css/main.css:409: margin-right: 10px;
On 2017/10/09 10:38:00, ire wrote:
> On 2017/10/06 12:57:58, juliandoucette wrote:
> > Note: This width wasn't specified, but it looks larger in the mockups. I
made
> it
> > smaller so that I could fit all of these glyphs on one line on smaller
> screens.
> > Of course, I could use a media query or a % width instead. But I won't
unless
> > you think it's necessary.
> 
> I don't think it is necessary. IMO the spacing is good and readable

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:321: #site-footer h5::after
On 2017/10/09 10:38:03, ire wrote:
> This should be `:after`

Done.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:326: height: 3px;
On 2017/10/09 10:38:03, ire wrote:
> If the width changes with the font size, I think the height should too

Good point. Done.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:327: margin: 1em 0em 0em 0em;
On 2017/10/09 10:38:02, ire wrote:
> NIT: I don't think you need to set the margins for all the sides since the
only
> change you're making is with the top margin

Done.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:328: border: none;
On 2017/10/09 10:38:02, ire wrote:
> I don't think this style is needed. There isn't a border applied by default
> (unless this happens in some browser I'm not aware of)

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:334: list-style-type: none;
On 2017/10/09 10:38:02, ire wrote:
> NIT: I don't know if it makes a difference, but I prefer to add the
`list-style:
> none` to the `ul` element, and it'll be applied to the `li`s. In my mind (no
> evidence for/against) it's more performant. I'll have to do some research on
if
> this is actually the case or not..

Acknowledged.

Never thought about this.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:359: #social-list li
On 2017/10/09 10:38:03, ire wrote:
> I think your trick of using negative margins on the parent and positive
margins
> on the children will work perfectly here, so you don't need the floating or
the
> rtl styles. e.g.:
> 
> ```
> #social-list ul
> {
>   margin-right: -0.5em;
>   margin-left: -0.5em;
> }
> 
> #social-list li
> {
>   display: inline-block;
>   margin-right: 0.5em;
>   margin-left: 0.5em;
> }
> 
> ```
> 
> And from my tests everything still fits on the one line even with the
additional
> space between the icons

Yep. Done.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:390: overflow: auto;
On 2017/10/09 10:38:03, ire wrote:
> NIT: I was a bit confused about why this style was here, then I realised it
was
> to stop the margins collapsing. To my eyes, it looks as though the
non-collapsed
> margins are bigger (therefore uneven) than the space between the other list
> sections, so I'm not sure it's actually necessary to have. 

There is no good reason for this.

https://codereview.adblockplus.org/29558641/diff/29567730/static/css/main.css...
static/css/main.css:412: margin-right: 0.25em;
On 2017/10/09 10:38:02, ire wrote:
> Since the margin i sonly applied to the `::after` element, it doesn't apply to
> lt IE 9 so the list items have no spacing between them.

Good point.

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:185: <a rel="nofollow"
href="https://twitter.com/AdblockPlus" target="_blank">
On 2017/10/09 10:38:04, ire wrote:
> I thought we agreed to not include the `rel=nofollow` manually?

I forgot / it was there already.

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:201: src="/img/footer-facebook-glyphicon.png"
On 2017/10/09 10:38:04, ire wrote:
> The images need to be optimised

Thank you for the reminder (I just optimized the header svg after the fact as a
result).

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:216: <p id="copyright-notice">{{ "Copyright © 2017 All
rights reserved. Adblock Plus® is a registered trademark of <a
href='https://eyeo.com'>eyeo GmbH</a>." | translate("copyright-notice", "Footer
copyright notice") }}</p>
On 2017/10/09 10:38:04, ire wrote:
> The ® could be wrapped in a <sup> and made a but smaller relative to the size
of
> the rest of the text

Done.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:158: <footer id="site-footer" class="content">
Note: I'm going to change this to just "footer" for consistency with the
#navbar. But I may change both of these separately.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:165: {{ pageitem(name) }}
Note: I'm going to refactor away pageitem so that I can remove it later.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:208: alt="{{ "Facebook" | translate("facebook-alt",
"Facebook glyphicon alt text")}}">
Note: I'm going to remove these alts and add them separately

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:229: </div><!-- .container -->
Note: I changed the comments to {# #} so that they don't show up in the
generated HTML

https://codereview.adblockplus.org/29558641/diff/29573692/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29573692/templates/simple.tm...
templates/simple.tmpl:34: <footer id="site-footer">
Note: I'm going to exclude this change

(uninstalled and uninstalled-submit use simple.tmpl)

[ire, 2017-10-11 16:28:33]

Thanks! LGTM

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:185: <a rel="nofollow"
href="https://twitter.com/AdblockPlus" target="_blank">
On 2017/10/11 13:12:08, juliandoucette wrote:
> On 2017/10/09 10:38:04, ire wrote:
> > I thought we agreed to not include the `rel=nofollow` manually?
> 
> I forgot / it was there already.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29567730/templates/default.t...
templates/default.tmpl:201: src="/img/footer-facebook-glyphicon.png"
On 2017/10/11 13:12:07, juliandoucette wrote:
> On 2017/10/09 10:38:04, ire wrote:
> > The images need to be optimised
> 
> Thank you for the reminder (I just optimized the header svg after the fact as
a
> result).

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:158: <footer id="site-footer" class="content">
On 2017/10/11 13:12:08, juliandoucette wrote:
> Note: I'm going to change this to just "footer" for consistency with the
> #navbar. But I may change both of these separately.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:165: {{ pageitem(name) }}
On 2017/10/11 13:12:09, juliandoucette wrote:
> Note: I'm going to refactor away pageitem so that I can remove it later.

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:208: alt="{{ "Facebook" | translate("facebook-alt",
"Facebook glyphicon alt text")}}">
On 2017/10/11 13:12:09, juliandoucette wrote:
> Note: I'm going to remove these alts and add them separately

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/default.t...
templates/default.tmpl:229: </div><!-- .container -->
On 2017/10/11 13:12:09, juliandoucette wrote:
> Note: I changed the comments to {# #} so that they don't show up in the
> generated HTML

Acknowledged.

https://codereview.adblockplus.org/29558641/diff/29573692/templates/simple.tmpl
File templates/simple.tmpl (right):

https://codereview.adblockplus.org/29558641/diff/29573692/templates/simple.tm...
templates/simple.tmpl:34: <footer id="site-footer">
On 2017/10/11 13:12:09, juliandoucette wrote:
> Note: I'm going to exclude this change
> 
> (uninstalled and uninstalled-submit use simple.tmpl)

Since there are no longer any styles for #footer-main, maybe just remove the ID
altogether?

[juliandoucette, 2017-10-11 17:55:54]

On 2017/10/11 16:28:33, ire wrote:
> Since there are no longer any styles for #footer-main, maybe just remove the
ID
> altogether?

Paul wants us to use the normal header and footer on these pages. I will address
this separately because it's not trivial (I will have to re-style the contents
of these pages).

[juliandoucette, 2017-10-12 11:51:20]

https://bitbucket.org/adblockplus/adblockplus.org/commits/845be27b7839963e1c5...