Issue 29370658: Issue 4462 - Update styles on Eyeo.com to match new logo (Closed)
DescriptionIssue 4462 - Update styles on Eyeo.com to match new logo
Download fonts here: https://drive.google.com/file/d/0B9dIRGeSpmZ_emRHbFpNQzZ5WW8/view?usp=sharing
COLLABORATOR=julian@adblockplus.org
COLLABORATOR=manvel@adblockplus.org
Patch Set 1 #
Total comments: 9
Patch Set 2 : See notes in patchset #
Total comments: 42
Patch Set 3 : See notes on last changeset #
MessagesTotal messages: 8
TLDR Not LGTM I think it would be better if we cleaned up this ticket and re-implemented so that we can get this out soon with the new images. https://codereview.adblockplus.org/29370658/diff/29370659/includes/index/ourw... File includes/index/ourwork-betterads.md (right): https://codereview.adblockplus.org/29370658/diff/29370659/includes/index/ourw... includes/index/ourwork-betterads.md:1: ####  #### - this shouldn't be a heading - drop the #### on the end https://codereview.adblockplus.org/29370658/diff/29370659/includes/index/ourw... includes/index/ourwork-betterads.md:3: ### BETTER ADS ### - Uppercase by CSS please - Drop the ### on the end https://codereview.adblockplus.org/29370658/diff/29370659/includes/index/ourw... includes/index/ourwork-betterads.md:5: We think online ads can be better. Our Acceptable Ads initiative aims to positively influence the web as a whole by giving all users an easy way to block unwanted elements, thus encouraging advertisers to follow guidelines for better advertising. [Read more](http://acceptableads.org/) The previous two comments apply to these files respectively. - includes/index/ourwork-betterads.md - includes/index/ourwork-opensource.md - includes/index/ourwork-privacy.md - includes/index/ourwork-security.md https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.c... static/css/styles.css:2: * Fonts We should prefer local("Source Sans Pro") over url("...") https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.c... static/css/styles.css:33: font-size: 17px; The issue says 18. Why 17? https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.c... static/css/styles.css:45: text-decoration: none; Please use spaces instead of tabs (everywhere). https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.c... static/css/styles.css:46: font-weight: 500; We don't have this font in 500. https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.c... static/css/styles.css:178: text-transform: uppercase; Please use 2 spaces (everywhere). https://codereview.adblockplus.org/29370658/diff/29370659/static/css/styles.c... static/css/styles.css:530: .four-columns .columns h1,h2,h3,h4,h5,h6 { - Please delete commented code blocks - This selector probably doesn't work like you expect (you have to re-start after the comma eg: .test h1, .test h2) - Please put additional selectors on separate lines
See notes for new patchset. I will delay fixing the NITs and IGNORES that I marked until you have completed your first round. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:11: src: url('../fonts/SourceSansPro-Light.eot') format('embedded-opentype'), NIT: I should prefer local(...) to url(...) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:48: html Note: I moved font and color to html because html is the ":root" element from which styles are inherited first. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:59: padding: 84px 0 0 0; Note: The header was larger when scrolled top in the screenshots. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:65: font-weight: 400; Note: There is no Source Sans Pro 500. 400 achieves the same effect when 300 is default. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:137: #header.top Note: This was a personal preference. (I removed the shadow on the header when scrolled to top) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:144: padding: 22px 10px; Note: This makes the header slightly larger when scrolled top. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:152: margin-top: -15px; IGNORE: These changes don't make sense. I will revert them in my next patchset. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:185: font-size: 16px; Note: Matching the screenshots. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:222: #header-hamburger:hover .icon-bar Note: Making the hamburger button the same color and not rounded like the others (the changes in the ticket alone broke this button). https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:321: font-size: 28px; Note: The ticket asked for 48px. This size was too big for small mobile devices and broke the line on the jobs pages. As a result, I changed it to 28px. I think this size looks good. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:347: border: 2px solid #161616; Note: Added a border by default (You can't see it because the background is the same color) to reserve the space (to prevent things from moving when you can see the border on hover as the background changes) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:364: background-color: #fff; Note: Removed text-decoration on hover (I think this makes sense) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:418: #our-work h3 Note: The text in these headers was uppercase in the screenshots https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:612: top: 1px; Note: I think 1px looks better when black. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:614: Note: I took a little creative license here. I made the hover/active vertical tab black instead of awkwardly pushing the other tabs down like it did before. I think this makes sense in the scope of this ticket because we were asked to make these tabs black and not rounded. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:865: #header-hamburger Note: Fixing broken hamburger (see comments above) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:896: .menu-button Note: Making a little more space so that everything fits @320px width https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:945: .ui-tabs-nav-vertical li.ui-tabs-active NIT: I should probably space out the rest of these.
Found a bug. Let me know if you find anything else! :) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:342: button, .button, .menu-button BUG: This change breaks the gray button on jobs pages.
https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:11: src: url('../fonts/SourceSansPro-Light.eot') format('embedded-opentype'), I'll suggest to use locale approach to prevent local fonts from being used: https://hg.adblockplus.org/web.adblockbrowser.org/file/tip/static/css/main.cs... For reference you can check Paul Irishi smiley approach: https://www.paulirish.com/2009/bulletproof-font-face-implementation-syntax/#s... https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:59: padding: 84px 0 0 0; Deatail: please declare units. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:152: margin-top: -15px; On 2017/01/11 14:22:46, juliandoucette wrote: > IGNORE: These changes don't make sense. I will revert them in my next patchset. Noted.
Notes from review meeting. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:11: src: url('../fonts/SourceSansPro-Light.eot') format('embedded-opentype'), On 2017/01/12 14:00:19, saroyanm wrote: > I'll suggest to use locale approach to prevent local fonts from being used: > https://hg.adblockplus.org/web.adblockbrowser.org/file/tip/static/css/main.cs... > > > For reference you can check Paul Irishi smiley approach: > https://www.paulirish.com/2009/bulletproof-font-face-implementation-syntax/#s... We will use the local font now and re-investigate this issue later. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:11: src: url('../fonts/SourceSansPro-Light.eot') format('embedded-opentype'), On 2017/01/12 14:00:19, saroyanm wrote: > I'll suggest to use locale approach to prevent local fonts from being used: > https://hg.adblockplus.org/web.adblockbrowser.org/file/tip/static/css/main.cs... > > > For reference you can check Paul Irishi smiley approach: > https://www.paulirish.com/2009/bulletproof-font-face-implementation-syntax/#s... Done. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:48: html On 2017/01/11 14:22:48, juliandoucette wrote: > Note: I moved font and color to html because html is the ":root" element from > which styles are inherited first. Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:59: padding: 84px 0 0 0; On 2017/01/12 14:00:19, saroyanm wrote: > Deatail: please declare units. Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:65: font-weight: 400; On 2017/01/11 14:22:46, juliandoucette wrote: > Note: There is no Source Sans Pro 500. 400 achieves the same effect when 300 is > default. Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:137: #header.top On 2017/01/11 14:22:48, juliandoucette wrote: > Note: This was a personal preference. (I removed the shadow on the header when > scrolled to top) Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:144: padding: 22px 10px; On 2017/01/11 14:22:47, juliandoucette wrote: > Note: This makes the header slightly larger when scrolled top. Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:152: margin-top: -15px; On 2017/01/11 14:22:46, juliandoucette wrote: > IGNORE: These changes don't make sense. I will revert them in my next patchset. Done. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:185: font-size: 16px; On 2017/01/11 14:22:49, juliandoucette wrote: > Note: Matching the screenshots. Acknowledged. There was a bug with overlapping logo and menu text. This was resolved by dropping the word "US" from the text. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:222: #header-hamburger:hover .icon-bar On 2017/01/11 14:22:49, juliandoucette wrote: > Note: Making the hamburger button the same color and not rounded like the others > (the changes in the ticket alone broke this button). Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:321: font-size: 28px; On 2017/01/11 14:22:47, juliandoucette wrote: > Note: The ticket asked for 48px. This size was too big for small mobile devices > and broke the line on the jobs pages. As a result, I changed it to 28px. I think > this size looks good. Acknowledged. We may check with Christiane after. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:342: button, .button, .menu-button On 2017/01/11 17:51:28, juliandoucette wrote: > BUG: This change breaks the gray button on jobs pages. Done. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:347: border: 2px solid #161616; On 2017/01/11 14:22:48, juliandoucette wrote: > Note: Added a border by default (You can't see it because the background is the > same color) to reserve the space (to prevent things from moving when you can see > the border on hover as the background changes) Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:364: background-color: #fff; On 2017/01/11 14:22:49, juliandoucette wrote: > Note: Removed text-decoration on hover (I think this makes sense) Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:418: #our-work h3 On 2017/01/11 14:22:47, juliandoucette wrote: > Note: The text in these headers was uppercase in the screenshots Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:612: top: 1px; On 2017/01/11 14:22:48, juliandoucette wrote: > Note: I think 1px looks better when black. Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:614: On 2017/01/11 14:22:48, juliandoucette wrote: > Note: I took a little creative license here. I made the hover/active vertical > tab black instead of awkwardly pushing the other tabs down like it did before. I > think this makes sense in the scope of this ticket because we were asked to make > these tabs black and not rounded. Acknowledged. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:865: #header-hamburger On 2017/01/11 14:22:48, juliandoucette wrote: > Note: Fixing broken hamburger (see comments above) Done. Fixed bug between Firefox and Chrome (padding) https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:896: .menu-button On 2017/01/11 14:22:47, juliandoucette wrote: > Note: Making a little more space so that everything fits @320px width Done. https://codereview.adblockplus.org/29370658/diff/29371005/static/css/styles.c... static/css/styles.css:945: .ui-tabs-nav-vertical li.ui-tabs-active On 2017/01/11 14:22:47, juliandoucette wrote: > NIT: I should probably space out the rest of these. Acknowledged.
LGTM
On 2017/01/12 15:41:37, saroyanm wrote: > LGTM https://hg.adblockplus.org/web.eyeo.com/rev/cdaad9ab73ed 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
