No Issue - Add social media buttons to the footer ABB.org

No Issue - Add social media buttons to the footer ABB.org

[saroyanm, 2015-09-08 09:48:00]

Thomas can you please have a look ?

[Thomas Greiner, 2015-09-08 09:59:52]

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css...
static/css/main.css:326: display: inline-block;
Detail: This is redundant because this style is applied by default to `<select>`
elements.

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css...
static/css/main.css:341: margin: 10px 0px;
Why is there a "margin-top" and "margin-bottom"? `#language` doesn't have one
either.

https://codereview.adblockplus.org/29326071/diff/29326072/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29326071/diff/29326072/templates/default.t...
templates/default.tmpl:81: <img src="/images/facebook.png"
alt="{{get_string("facebook-icon-alt", "footer")}}">
Detail: Other `<img>` tags on this page are closed explicitly (i.e. `<img />`).

https://codereview.adblockplus.org/29326071/diff/29326072/templates/default.t...
templates/default.tmpl:86: <a href="https://google.com/+AdblockPlus">
Detail: While this link does work, it will redirect to
https://plus.google.com/+AdblockPlus so let's link directly to that URL to avoid
that.

[saroyanm, 2015-09-08 10:04:30]

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css...
static/css/main.css:326: display: inline-block;
On 2015/09/08 09:59:52, Thomas Greiner wrote:
> Detail: This is redundant because this style is applied by default to
`<select>`
> elements.

Done.

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css...
static/css/main.css:341: margin: 10px 0px;
On 2015/09/08 09:59:52, Thomas Greiner wrote:
> Why is there a "margin-top" and "margin-bottom"? `#language` doesn't have one
> either.

It's the case for smaller screens when the social media buttons wrap down under
the select box.

https://codereview.adblockplus.org/29326071/diff/29326072/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29326071/diff/29326072/templates/default.t...
templates/default.tmpl:81: <img src="/images/facebook.png"
alt="{{get_string("facebook-icon-alt", "footer")}}">
On 2015/09/08 09:59:52, Thomas Greiner wrote:
> Detail: Other `<img>` tags on this page are closed explicitly (i.e. `<img
/>`).

Done.

https://codereview.adblockplus.org/29326071/diff/29326072/templates/default.t...
templates/default.tmpl:86: <a href="https://google.com/+AdblockPlus">
On 2015/09/08 09:59:52, Thomas Greiner wrote:
> Detail: While this link does work, it will redirect to
> https://plus.google.com/+AdblockPlus so let's link directly to that URL to
avoid
> that.

Done.

[Thomas Greiner, 2015-09-08 10:08:10]

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css...
static/css/main.css:341: margin: 10px 0px;
On 2015/09/08 10:04:30, saroyanm wrote:
> On 2015/09/08 09:59:52, Thomas Greiner wrote:
> > Why is there a "margin-top" and "margin-bottom"? `#language` doesn't have
one
> > either.
> 
> It's the case for smaller screens when the social media buttons wrap down
under
> the select box.

Ok, in that case "margin-top" should be sufficient.

[saroyanm, 2015-09-08 10:12:17]

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29326071/diff/29326072/static/css/main.css...
static/css/main.css:341: margin: 10px 0px;
On 2015/09/08 10:08:09, Thomas Greiner wrote:
> On 2015/09/08 10:04:30, saroyanm wrote:
> > On 2015/09/08 09:59:52, Thomas Greiner wrote:
> > > Why is there a "margin-top" and "margin-bottom"? `#language` doesn't have
> one
> > > either.
> > 
> > It's the case for smaller screens when the social media buttons wrap down
> under
> > the select box.
> 
> Ok, in that case "margin-top" should be sufficient.

No, because I'm aligning in the middle.

[Thomas Greiner, 2015-09-08 10:35:08]

LGTM