Issue 4461 - update Eyeo logo

COLLABORATOR=julian@adblockplus.org

Info on initial patch:
* I've changed the size of the original images and made style changes, so the buttons will not overlap the logo
* processed images through pngout

[saroyanm, 2016-09-22 12:45:40]

Julian can you have a look please.

[juliandoucette, 2016-09-26 14:50:50]

On 2016/09/22 12:45:40, saroyanm wrote:
> Julian can you have a look please.

The logo is not vertically centered.

You can accomplish this easily using absolute positioning by:

#logo
{
  position: absolute;
  top: 50%;
  left: 10px;
  margin-top: -20px;
  height: 40px;
}

[saroyanm, 2016-09-27 13:21:16]

On 2016/09/26 14:50:50, juliandoucette wrote:
> On 2016/09/22 12:45:40, saroyanm wrote:
> > Julian can you have a look please.
> 
> The logo is not vertically centered.
> 
> You can accomplish this easily using absolute positioning by:
> 
> #logo
> {
>   position: absolute;
>   top: 50%;
>   left: 10px;
>   margin-top: -20px;
>   height: 40px;
> }

Done.

[juliandoucette, 2016-09-27 13:44:56]

See comments below.

https://codereview.adblockplus.org/29354725/diff/29355183/static/css/styles.css
File static/css/styles.css (right):

https://codereview.adblockplus.org/29354725/diff/29355183/static/css/styles.c...
static/css/styles.css:822: #logo > img
It's not centered properly on mobile (probably a side effect of the last
change).

I suggest we:

/* set the height in CSS instead of HTML */

#logo
{
  position: absolute;
  left: 10px;
  top: 50%;
  margin-top: -20px;
  height: 40px;
}

/* set the height of the #logo instead of the #logo > img */

#logo img
{
  height: 100%;
}

/* change the height on mobile to 30px */

@media(max-width: 768px)
{
  #logo
  {
    height: 30px;
    margin-top: -15px;
  }
}

PS: This would be easier if I could submit a patchset.

[saroyanm, 2016-09-27 13:47:53]

On 2016/09/27 13:44:56, juliandoucette wrote:
> See comments below.
> 
>
https://codereview.adblockplus.org/29354725/diff/29355183/static/css/styles.css
> File static/css/styles.css (right):
> 
>
https://codereview.adblockplus.org/29354725/diff/29355183/static/css/styles.c...
> static/css/styles.css:822: #logo > img
> It's not centered properly on mobile (probably a side effect of the last
> change).
> 
> I suggest we:
> 
> /* set the height in CSS instead of HTML */
> 
> #logo
> {
>   position: absolute;
>   left: 10px;
>   top: 50%;
>   margin-top: -20px;
>   height: 40px;
> }
> 
> /* set the height of the #logo instead of the #logo > img */
> 
> #logo img
> {
>   height: 100%;
> }
> 
> /* change the height on mobile to 30px */
> 
> @media(max-width: 768px)
> {
>   #logo
>   {
>     height: 30px;
>     margin-top: -15px;
>   }
> }
> 
> PS: This would be easier if I could submit a patchset.

Added you as a COLLABORATOR feel free to update

[juliandoucette, 2016-09-27 13:55:51]

Done.

Please review.

https://codereview.adblockplus.org/29354725/diff/29355183/static/css/styles.css
File static/css/styles.css (right):

https://codereview.adblockplus.org/29354725/diff/29355183/static/css/styles.c...
static/css/styles.css:822: #logo > img
On 2016/09/27 13:44:56, juliandoucette wrote:
> It's not centered properly on mobile (probably a side effect of the last
> change).
> 
> I suggest we:
> 
> /* set the height in CSS instead of HTML */
> 
> #logo
> {
>   position: absolute;
>   left: 10px;
>   top: 50%;
>   margin-top: -20px;
>   height: 40px;
> }
> 
> /* set the height of the #logo instead of the #logo > img */
> 
> #logo img
> {
>   height: 100%;
> }
> 
> /* change the height on mobile to 30px */
> 
> @media(max-width: 768px)
> {
>   #logo
>   {
>     height: 30px;
>     margin-top: -15px;
>   }
> }
> 
> PS: This would be easier if I could submit a patchset.

Done.

