templates/default.tmpl - Issue 29335113: Issue 2675 - Implemented responsive header for web.eyeo.com

Delta Between Two Patch Sets: templates/default.tmpl

Issue 29335113: Issue 2675 - Implemented responsive header for web.eyeo.com (Closed)

Left Patch Set: Implemented scripts in vanilla.js Created May 11, 2016, 11:25 p.m.

Right Patch Set: Fixed code style and consistancy issues Created June 23, 2016, 5:23 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html lang="{{locale}}">	2 <html lang="{{locale}}">

3 <head>	3 <head>

4 <meta charset="UTF-8" />	4 <meta charset="UTF-8" />

5 <meta name="viewport" content="width=device-width,initial-scale=1,maximum-scal e=1,user-scalable=no" />	5 <meta name="viewport" content="width=device-width,initial-scale=1,maximum-scal e=1,user-scalable=no" />

6 <meta http-equiv="X-UA-Compatible" content="IE=edge">	6 <meta http-equiv="X-UA-Compatible" content="IE=edge">

7	7

8 {% if title %}	8 {% if title %}

9 <title>{{title}} \| Eyeo GmbH</title>	9 <title>{{title}} \| Eyeo GmbH</title>

10 {% else %}	10 {% else %}

11 <title>Eyeo GmbH</title>	11 <title>Eyeo GmbH</title>

12 {% endif %}	12 {% endif %}

13	13

14 <link rel="shortcut icon" href="/images/favicon.png" />	14 <link rel="shortcut icon" href="/images/favicon.png" />

15	15

16 <link rel="stylesheet" href="/css/styles.css" type="text/css" media="all">	16 <link rel="stylesheet" href="/css/styles.css" type="text/css" media="all">

17	17

18 <!--[if lt IE 9]>	18 <!--[if lt IE 9]>

19 <script src="/static/js/html5shiv.min.js"></script>	19 <script src="/js/html5shiv.min.js"></script>

20 <script src="/static/js/respond.min.js"></script>	20 <script src="/js/respond.min.js"></script>

21 <![endif]-->	21 <![endif]-->

22	22

23 <script src="/js/jquery.js"></script>	23 <script src="/js/jquery.js"></script>

24 <script src="/js/scripts.js"></script>	24 <script src="/js/scripts.js"></script>

25	25

26 <link rel="canonical" href="https://eyeo.com/{{ page[:-5] if page.split('/')[- 1] == 'index' else page }}">	26 <link rel="canonical" href="https://eyeo.com/{{ page[:-5] if page.split('/')[- 1] == 'index' else page }}">

27	27

28 {% block head %}	28 {% block head %}

29 {{head\|safe}}	29 {{head\|safe}}

30 {% endblock %}	30 {% endblock %}

31 </head>	31 </head>

32	32

33 <body id="top" class="home page page-template-default apollo_validation_on cover wpb-js-composer js-comp-ver-3.6.14.1 vc_responsive">	33 <body id="top" class="home page page-template-default apollo_validation_on cover wpb-js-composer js-comp-ver-3.6.14.1 vc_responsive">

34	34

35 <header id="header" class="top">	35 <header id="header" class="top">

36 <div class="content-block">	36 <div class="content-block">

37	37

38 <a id="logo" href="{{source.resolve_link("index", locale)[1]}}" hreflang=" {{source.resolve_link("index", locale)[0]}}" title="Eyeo GmbH" rel="home">	38 {{"jobs"\|linkify(id="logo",title="Eyeo GmbH",rel="home")}}

39 <img src="/images/tog_logo.png" height="51px" alt="Eyeo GmbH" />	39 <img src="/images/logo.png" srcset="/images/logo.png 1x, /images/logo-2x .png
saroyanm 2016/05/19 07:43:41 Detail: I assume we had misspelled beforehand, top Detail: I assume we had misspelled beforehand, top_logo ? juliandoucette 2016/05/19 12:01:45 Yes. I'll correct it though. Show quoted text On 2016/05/19 07:43:41, saroyanm wrote: > Detail: I assume we had misspelled beforehand, top_logo ? Yes. I'll correct it though. saroyanm 2016/05/23 12:39:21 I made a comment regarding high DPI screens here: Show quoted text 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. juliandoucette 2016/06/16 18:07:45 Done. I added a srcset attribute that specifies an Show quoted text 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). saroyanm 2016/06/22 07:54:18 I like this solution! Show quoted text 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!
	40 2x" height="51px" alt="Eyeo GmbH" />

40 </a>	41 </a>

41	42

42 <a class="assistive-text" href="#content">	43 <a class="assistive-text" href="#content">

