Issue 29335113: Issue 2675 - Implemented responsive header for web.eyeo.com (Closed)
|
Created:
Jan. 29, 2016, 5:25 p.m. by juliandoucette Modified:
July 14, 2016, 5:25 p.m. Reviewers:
saroyanm CC:
Thomas Greiner, Sebastian Noack Visibility:
Public. |
DescriptionIssue 2675 - Implemented responsive header for web.eyeo.com
Patch Set 1 #Patch Set 2 : Removed extra whitespace #Patch Set 3 : Made join us button smaller on phones #Patch Set 4 : Minor code style and variable scope cleanup #
Total comments: 32
Patch Set 5 : Removed phablet media query #Patch Set 6 : Fixed html formatting, script links, and assistive text. #Patch Set 7 : Implemented scripts in vanilla.js #
Total comments: 40
Patch Set 8 : Fixed logo, floats, css style, and IE8 events #Patch Set 9 : Fixed logo in form and hero banner spacing #Patch Set 10 : Fixed minor css style issues #
Total comments: 22
Patch Set 11 : Fixed code style and consistancy issues #
MessagesTotal messages: 18
On 2016/01/29 17:42:21, juliandoucette wrote: Updated based on Christine's comment. https://issues.adblockplus.org/ticket/2675#comment:15
On 2016/02/23 15:23:19, juliandoucette wrote: > On 2016/01/29 17:42:21, juliandoucette wrote: > > Updated based on Christine's comment. > > https://issues.adblockplus.org/ticket/2675#comment:15 Added minor update that cleans up minor code style issues.
Sorry Julian that it takes so long for review, I'll try to finish with others ASAP. Also two things that I couldn't address in the comments: 1. We usually using pngout for the images in case you didn't used pngout on new image would be greate if can process it through. 2. I've noticed that we are using several elements in the header "assistive-text" I can't see if we are using that elements anywhere in the eyeo.com, I think we can remove them while they are not used. https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:121: float: left; Why do we need to use float ? https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:846: @media(max-width: 660px) Why do we need two media queries ? I think would be great to have only one, maybe we can use percentages for logo for smaller screens or just go with small size implementation ? Because as much we will introduce media queries as much maintenance effort we will introduce for future. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:1: jQuery(function() Note: I think if we could use Javascript instead of Jquery we will be able to get rid of Jquery from some point, because it's only website we are using Jquery and lots of Jquery based suspicious libraries, but I'll let that up to you, while that can be done in separate review as well. Also I'm not sure if the animations provide that much value. Maybe make sense also discuss the scroll animation removal with Designers before doing that, but I assume that will save a lot of headache in future if we can get rid of Jquery. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); Why to use dollar sign in front of the variable names ? https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); The element is missing, it was removed. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:41: if ($menu.hasClass("open")) { Detail: please move opening curly brace on it's own line. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:20: <script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script> I think you would like to get both files from our server. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:38: <a While we do have 80 line limit for coding style we didn't used that rule for HTML pages yet, so I'll suggest not to apply that rule for html yet, also it will make this review easier.
No worries Manvel. And sorry it took so long to push back on this. My priorities were shifted. Regarding: 1. I'm not familiar with pngout. What exactly do you want me to do? 2. The assistive text in the header navigation would be sufficient for the entire website (we don't need to put it elsewhere). But, I can remove it, if you insist. I thought it made sense because I implemented the header navigation menu in an accessible way. https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:121: float: left; On 2016/04/08 16:56:09, saroyanm wrote: > Why do we need to use float ? Because I don't want it to overlap. - absolute/fixed would overlap - inline block would require a hacky container/separator - floats stack when they run into each other https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:846: @media(max-width: 660px) On 2016/04/08 16:56:09, saroyanm wrote: > Why do we need two media queries ? > I think would be great to have only one, maybe we can use percentages for logo > for smaller screens or just go with small size implementation ? > Because as much we will introduce media queries as much maintenance effort we > will introduce for future. This media query does have an admittedly small use case... I will remove it. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:1: jQuery(function() On 2016/04/08 16:56:10, saroyanm wrote: > Note: I think if we could use Javascript instead of Jquery we will be able to > get rid of Jquery from some point, because it's only website we are using Jquery > and lots of Jquery based suspicious libraries, but I'll let that up to you, > while that can be done in separate review as well. > Also I'm not sure if the animations provide that much value. > Maybe make sense also discuss the scroll animation removal with Designers before > doing that, but I assume that will save a lot of headache in future if we can > get rid of Jquery. - I agree that we should get rid of jquery - But I think that is outside of the scope of this review - I also think that we should remove/change the animation - But I think that is outside of the scope of this review too (I intentionally kept it for that reason) Since you are the module owner, please agree and make new issues for these, or disagree in response to this comment. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); On 2016/04/08 16:56:09, saroyanm wrote: > Why to use dollar sign in front of the variable names ? This is a standard practice used to identify a value that is an instance of jQuery. ($() returns an instance of jQuery) https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); On 2016/04/08 16:56:10, saroyanm wrote: > The element is missing, it was removed. Good point :D . I didn't notice that. I think we should leave it out. (I know, that is inconsistent with _staying in scope_ above.) What do you think? https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:41: if ($menu.hasClass("open")) { On 2016/04/08 16:56:09, saroyanm wrote: > Detail: please move opening curly brace on it's own line. Nice catch. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:20: <script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script> On 2016/04/08 16:56:10, saroyanm wrote: > I think you would like to get both files from our server. Good catch. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:38: <a On 2016/04/08 16:56:10, saroyanm wrote: > While we do have 80 line limit for coding style we didn't used that rule for > HTML pages yet, so I'll suggest not to apply that rule for html yet, also it > will make this review easier. Sorry, I thought it was easier to read this way.
https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:121: float: left; On 2016/04/25 19:08:55, juliandoucette wrote: > On 2016/04/08 16:56:09, saroyanm wrote: > > Why do we need to use float ? > > Because I don't want it to overlap. > > - absolute/fixed would overlap > - inline block would require a hacky container/separator > - floats stack when they run into each other Clarification: I'm talking about overlapping when the screen gets too small to contain both elements side by side.
https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:121: float: left; On 2016/04/25 19:08:55, juliandoucette wrote: > On 2016/04/08 16:56:09, saroyanm wrote: > > Why do we need to use float ? > > Because I don't want it to overlap. > > - absolute/fixed would overlap > - inline block would require a hacky container/separator > - floats stack when they run into each other I think you can alternatively have logo positioned absolutely and have left margin equal to the size of the logo so you will not have overlap. Ex.: <logo positioned absolute width=100px></logo> <nav width=100% text-align=right> <inline block margin-left=100px></inline block> <inline block></inline block> <inline block></inline block> </nav> What you think ? https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:1: jQuery(function() On 2016/04/25 19:08:56, juliandoucette wrote: > On 2016/04/08 16:56:10, saroyanm wrote: > > Note: I think if we could use Javascript instead of Jquery we will be able to > > get rid of Jquery from some point, because it's only website we are using > Jquery > > and lots of Jquery based suspicious libraries, but I'll let that up to you, > > while that can be done in separate review as well. > > Also I'm not sure if the animations provide that much value. > > Maybe make sense also discuss the scroll animation removal with Designers > before > > doing that, but I assume that will save a lot of headache in future if we can > > get rid of Jquery. > > - I agree that we should get rid of jquery > - But I think that is outside of the scope of this review > - I also think that we should remove/change the animation > - But I think that is outside of the scope of this review too (I intentionally > kept it for that reason) > > Since you are the module owner, please agree and make new issues for these, or > disagree in response to this comment. I think if we will decide to get rid of animation function we can also implement Javascript solution in this review while most of the page is affected and I think it should be easy to use Javascript while we only have few jquery script and the only tricky is animation also if we will consider to remote the to-top element as you suggested I will really be in favor to have Javascript in current review, otherwise we will implement something and reimpolement that in Javascript in separate review I think having it in this review is justified in this case. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); On 2016/04/25 19:08:56, juliandoucette wrote: > On 2016/04/08 16:56:09, saroyanm wrote: > > Why to use dollar sign in front of the variable names ? > > This is a standard practice used to identify a value that is an instance of > jQuery. > > ($() returns an instance of jQuery) Acknowledged. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); On 2016/04/25 19:08:56, juliandoucette wrote: > On 2016/04/08 16:56:10, saroyanm wrote: > > The element is missing, it was removed. > > Good point :D . I didn't notice that. > > I think we should leave it out. (I know, that is inconsistent with _staying in > scope_ above.) What do you think? I agree I'm not sure if it provides much value, but please remove all related code as well. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:39: jQuery('#header-hamberger').click(function() Detail: it's "hamburger" also please use double quote '"' sign for consistency. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:38: <a On 2016/04/25 19:08:56, juliandoucette wrote: > On 2016/04/08 16:56:10, saroyanm wrote: > > While we do have 80 line limit for coding style we didn't used that rule for > > HTML pages yet, so I'll suggest not to apply that rule for html yet, also it > > will make this review easier. > > Sorry, I thought it was easier to read this way. Currently how it's done it's inconsistent, you have elements that doesn't exceed 80 and you move their attributes to new line, while there are elements which attributes are not moved to new line, I think we need to come up with some general rules for that, I'll suggest leave it as it was before for this review and come up with some general rules to improve coding standard for HTML. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:46: <a I can't understand why we have this element and the one below. Does they provide any value ?
https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:121: float: left; On 2016/05/06 14:49:27, saroyanm wrote: > On 2016/04/25 19:08:55, juliandoucette wrote: > > On 2016/04/08 16:56:09, saroyanm wrote: > > > Why do we need to use float ? > > > > Because I don't want it to overlap. > > > > - absolute/fixed would overlap > > - inline block would require a hacky container/separator > > - floats stack when they run into each other > > I think you can alternatively have logo positioned absolutely and have left > margin equal to the size of the logo so you will not have overlap. Ex.: > > <logo positioned absolute width=100px></logo> > <nav width=100% text-align=right> > <inline block margin-left=100px></inline block> > <inline block></inline block> > <inline block></inline block> > </nav> > aligned logo and menu. > What you think ? It would be better to set a minimum width on the container to prevent overlapping. Absolute positioning is great for positioning blocks in absolute positions. Floating is great for positioning blocks inline left or right. I intended to position these blocks inline inside the header left and right. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:1: jQuery(function() On 2016/05/06 14:49:28, saroyanm wrote: > On 2016/04/25 19:08:56, juliandoucette wrote: > > On 2016/04/08 16:56:10, saroyanm wrote: > > > Note: I think if we could use Javascript instead of Jquery we will be able > to > > > get rid of Jquery from some point, because it's only website we are using > > Jquery > > > and lots of Jquery based suspicious libraries, but I'll let that up to you, > > > while that can be done in separate review as well. > > > Also I'm not sure if the animations provide that much value. > > > Maybe make sense also discuss the scroll animation removal with Designers > > before > > > doing that, but I assume that will save a lot of headache in future if we > can > > > get rid of Jquery. > > > > - I agree that we should get rid of jquery > > - But I think that is outside of the scope of this review > > - I also think that we should remove/change the animation > > - But I think that is outside of the scope of this review too (I intentionally > > kept it for that reason) > > > > Since you are the module owner, please agree and make new issues for these, or > > disagree in response to this comment. > > I think if we will decide to get rid of animation function we can also implement > Javascript solution in this review while most of the page is affected and I > think it should be easy to use Javascript while we only have few jquery script > and the only tricky is animation also if we will consider to remote the to-top > element as you suggested I will really be in favor to have Javascript in current > review, otherwise we will implement something and reimpolement that in > Javascript in separate review I think having it in this review is justified in > this case. Ok, I will implement this without jQuery. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:3: var $toTop = jQuery("#to-top"); On 2016/05/06 14:49:28, saroyanm wrote: > On 2016/04/25 19:08:56, juliandoucette wrote: > > On 2016/04/08 16:56:10, saroyanm wrote: > > > The element is missing, it was removed. > > > > Good point :D . I didn't notice that. > > > > I think we should leave it out. (I know, that is inconsistent with _staying in > > scope_ above.) What do you think? > > I agree I'm not sure if it provides much value, but please remove all related > code as well. Will do. https://codereview.adblockplus.org/29335113/diff/29337819/static/js/scripts.j... static/js/scripts.js:39: jQuery('#header-hamberger').click(function() On 2016/05/06 14:49:28, saroyanm wrote: > Detail: it's "hamburger" also please use double quote '"' sign for consistency. Nice catch :) https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:38: <a On 2016/05/06 14:49:28, saroyanm wrote: > On 2016/04/25 19:08:56, juliandoucette wrote: > > On 2016/04/08 16:56:10, saroyanm wrote: > > > While we do have 80 line limit for coding style we didn't used that rule for > > > HTML pages yet, so I'll suggest not to apply that rule for html yet, also it > > > will make this review easier. > > > > Sorry, I thought it was easier to read this way. > > Currently how it's done it's inconsistent, you have elements that doesn't exceed > 80 and you move their attributes to new line, while there are elements which > attributes are not moved to new line, I think we need to come up with some > general rules for that, I'll suggest leave it as it was before for this review > and come up with some general rules to improve coding standard for HTML. Agreed. Will do. https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:46: <a On 2016/05/06 14:49:28, saroyanm wrote: > I can't understand why we have this element and the one below. Does they provide > any value ? Providing a "skip to content" link is a standard accessibility practice. However, our template does not have a #beginning-of-content or #secondary content area. I will remove these.
https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29337819/templates/default.t... templates/default.tmpl:46: <a On 2016/05/11 18:11:06, juliandoucette wrote: > On 2016/05/06 14:49:28, saroyanm wrote: > > I can't understand why we have this element and the one below. Does they > provide > > any value ? > > Providing a "skip to content" link is a standard accessibility practice. > > However, our template does not have a #beginning-of-content or #secondary > content area. > > I will remove these. Scratch that. I actually just removed one and fixed the other to link to #content. Let me know if you approve :) .
https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29337819/static/css/styles.c... static/css/styles.css:121: float: left; On 2016/05/11 18:11:06, juliandoucette wrote: > On 2016/05/06 14:49:27, saroyanm wrote: > > On 2016/04/25 19:08:55, juliandoucette wrote: > > > On 2016/04/08 16:56:09, saroyanm wrote: > > > > Why do we need to use float ? > > > > > > Because I don't want it to overlap. > > > > > > - absolute/fixed would overlap > > > - inline block would require a hacky container/separator > > > - floats stack when they run into each other > > > > I think you can alternatively have logo positioned absolutely and have left > > margin equal to the size of the logo so you will not have overlap. Ex.: > > > > <logo positioned absolute width=100px></logo> > > <nav width=100% text-align=right> > > <inline block margin-left=100px></inline block> > > <inline block></inline block> > > <inline block></inline block> > > </nav> > > aligned logo and menu. > > What you think ? > > It would be better to set a minimum width on the container to prevent > overlapping. > > Absolute positioning is great for positioning blocks in absolute positions. > > Floating is great for positioning blocks inline left or right. > > I intended to position these blocks inline inside the header left and right. You are right setting min-width is looks to be a better solution, currently we are using bunch of floats and it's bit chaotic now and you are right absolute positioning is also not a good solution for this case, but I would argue that Float are not meant for layouting and they only needs to be used when there is no alternative for layouting IMO if we can avoid floating that would be great. "The float CSS property specifies that an element should be taken from the normal flow and placed along the left or right side of its container, where text and inline elements will wrap around it" So while it's possible to achieve result without using float that will be great and if we can use floats only for the reason they are meant for. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:116: padding: 8px 0; Detail: "CSS number values should specify units where possible" according to the style guide, same goes to the changes below https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:176: #header-hamberger Detail: It's "hamburger", same goes to the styles below https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:181: #header-hamberger:hover, It feels like that this styles are redundant https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:190: .icon-bar { Maybe you can modify this styles to make it obvious that the menu is clickable, by using at least :hover https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:190: .icon-bar { Detail: According to the style guide "opening braces always go on their own line" Same for the styles below. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:192: display: block; Detail: According to our style guide colors and typography goes after "Display" property and "Box model": https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... Same applies to other changes on this page, please use property ordering specified in the styleguide https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:895: margin: 0 -1px .2em 0; Is the change a leftover ? https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:1: document.addEventListener("DOMContentLoaded", function() addEventListener is not supported in older versions of IE: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener... https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:6: var headerPadding = 13; Detail: you are using the value only in 1 place so no need to assign it to the variable. https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:23: var menu = document.getElementById("menu"); Detail: you can get the menu element using the "header" above instead of document. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:39: <img src="/images/tog_logo.png" height="51px" alt="Eyeo GmbH" /> Detail: I assume we had misspelled beforehand, top_logo ? https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:46: <button type="button" id="header-hamberger" aria-expanded="false"> Hamburger also should belong to the navigation as well as the "skip to content button" and "join us" button. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:62: <li class="menu-item menu-item-type-post_type menu-item-object-page current-menu-item page_item current_page_item"> Feels like only "current-menu-item" class is being used.
> You are right setting min-width is looks to be a better solution, currently we > are using bunch of floats and it's bit chaotic now and you are right absolute > positioning is also not a good solution for this case, but I would argue that > Float are not meant for layouting and they only needs to be used when there is > no alternative for layouting IMO if we can avoid floating that would be great. > "The float CSS property specifies that an element should be taken from the > normal flow and placed along the left or right side of its container, where text > and inline elements will wrap around it" > So while it's possible to achieve result without using float that will be great > and if we can use floats only for the reason they are meant for. Will do.
https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:116: padding: 8px 0; On 2016/05/19 07:43:39, saroyanm wrote: > Detail: "CSS number values should specify units where possible" according to the > style guide, same goes to the changes below Good catch. Thanks. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:176: #header-hamberger On 2016/05/19 07:43:40, saroyanm wrote: > Detail: It's "hamburger", same goes to the styles below Haha, the magic of auto-complete. (Persistent typos). Good catch. Thanks :). https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:181: #header-hamberger:hover, On 2016/05/19 07:43:40, saroyanm wrote: > It feels like that this styles are redundant I feel you :(. I'm killing the default button styles in the browser. Maybe I should just use an anchor instead of a button. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:190: .icon-bar { On 2016/05/19 07:43:40, saroyanm wrote: > Maybe you can modify this styles to make it obvious that the menu is clickable, > by using at least :hover Will do. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:190: .icon-bar { On 2016/05/19 07:43:40, saroyanm wrote: > Detail: According to the style guide "opening braces always go on their own > line" > > Same for the styles below. My mistake. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:192: display: block; On 2016/05/19 07:43:40, saroyanm wrote: > Detail: According to our style guide colors and typography goes after "Display" > property and "Box model": > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... > > Same applies to other changes on this page, please use property ordering > specified in the styleguide My mistake. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:895: margin: 0 -1px .2em 0; On 2016/05/19 07:43:40, saroyanm wrote: > Is the change a leftover ? I changed the order to match the style guide. https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:1: document.addEventListener("DOMContentLoaded", function() On 2016/05/19 07:43:41, saroyanm wrote: > addEventListener is not supported in older versions of IE: > https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener... > My mistake. Will fix. https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:6: var headerPadding = 13; On 2016/05/19 07:43:41, saroyanm wrote: > Detail: you are using the value only in 1 place so no need to assign it to the > variable. For readability? (To give the literal value meaning in the code) https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:23: var menu = document.getElementById("menu"); On 2016/05/19 07:43:40, saroyanm wrote: > Detail: you can get the menu element using the "header" above instead of > document. Yes. But that is a preference. document.getElementById is the fastest, best supported, and most widely used method for this purpose. element.querySelector is also appropriate, although a little slower, and not supported < IE8. I could also loop the header children for element.id === 'menu'. Sacrificing a little readability and maintainability (if we later moved the element for some reason). I would recommend document.getElementById for this purpose. What do you think? https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:39: <img src="/images/tog_logo.png" height="51px" alt="Eyeo GmbH" /> On 2016/05/19 07:43:41, saroyanm wrote: > Detail: I assume we had misspelled beforehand, top_logo ? Yes. I'll correct it though. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:46: <button type="button" id="header-hamberger" aria-expanded="false"> On 2016/05/19 07:43:41, saroyanm wrote: > Hamburger also should belong to the navigation as well as the "skip to content > button" and "join us" button. About the _skip to content_ link: I disagree because: - This placement is standard (after the first header, outside of any other navigation) - The _skip to content_ link is not related to the other links in the header nav About the _hamburger_ button: I disagree because: - This placement is standard (hidden by default, above the nav, linked by aria) - The _Toggle navigation_ link is not related to the other links in the header nav More explanation: The _skip to content_ and _toggle menu_ links are not like _section content links_ that may exist inside a _section nav_. They are purely *functional*, mostly hidden, and not *content* (like the header nav links). Therefore, we should not group them with other *content links*, because we do not want them to appear like other *content links* in the **document outline** (or the tab index). About the _join us_ button: Yes, the _join us_ button is related. But, because we chose to separate it (design), and put it outside of the main nav (visually), it is important that we also communicate that semantically (markup) for accessibility. If you were to describe the content visually, you would describe the link as being part of the header, and not the nav menu that is expanded / contracted (when on a small screen). You would also describe it as appearing differently and after the nav menu links (when on a large screen). In terms of SEO, because it is within the first header, it is always displayed, and it is not a #section link: it should be weighted appropriately. However, it should also appear in the footer or provided sitemap like a main nav link. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:62: <li class="menu-item menu-item-type-post_type menu-item-object-page current-menu-item page_item current_page_item"> On 2016/05/19 07:43:41, saroyanm wrote: > Feels like only "current-menu-item" class is being used. A WordPress artifact, no doubt. I'll check and remove the unused classes then.
https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:895: margin: 0 -1px .2em 0; On 2016/05/19 12:01:41, juliandoucette wrote: > On 2016/05/19 07:43:40, saroyanm wrote: > > Is the change a leftover ? > > I changed the order to match the style guide. Borders and margins are part of box model. I think "clear" can go below. https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:6: var headerPadding = 13; On 2016/05/19 12:01:45, juliandoucette wrote: > On 2016/05/19 07:43:41, saroyanm wrote: > > Detail: you are using the value only in 1 place so no need to assign it to the > > variable. > > For readability? (To give the literal value meaning in the code) Acknowledged. https://codereview.adblockplus.org/29335113/diff/29341294/static/js/scripts.j... static/js/scripts.js:23: var menu = document.getElementById("menu"); On 2016/05/19 12:01:45, juliandoucette wrote: > On 2016/05/19 07:43:40, saroyanm wrote: > > Detail: you can get the menu element using the "header" above instead of > > document. > > Yes. But that is a preference. > > document.getElementById is the fastest, best supported, and most widely used > method for this purpose. > > element.querySelector is also appropriate, although a little slower, and not > supported < IE8. > > I could also loop the header children for element.id === 'menu'. Sacrificing a > little readability and maintainability (if we later moved the element for some > reason). > > I would recommend document.getElementById for this purpose. What do you think? I agree with you https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:39: <img src="/images/tog_logo.png" height="51px" alt="Eyeo GmbH" /> On 2016/05/19 12:01:45, juliandoucette wrote: > On 2016/05/19 07:43:41, saroyanm wrote: > > Detail: I assume we had misspelled beforehand, top_logo ? > > Yes. > > I'll correct it though. I made a comment regarding high DPI screens here: https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n... Users shouldn't download bigger image in case they do not have high DPI screens. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:46: <button type="button" id="header-hamberger" aria-expanded="false"> On 2016/05/19 12:01:45, juliandoucette wrote: > On 2016/05/19 07:43:41, saroyanm wrote: > > Hamburger also should belong to the navigation as well as the "skip to content > > button" and "join us" button. > > About the _skip to content_ link: > > I disagree because: > > - This placement is standard (after the first header, outside of any other > navigation) > - The _skip to content_ link is not related to the other links in the header nav > > About the _hamburger_ button: > > I disagree because: > > - This placement is standard (hidden by default, above the nav, linked by aria) > - The _Toggle navigation_ link is not related to the other links in the header > nav > > More explanation: > > The _skip to content_ and _toggle menu_ links are not like _section content > links_ that may exist inside a _section nav_. They are purely *functional*, > mostly hidden, and not *content* (like the header nav links). Therefore, we > should not group them with other *content links*, because we do not want them to > appear like other *content links* in the **document outline** (or the tab > index). > > About the _join us_ button: > > Yes, the _join us_ button is related. But, because we chose to separate it > (design), and put it outside of the main nav (visually), it is important that we > also communicate that semantically (markup) for accessibility. If you were to > describe the content visually, you would describe the link as being part of the > header, and not the nav menu that is expanded / contracted (when on a small > screen). You would also describe it as appearing differently and after the nav > menu links (when on a large screen). > > In terms of SEO, because it is within the first header, it is always displayed, > and it is not a #section link: it should be weighted appropriately. However, it > should also appear in the footer or provided sitemap like a main nav link. Thanks for detailed explanation I think you are right.
https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:116: padding: 8px 0; On 2016/05/19 12:01:41, juliandoucette wrote: > On 2016/05/19 07:43:39, saroyanm wrote: > > Detail: "CSS number values should specify units where possible" according to > the > > style guide, same goes to the changes below > > Good catch. Thanks. Done. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:176: #header-hamberger On 2016/05/19 12:01:44, juliandoucette wrote: > On 2016/05/19 07:43:40, saroyanm wrote: > > Detail: It's "hamburger", same goes to the styles below > > Haha, the magic of auto-complete. (Persistent typos). > > Good catch. Thanks :). Done. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:181: #header-hamberger:hover, On 2016/05/19 12:01:45, juliandoucette wrote: > On 2016/05/19 07:43:40, saroyanm wrote: > > It feels like that this styles are redundant > > I feel you :(. I'm killing the default button styles in the browser. Maybe I > should just use an anchor instead of a button. Done. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:190: .icon-bar { On 2016/05/19 12:01:44, juliandoucette wrote: > On 2016/05/19 07:43:40, saroyanm wrote: > > Maybe you can modify this styles to make it obvious that the menu is > clickable, > > by using at least :hover > > Will do. Done. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:192: display: block; On 2016/05/19 12:01:41, juliandoucette wrote: > On 2016/05/19 07:43:40, saroyanm wrote: > > Detail: According to our style guide colors and typography goes after > "Display" > > property and "Box model": > > > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... > > > > Same applies to other changes on this page, please use property ordering > > specified in the styleguide > > My mistake. Done. https://codereview.adblockplus.org/29335113/diff/29341294/static/css/styles.c... static/css/styles.css:895: margin: 0 -1px .2em 0; On 2016/05/23 12:39:20, saroyanm wrote: > On 2016/05/19 12:01:41, juliandoucette wrote: > > On 2016/05/19 07:43:40, saroyanm wrote: > > > Is the change a leftover ? > > > > I changed the order to match the style guide. > > Borders and margins are part of box model. I think "clear" can go below. Done. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:39: <img src="/images/tog_logo.png" height="51px" alt="Eyeo GmbH" /> On 2016/05/23 12:39:21, saroyanm wrote: > On 2016/05/19 12:01:45, juliandoucette wrote: > > On 2016/05/19 07:43:41, saroyanm wrote: > > > Detail: I assume we had misspelled beforehand, top_logo ? > > > > Yes. > > > > I'll correct it though. > > I made a comment regarding high DPI screens here: > https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n... > > Users shouldn't download bigger image in case they do not have high DPI screens. Done. I added a srcset attribute that specifies an image for 1x and 2x. Browsers that do not support srcset will fall back to src (1x). https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:62: <li class="menu-item menu-item-type-post_type menu-item-object-page current-menu-item page_item current_page_item"> On 2016/05/19 12:01:45, juliandoucette wrote: > On 2016/05/19 07:43:41, saroyanm wrote: > > Feels like only "current-menu-item" class is being used. > > A WordPress artifact, no doubt. > > I'll check and remove the unused classes then. Done.
Mostly looks good, I made only some coding style consistency changes, we are close here as well. https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29341294/templates/default.t... templates/default.tmpl:39: <img src="/images/tog_logo.png" height="51px" alt="Eyeo GmbH" /> On 2016/06/16 18:07:45, juliandoucette wrote: > On 2016/05/23 12:39:21, saroyanm wrote: > > On 2016/05/19 12:01:45, juliandoucette wrote: > > > On 2016/05/19 07:43:41, saroyanm wrote: > > > > Detail: I assume we had misspelled beforehand, top_logo ? > > > > > > Yes. > > > > > > I'll correct it though. > > > > I made a comment regarding high DPI screens here: > > > https://codereview.adblockplus.org/29340844/diff/29340845/pages/adware.html#n... > > > > Users shouldn't download bigger image in case they do not have high DPI > screens. > > Done. I added a srcset attribute that specifies an image for 1x and 2x. Browsers > that do not support srcset will fall back to src (1x). I like this solution! https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:107: top: 0; Detail: CSS number values should specify units where possible While you are changing this selector styles. https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:135: top: 17.5px; 17.5px, just curious why are you using 0.5px accuracy for positioning ? https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:140: margin: 0 0 0 25px; Detail: CSS number values should specify units where possible https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:152: top: -99999px; I'm not really familiar with this implementation, can you please describe why we are positioning this element absolutely and why it's not possible to change the visibility ? https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:2: Detail: compared to our other JS codes we do not have so many new lines after each block and piece of code. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:6: { I couldn't find the rule in our coding style whether we mention that single line condition should be wrapped with the braces, but for consistency reasons with other JS codes we have, I'll suggest not to have braces for single line conditions. Same goes to other single line conditions below. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:30: Detail: Same as above comment about new lines, same goes to the new lines below. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:33: Detail: no new line needed here as well https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:68: }()); Detail: According to our coding styles "Newline at end of file, otherwise no trailing whitespace" https://codereview.adblockplus.org/29335113/diff/29346588/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29346588/templates/default.t... templates/default.tmpl:38: <a id="logo" href="{{source.resolve_link("index", locale)[1]}}" hreflang="{{source.resolve_link("index", locale)[0]}}" title="Eyeo GmbH" rel="home"> While we are fixing the header I think it make sense to also fix the way we are generating the link, currently we are using source.resolve_link which is not documented in the CMS, so I'll suggest to use linkify function: {{"jobs"|linkify(id="logo",title="Eyeo GmbH",rel="home")}} <img src="/images/logo.png" srcset="/images/logo.png 1x, /images/logo-2x.png 2x" height="51px" alt="Eyeo GmbH" /> </a> https://codereview.adblockplus.org/29335113/diff/29346588/templates/default.t... templates/default.tmpl:74: <a href="{{source.resolve_link("jobs", locale)[1]}}" hreflang="{{source.resolve_link("jobs", locale)[0]}}" class="menu-button"> Same as the comment above you can use linkify: {{"jobs"|linkify(class="menu-button")}}Join us - we are hiring!</a>
Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:107: top: 0; On 2016/06/22 07:54:19, saroyanm wrote: > Detail: CSS number values should specify units where possible > While you are changing this selector styles. Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:135: top: 17.5px; On 2016/06/22 07:54:19, saroyanm wrote: > 17.5px, just curious why are you using 0.5px accuracy for positioning ? Changed. https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:140: margin: 0 0 0 25px; On 2016/06/22 07:54:19, saroyanm wrote: > Detail: CSS number values should specify units where possible Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/css/styles.c... static/css/styles.css:152: top: -99999px; On 2016/06/22 07:54:18, saroyanm wrote: > I'm not really familiar with this implementation, can you please describe why we > are positioning this element absolutely and why it's not possible to change the > visibility ? See: https://make.wordpress.org/accessibility/2015/02/09/hiding-text-for-screen-re... It affects the shape of the block when focused. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.js File static/js/scripts.js (right): https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:2: On 2016/06/22 07:54:20, saroyanm wrote: > Detail: compared to our other JS codes we do not have so many new lines after > each block and piece of code. Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:6: { On 2016/06/22 07:54:23, saroyanm wrote: > I couldn't find the rule in our coding style whether we mention that single line > condition should be wrapped with the braces, but for consistency reasons with > other JS codes we have, I'll suggest not to have braces for single line > conditions. > Same goes to other single line conditions below. Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:30: On 2016/06/22 07:54:19, saroyanm wrote: > Detail: Same as above comment about new lines, same goes to the new lines below. Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:33: On 2016/06/22 07:54:23, saroyanm wrote: > Detail: no new line needed here as well Done. https://codereview.adblockplus.org/29335113/diff/29346588/static/js/scripts.j... static/js/scripts.js:68: }()); On 2016/06/22 07:54:19, saroyanm wrote: > Detail: According to our coding styles "Newline at end of file, otherwise no > trailing whitespace" Done. https://codereview.adblockplus.org/29335113/diff/29346588/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29335113/diff/29346588/templates/default.t... templates/default.tmpl:38: <a id="logo" href="{{source.resolve_link("index", locale)[1]}}" hreflang="{{source.resolve_link("index", locale)[0]}}" title="Eyeo GmbH" rel="home"> On 2016/06/22 07:54:24, saroyanm wrote: > While we are fixing the header I think it make sense to also fix the way we are > generating the link, currently we are using source.resolve_link which is not > documented in the CMS, so I'll suggest to use linkify function: > {{"jobs"|linkify(id="logo",title="Eyeo GmbH",rel="home")}} > <img src="/images/logo.png" srcset="/images/logo.png 1x, /images/logo-2x.png > 2x" height="51px" alt="Eyeo GmbH" /> > </a> Done. https://codereview.adblockplus.org/29335113/diff/29346588/templates/default.t... templates/default.tmpl:74: <a href="{{source.resolve_link("jobs", locale)[1]}}" hreflang="{{source.resolve_link("jobs", locale)[0]}}" class="menu-button"> On 2016/06/22 07:54:24, saroyanm wrote: > Same as the comment above you can use linkify: > {{"jobs"|linkify(class="menu-button")}}Join us - we are hiring!</a> Done.
LGTM
On 2016/07/12 14:26:56, saroyanm wrote: > LGTM Woot! 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