(Also removed #logo.top edge case bug on mobile.)

[saroyanm, 2016-09-27 14:01:56]

https://codereview.adblockplus.org/29354725/diff/29355197/static/css/styles.css
File static/css/styles.css (right):

https://codereview.adblockplus.org/29354725/diff/29355197/static/css/styles.c...
static/css/styles.css:825: height: 30px;
This change causes the logo to be overlaped by button on Iphone5

[saroyanm, 2016-09-27 14:04:32]

https://codereview.adblockplus.org/29354725/diff/29355197/static/css/styles.css
File static/css/styles.css (right):

https://codereview.adblockplus.org/29354725/diff/29355197/static/css/styles.c...
static/css/styles.css:825: height: 30px;
On 2016/09/27 14:01:56, saroyanm wrote:
> This change causes the logo to be overlaped by button on Iphone5

height: 22px;
margin-top: -11px;

Should fix the issue, I'm not sure how we need to collaborate on same project,
should I upload patch instead of commenting ?

[juliandoucette, 2016-09-27 14:09:03]

> > This change causes the logo to be overlaped by button on Iphone5
> 
> height: 22px;
> margin-top: -11px;
> 
> Should fix the issue, I'm not sure how we need to collaborate on same project,
> should I upload patch instead of commenting ?

Your choice.

- Whatever is faster for you
- Whatever makes more sense according to your priorities

I added you as a reviewer. We should both approve a change that we have both
contributed too.

[juliandoucette, 2016-09-27 14:11:12]

> I added you as a reviewer. We should both approve a change that we have both
contributed too.

I mean we should both approve the final changeset before pushing.

Looks like we also broke alignment when you expand the mobile menu. Uh-oh.

[saroyanm, 2016-09-27 14:14:51]

On 2016/09/27 14:11:12, juliandoucette wrote:
> > I added you as a reviewer. We should both approve a change that we have both
> contributed too.
> 
> I mean we should both approve the final changeset before pushing.
> 
> Looks like we also broke alignment when you expand the mobile menu. Uh-oh.

That's true, I'll suggest to go with the initial solution, I intended to make it
same way as it works correctly, probably introduction of additional magic break
the layout.

[juliandoucette, 2016-09-27 14:18:51]

> That's true, I'll suggest to go with the initial solution, I intended to make
it
> same way as it works correctly, probably introduction of additional magic
break
> the layout.

Fine with me.

Just fix the alignment when the logo is not .top on desktop please.

[saroyanm, 2016-09-27 14:29:00]

On 2016/09/27 14:18:51, juliandoucette wrote:
> > That's true, I'll suggest to go with the initial solution, I intended to
make
> it
> > same way as it works correctly, probably introduction of additional magic
> break
> > the layout.
> 
> Fine with me.
> 
> Just fix the alignment when the logo is not .top on desktop please.

Done.

[juliandoucette, 2016-09-27 14:30:37]

> Done.

Not done?

It's still not centered when you scroll down on desktop.

[saroyanm, 2016-09-27 14:49:23]

On 2016/09/27 14:30:37, juliandoucette wrote:
> > Done.
> 
> Not done?
> 
> It's still not centered when you scroll down on desktop.

Oh crap! Sorry for that, I thought for some reason you were talking about the
mobile version, should be done now, if I got you right :)

[juliandoucette, 2016-09-27 15:11:24]

> Oh crap! Sorry for that, I thought for some reason you were talking about the
> mobile version, should be done now, if I got you right :)

Yep :),

But now you broke it on mobile when you scroll down :D

[juliandoucette, 2016-09-27 19:33:22]

> Yep :),
> 
> But now you broke it on mobile when you scroll down :D

Done.

Please review.

[saroyanm, 2016-09-28 15:44:14]

On 2016/09/27 19:33:22, juliandoucette wrote:
> > Yep :),
> > 
> > But now you broke it on mobile when you scroll down :D
> 
> Done.
> 
> Please review.

LGTM

[saroyanm, 2016-09-28 15:44:29]

LGTM

[saroyanm, 2016-09-28 15:49:36]

I guess I can't LGTM the review created by me :)

[juliandoucette, 2016-09-29 11:12:05]

On 2016/09/28 15:49:36, saroyanm wrote:
> I guess I can't LGTM the review created by me :)

Haha, LGTM.