templates/default.tmpl - Issue 29551738: Issue 5634 - Replaced logo and refactored navbar width and colors

Delta Between Two Patch Sets: templates/default.tmpl

Issue 29551738: Issue 5634 - Replaced logo and refactored navbar width and colors (Closed) Base URL: https://hg.adblockplus.org/web.adblockplus.org

Left Patch Set: Fixed navbar-logo-2x Created Sept. 28, 2017, 1:03 a.m.

Right Patch Set: Addressed comments Created Oct. 11, 2017, 11:59 a.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 {#	1 {#

2 # This file is part of the Adblock Plus website,	2 # This file is part of the Adblock Plus website,

3 # Copyright (C) 2006-present eyeo GmbH	3 # Copyright (C) 2006-present eyeo GmbH

4 #	4 #

5 # Adblock Plus is free software: you can redistribute it and/or modify	5 # Adblock Plus is free software: you can redistribute it and/or modify

6 # it under the terms of the GNU General Public License version 3 as	6 # it under the terms of the GNU General Public License version 3 as

7 # published by the Free Software Foundation.	7 # published by the Free Software Foundation.

8 #	8 #

9 # Adblock Plus is distributed in the hope that it will be useful,	9 # Adblock Plus is distributed in the hope that it will be useful,

10 # but WITHOUT ANY WARRANTY; without even the implied warranty of	10 # but WITHOUT ANY WARRANTY; without even the implied warranty of

(...skipping 27 matching lines...) Expand all Loading...
38 {% if alternate_locale != locale %}	38 {% if alternate_locale != locale %}

39 <meta property="og:locale:alternate" content="{{ alternate_locale \| to_o g_locale }}">	39 <meta property="og:locale:alternate" content="{{ alternate_locale \| to_o g_locale }}">

40 {% endif %}	40 {% endif %}

41 {% endfor %}	41 {% endfor %}

42 <meta property="og:url" content="{{ get_canonical_url(page \| ignore_browsers ) }}">	42 <meta property="og:url" content="{{ get_canonical_url(page \| ignore_browsers ) }}">

43	43

44 {# twitter https://dev.twitter.com/cards/markup #}	44 {# twitter https://dev.twitter.com/cards/markup #}

45 <meta name="twitter:site" content="@AdblockPlus" />	45 <meta name="twitter:site" content="@AdblockPlus" />

46 <meta name="twitter:creator" content="@AdblockPlus" />	46 <meta name="twitter:creator" content="@AdblockPlus" />

47	47

	48 <link rel="stylesheet" href="/css/defaults.css" class="cssfx">

48 <link rel="stylesheet" href="/css/main.css" class="cssfx">	49 <link rel="stylesheet" href="/css/main.css" class="cssfx">

49 <link rel="stylesheet" href="/css/main-desktop.css" media="(min-width: 1000p x)" class="cssfx">	50 <link rel="stylesheet" href="/css/main-desktop.css" media="(min-width: 1000p x)" class="cssfx">

50 <link rel="stylesheet" href="/css/main-mobile.css" media="(max-width: 1000px )">	51 <link rel="stylesheet" href="/css/main-mobile.css" media="(max-width: 1000px )">

51 <link rel="apple-touch-icon" href="/img/favicon-large.png" />	52 <link rel="apple-touch-icon" href="/img/favicon-large.png" />

52	53

53 <!--[if IE 8]>	54 <!--[if IE 8]>

54 <script src="/js/vendor/ie8.js"></script>	55 <script src="/js/vendor/ie8.js"></script>

55 <![endif]-->	56 <![endif]-->

56	57

57 {% block head %}	58 {% block head %}

(...skipping 11 matching lines...) Expand all Loading...
69 <link rel="stylesheet" href="/css/empty.css" class="cssfx"/>	70 <link rel="stylesheet" href="/css/empty.css" class="cssfx"/>

70 <![endif]-->	71 <![endif]-->

71 </head>	72 </head>

72	73

73 <body>	74 <body>

74 <noscript>	75 <noscript>

75 <link rel="stylesheet" href="/css/noscript-desktop.css" media="(min-width: 1000px)"/>	76 <link rel="stylesheet" href="/css/noscript-desktop.css" media="(min-width: 1000px)"/>

76 <link rel="stylesheet" href="/css/noscript-mobile.css" media="(max-width: 1000px)"/>	77 <link rel="stylesheet" href="/css/noscript-mobile.css" media="(max-width: 1000px)"/>

77 </noscript>	78 </noscript>

78	79

79 {% macro pageitem(pagename) %}	80 {% macro pageitem(name) %}

80 <li{{ " class=\"selected\"" if pagename == page else "" }}>	81 {% if name == page %}
ire 2017/09/28 08:38:44 Theres no styling for selected items Theres no styling for selected items ire 2017/09/28 08:38:45 `class=\"selected\"` This is resulting in there be `class=\"selected\"` This is resulting in there being double quotes, i.e. class=""selected"". You don't need the inner quotes. ire 2017/09/28 08:38:45 I don't think you need the else part of this state I don't think you need the else part of this statement juliandoucette 2017/10/04 15:33:29 Acknowledged. Show quoted text On 2017/09/28 08:38:45, ire wrote: > `class=\"selected\"` This is resulting in there being double quotes, i.e. > class=""selected"". You don't need the inner quotes. Acknowledged. juliandoucette 2017/10/04 15:33:29 ~Not true https://stackoverflow.com/questions/121 Show quoted text On 2017/09/28 08:38:45, ire wrote: > I don't think you need the else part of this statement ~Not true https://stackoverflow.com/questions/12199757/python-ternary-operator-without-... https://stackoverflow.com/questions/14214942/python-jinja2-shorthand-conditional I'll give the "and" solution a shot. Let me know if you think it's too confusing. juliandoucette 2017/10/04 15:33:29 Acknowledged. (This is used in the footer too. I Show quoted text On 2017/09/28 08:38:44, ire wrote: > Theres no styling for selected items Acknowledged. (This is used in the footer too. I was too lazy to check if there were footer specific styles.) ire 2017/10/04 21:44:26 It's in the Jinja2 docs: Show quoted text On 2017/10/04 15:33:29, juliandoucette wrote: > On 2017/09/28 08:38:45, ire wrote: > > I don't think you need the else part of this statement > > ~Not true > > https://stackoverflow.com/questions/12199757/python-ternary-operator-without-... > https://stackoverflow.com/questions/14214942/python-jinja2-shorthand-conditional > > I'll give the "and" solution a shot. Let me know if you think it's too > confusing. It's in the Jinja2 docs: Show quoted text > The else part is optional. If not provided, the else block implicitly evaluates into an undefined object (http://jinja.pocoo.org/docs/2.9/templates/#if-expression) I also tried it myself and it works without just `{{ " class=selected " if pagename == page }}`. I think those answers are to do with just Python, not using Jinja.
81 {{ pagename \| linkify }}{{ get_string(pagename, "menu") }}</a>	82 <li class="selected">{{get_string(name, "menu")}}</li>

82 </li>	83 {% else %}

	84 <li>{{name\|linkify}}{{get_string(name, "menu")}}</a></li>

	85 {% endif %}

83 {% endmacro %}	86 {% endmacro %}

84	87

85 <nav id="navbar-primary">	88 <nav id="navbar">

86 <div class="container">	89 <div class="navbar-container">

87 {{ "index" \| linkify(id="navbar-logo") }}	90 {{ "index" \| linkify(id="navbar-logo") }}

88 <img	91 <img

89 alt="{{ "ABP" \| translate("navbar-logo-alt", "Navbar logo alt text") }}"
ire 2017/09/28 08:38:46 NIT: Use a more descriptive alt text? NIT: Use a more descriptive alt text? juliandoucette 2017/10/04 15:33:28 I used "ABP" because it doesn't require translatio Show quoted text On 2017/09/28 08:38:46, ire wrote: > NIT: Use a more descriptive alt text? I used "ABP" because it doesn't require translation. I think that we can address this separately. ire 2017/10/04 21:44:26 Sure. Show quoted text On 2017/10/04 15:33:28, juliandoucette wrote: > On 2017/09/28 08:38:46, ire wrote: > > NIT: Use a more descriptive alt text? > > I used "ABP" because it doesn't require translation. I think that we can address > this separately. Sure.
90 src="/img/navbar-logo.png"	92 src="/img/navbar-logo.png"

91 srcset="/img/navbar-logo-2x.svg 2x">	93 srcset="/img/navbar-logo.svg 2x">
ire 2017/09/28 08:38:44 NIT: Do we typically add -2x modifiers on the SVG NIT: Do we typically add -2x modifiers on the SVG versions? juliandoucette 2017/10/04 15:33:28 I don't know (we don't typically support 2x - I di Show quoted text On 2017/09/28 08:38:44, ire wrote: > NIT: Do we typically add -2x modifiers on the SVG versions? I don't know (we don't typically support 2x - I did for the first time when I rushed through acceptableads.com). Do you love it or hate it? ire 2017/10/04 21:44:26 Here's my thoughts: 1. I like our other method of Show quoted text On 2017/10/04 15:33:28, juliandoucette wrote: > On 2017/09/28 08:38:44, ire wrote: > > NIT: Do we typically add -2x modifiers on the SVG versions? > > I don't know (we don't typically support 2x - I did for the first time when I > rushed through http://acceptableads.com). > > Do you love it or hate it? Here's my thoughts: 1. I like our other method of separating the files into different folders (png, png2x, svg) which means we don't have to add the 2x modifier on the filenames. 2. Assuming we can't (or don't want to for whatever reason) do that here, I think it makes sense to have the modifiers on the filenames so it's clear. 3. I think -2x on an svg file is misleading because an svg can be any size
92 <span>{{ "Adblock <strong>Plus</strong>" \| translate("navbar-logo-text ", "Navbar logo text") }}</span>	94 <span>{{ "Adblock <strong>Plus</strong>" \| translate("navbar-logo-text ", "Navbar logo text") }}</span>

93 </a>	95 </a>

94 <a href="#" id="navbar-menu">	96 <a href="#" id="navbar-menu-toggle">

95 <img	97 <img

96 height="25px"	98 src="/img/menu-toggle.png"

97 src="/img/navbar-menu-icon.png"	99 srcset="/img/menu-toggle.svg 2x">

98 srcset="/img/navbar-menu-icon-2x.png"

99 alt="{{ "Menu icon" \| translate("navbar-menu-logo", "Navbar menu log o alt text") }}">
ire 2017/09/28 08:38:45 NIT/Suggest: "Navbar menu icon alt text" NIT/Suggest: "Navbar menu icon alt text" ire 2017/09/28 08:38:45 "Menu icon" alone doesn't convey that this will to "Menu icon" alone doesn't convey that this will toggle the menubar open/closed. Will this be handled in #5753? juliandoucette 2017/10/04 15:33:29 I will remove this text and address it in #5753. T Show quoted text On 2017/09/28 08:38:45, ire wrote: > "Menu icon" alone doesn't convey that this will toggle the menubar open/closed. > Will this be handled in #5753? I will remove this text and address it in #5753. The same goes elsewhere. juliandoucette 2017/10/04 15:33:29 Acknowledged. Show quoted text On 2017/09/28 08:38:45, ire wrote: > NIT/Suggest: "Navbar menu icon alt text" Acknowledged. ire 2017/10/04 21:44:25 Acknowledged. Show quoted text On 2017/10/04 15:33:29, juliandoucette wrote: > On 2017/09/28 08:38:45, ire wrote: > > "Menu icon" alone doesn't convey that this will toggle the menubar > open/closed. > > Will this be handled in #5753? > > I will remove this text and address it in #5753. The same goes elsewhere. Acknowledged.
100 </a>	100 </a>

101 <ul id="navbar-nav">	101 <ul id="navbar-menu">

102 {% for pagename in ["about", "features", "bugs", "contribute"] %}	102 {% for pagename in ["about", "features", "bugs", "contribute"] %}

103 {{ pageitem(pagename) }}	103 <li>

	104 {{ pagename \| linkify }}{{ get_string(pagename, "menu") }}</a>

	105 </li>

104 {% endfor %}	106 {% endfor %}

105 <li id="locale">	107 <li id="navbar-locale-menubar">

106 <a href="#" id="selected-locale">	108 <a href="#" id="navbar-locale-selected">

107 {{ config.get("langnames", locale) }} ({{ locale \| upper }})	109 {{ config.get("langnames", locale) }} ({{ locale \| to_og_locale \| to_og_location }})

108 </a>	110 </a>

109 <ul id="locales" role="menu" aria-labelledby="selected-locale">	111 <ul id="navbar-locale-menu">

110 {% for available_locale in available_locales %}	112 {% for available_locale in available_locales %}

111 <li class="language-entry">	113 <li>

112 {{ page \| linkify(available_locale, class="selected" if availa ble_locale == locale else "") }}	114 {{ page \| linkify(available_locale) }}
ire 2017/09/28 08:38:46 Same here. I don't think the `else ""` is necessar Same here. I don't think the `else ""` is necessary juliandoucette 2017/10/04 15:33:28 See comment above. Show quoted text On 2017/09/28 08:38:46, ire wrote: > Same here. I don't think the `else ""` is necessary See comment above.
113 {{ config.get("langnames", available_locale) }} ({{ availabl e_locale \| upper }})	115 {{ config.get("langnames", available_locale) }} ({{ availabl e_locale \| to_og_locale \| to_og_location }})

114 </a>	116 </a>

115 </li>	117 </li>

116 {% endfor %}	118 {% endfor %}

117 </ul>	119 </ul>{# #navbar-locale-menu #}

118 </li>	120 </li>

119 </ul>	121 </ul>{# #navbar-menu #}

120 </div><!-- .container -->	122 </div>{# .container #}

121 </nav>	123 </nav>{# #navbar #}

122	124

123 <div id="content" {% if localefile == "index" %}class="{{page}}"{% endif %}>	125 <div id="content" class="container content {{ localefile == "index" and page }}">

124 {% if abbnotification %}	126 {% if abbnotification %}

125 <? include abb-notification ?>	127 <? include abb-notification ?>

126 {% endif %}	128 {% endif %}

127 {% if not noheading %}	129 {% if not noheading %}

128 <h1>{{title\|translate("title")}}</h1>	130 <h1>{{title\|translate("title")}}</h1>

129 {% endif %}	131 {% endif %}

130	132

131 {% if not notoc %}	133 {% if not notoc %}

132 {% macro toc(toclist) %}	134 {% macro toc(toclist) %}

133 <ul>	135 <ul>

(...skipping 65 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
199 <a rel="nofollow" id="social-gplus" class="sprite" href="https://p lus.google.com/110020691898167279887"></a>	201 <a rel="nofollow" id="social-gplus" class="sprite" href="https://p lus.google.com/110020691898167279887"></a>

200 </li>	202 </li>

201 </ul>	203 </ul>

202 </section>	204 </section>

203 </div>	205 </div>

204 </footer>	206 </footer>

205	207

206 <script src="/js/main.js"></script>	208 <script src="/js/main.js"></script>

207 </body>	209 </body>

208 </html>	210 </html>

LEFT	RIGHT