Unified Diff: 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
Patch Set: Fixed navbar-logo-2x
Created Sept. 28, 2017, 1:03 a.m.
Use n/p to move between diff chunks;
N/P to move between comments.
« static/js/main.js ('K')
|
« static/js/main.js ('k')
|
no next file »
|
no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')
| Index: templates/default.tmpl |
| =================================================================== |
| --- a/templates/default.tmpl |
| +++ b/templates/default.tmpl |
| @@ -71,55 +71,59 @@ |
| </head> |
| <body> |
| <noscript> |
| <link rel="stylesheet" href="/css/noscript-desktop.css" media="(min-width: 1000px)"/> |
| <link rel="stylesheet" href="/css/noscript-mobile.css" media="(max-width: 1000px)"/> |
| </noscript> |
| - {% macro pageitem(name) %} |
| - {% if name == page %} |
| - <li class="selected">{{get_string(name, "menu")}}</li> |
| - {% else %} |
| - <li>{{name|linkify}}{{get_string(name, "menu")}}</a></li> |
| - {% endif %} |
| + {% macro pageitem(pagename) %} |
| + <li{{ " class=\"selected\"" if pagename == page else "" }}> |
|
ire
2017/09/28 08:38:44
Theres no styling for selected items
ire
2017/09/28 08:38:45
`class=\"selected\"` This is resulting in there be
ire
2017/09/28 08:38:45
I don't think you need the else part of this state
juliandoucette
2017/10/04 15:33:29
Acknowledged.
juliandoucette
2017/10/04 15:33:29
~Not true
https://stackoverflow.com/questions/121
juliandoucette
2017/10/04 15:33:29
Acknowledged.
(This is used in the footer too. I
ire
2017/10/04 21:44:26
It's in the Jinja2 docs:
|
| + {{ pagename | linkify }}{{ get_string(pagename, "menu") }}</a> |
| + </li> |
| {% endmacro %} |
| - <header> |
| - {{"index"|linkify(id="logo", class="sprite", itemprop="image")}}</a> |
| - <nav> |
| - <ul> |
| - {% if localefile == "index" %} |
| - <li class="selected first">{{get_string("installation", "menu")}}</li> |
| - {% else %} |
| - <li class="install-link first">{{"index"|linkify}}{{get_string("installation", "menu")}} <span class="sprite install-link-icon"></span></a></li> |
| - {% endif %} |
| - {% for name in ["about", "features", "bugs", "contribute"] %} |
| - {{pageitem(name)}} |
| + <nav id="navbar-primary"> |
| + <div class="container"> |
| + {{ "index" | linkify(id="navbar-logo") }} |
| + <img |
| + alt="{{ "ABP" | translate("navbar-logo-alt", "Navbar logo alt text") }}" |
|
ire
2017/09/28 08:38:46
NIT: Use a more descriptive alt text?
juliandoucette
2017/10/04 15:33:28
I used "ABP" because it doesn't require translatio
ire
2017/10/04 21:44:26
Sure.
|
| + src="/img/navbar-logo.png" |
| + srcset="/img/navbar-logo-2x.svg 2x"> |
|
ire
2017/09/28 08:38:44
NIT: Do we typically add -2x modifiers on the SVG
juliandoucette
2017/10/04 15:33:28
I don't know (we don't typically support 2x - I di
ire
2017/10/04 21:44:26
Here's my thoughts:
1. I like our other method of
|
| + <span>{{ "Adblock <strong>Plus</strong>" | translate("navbar-logo-text", "Navbar logo text") }}</span> |
| + </a> |
| + <a href="#" id="navbar-menu"> |
| + <img |
| + height="25px" |
| + src="/img/navbar-menu-icon.png" |
| + srcset="/img/navbar-menu-icon-2x.png" |
| + alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> |
|
ire
2017/09/28 08:38:45
NIT/Suggest: "Navbar menu icon alt text"
ire
2017/09/28 08:38:45
"Menu icon" alone doesn't convey that this will to
juliandoucette
2017/10/04 15:33:29
I will remove this text and address it in #5753. T
juliandoucette
2017/10/04 15:33:29
Acknowledged.
ire
2017/10/04 21:44:25
Acknowledged.
|
| + </a> |
| + <ul id="navbar-nav"> |
| + {% for pagename in ["about", "features", "bugs", "contribute"] %} |
| + {{ pageitem(pagename) }} |
| {% endfor %} |
| - <li id="language"> |
| - <div id="current-language"> |
| + <li id="locale"> |
| + <a href="#" id="selected-locale"> |
| {{ config.get("langnames", locale) }} ({{ locale | upper }}) |
| - <span id="language-arrow" class="sprite"></span> |
| - </div> |
| - <ul id="language-selector"> |
| + </a> |
| + <ul id="locales" role="menu" aria-labelledby="selected-locale"> |
| {% for available_locale in available_locales %} |
| <li class="language-entry"> |
| - {{ page | linkify(available_locale) }} |
| + {{ page | linkify(available_locale, class="selected" if available_locale == locale else "") }} |
|
ire
2017/09/28 08:38:46
Same here. I don't think the `else ""` is necessar
juliandoucette
2017/10/04 15:33:28
See comment above.
|
| {{ config.get("langnames", available_locale) }} ({{ available_locale | upper }}) |
| </a> |
| </li> |
| {% endfor %} |
| </ul> |
| </li> |
| </ul> |
| - <a href="#" id="hamburger"></a> |
| - </nav> |
| - </header> |
| + </div><!-- .container --> |
| + </nav> |
| <div id="content" {% if localefile == "index" %}class="{{page}}"{% endif %}> |
| {% if abbnotification %} |
| <? include abb-notification ?> |
| {% endif %} |
| {% if not noheading %} |
| <h1>{{title|translate("title")}}</h1> |
| {% endif %} |
« static/js/main.js ('K')
|
« static/js/main.js ('k')
|
no next file »
|
no next file with comments »