43 Skip to main content	44 Skip to main content

44 </a>	45 </a>

45	46

46 <button type="button" id="header-hamberger" aria-expanded="false">	47 <button id="header-hamburger" aria-expanded="false">
saroyanm 2016/05/19 07:43:41 Hamburger also should belong to the navigation as Hamburger also should belong to the navigation as well as the "skip to content button" and "join us" button. juliandoucette 2016/05/19 12:01:45 About the _skip to content_ link: I disagree beca Show quoted text 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. saroyanm 2016/05/23 12:39:21 Thanks for detailed explanation I think you are ri Show quoted text 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.
47 <span class="assistive-text">Toggle navigation</span>	48 <span class="assistive-text">Toggle navigation</span>

48 <span class="icon-bar"></span>	49 <span class="icon-bar"></span>

49 <span class="icon-bar"></span>	50 <span class="icon-bar"></span>

50 <span class="icon-bar"></span>	51 <span class="icon-bar"></span>

51 </button>	52 </button>

52	53

53 <a href="{{source.resolve_link("jobs", locale)[1]}}" hreflang="{{source.re solve_link("jobs", locale)[0]}}" class="menu-button">	54 <nav id="menu" role="navigation" aria-labelledby="header-hamburger" aria-e xpanded="false">

54 Join us - we are hiring!

55 </a>

56

57 <nav id="menu" role="navigation" aria-labelledby="header-hamberger" aria-e xpanded="false">

58 <h1 class="assistive-text">Main menu</h1>	55 <h1 class="assistive-text">Main menu</h1>

59 <ul class="menu-list">	56 <ul class="menu-list">

60 {%- for link, title in [("index", "About Us"), ("services", "Services" ), ("team", "Team"), ("press", "Press"), ("contact", "Contact Us")] %}	57 {%- for link, title in [("index", "About Us"), ("services", "Services" ), ("team", "Team"), ("press", "Press"), ("contact", "Contact Us")] %}

61 {%- if link == page %}	58 {%- if link == page %}

62 <li class="menu-item menu-item-type-post_type menu-item-object-pag e current-menu-item page_item current_page_item">	59 <li class="current-menu-item">
saroyanm 2016/05/19 07:43:41 Feels like only "current-menu-item" class is being Feels like only "current-menu-item" class is being used. juliandoucette 2016/05/19 12:01:45 A WordPress artifact, no doubt. I'll check and re Show quoted text 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. juliandoucette 2016/06/16 18:07:45 Done. Show quoted text 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.
63 {{link\|linkify}}	60 {{link\|linkify}}

64 <span>{{title}}</span>	61 <span>{{title}}</span>

65 </a>	62 </a>

66 </li>	63 </li>

67 {%- else %}	64 {%- else %}

68 <li class="menu-item menu-item-type-post_type menu-item-object-pag e">	65 <li>

69 {{link\|linkify}}	66 {{link\|linkify}}

70 <span>{{title}}</span>	67 <span>{{title}}</span>

71 </a>	68 </a>

72 </li>	69 </li>

73 {%- endif %}	70 {%- endif %}

74 {%- endfor %}	71 {%- endfor %}

75 </ul>	72 </ul>

76 </nav>	73 </nav>

	74

	75 {{"jobs"\|linkify(class="menu-button")}}Join us - we are hiring!</a>

77	76

78 </div>	77 </div>

79 </header>	78 </header>

80	79

81 <div id="content">	80 <div id="content">

82 {{body\|safe}}	81 {{body\|safe}}

83 </div>	82 </div>

84	83

85 <footer id="footer">	84 <footer id="footer">

86 <div class="content-block">	85 <div class="content-block">

87 <a id="footer-contact" href="{{source.resolve_link("contact", locale)[1]}} " hreflang="{{source.resolve_link("contact", locale)[0]}}">	86 <a id="footer-contact" href="{{source.resolve_link("contact", locale)[1]}} " hreflang="{{source.resolve_link("contact", locale)[0]}}">

88 Made with ♥ in Cologne	87 Made with ♥ in Cologne

89 </a>	88 </a>

90	89

91 <div class="footer-text">	90 <div class="footer-text">

92 Adblock Plus™ and Acceptable Ads™ are registered trademarks of Eyeo GmbH .	91 Adblock Plus™ and Acceptable Ads™ are registered trademarks of Eyeo GmbH .

93 </div>	92 </div>

94	93

95 <a href="#top" id="to-top" style="opacity: 0;"></a>

96 </div>	94 </div>

97 </footer>	95 </footer>

98 </body>	96 </body>

99	97

100 </html>	98 </html>

LEFT	RIGHT