includes/layout/header.tmpl - Issue 29485575: Issue 5385 - Create Site Header Component for Help Center

Delta Between Two Patch Sets: includes/layout/header.tmpl

Issue 29485575: Issue 5385 - Create Site Header Component for Help Center (Closed)

Left Patch Set: Fix svgs, Implement search, Show searchbar for no-js Created July 17, 2017, 9:24 a.m.

Right Patch Set: Include site variables before website-defaults Created Sept. 1, 2017, 12:11 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 <header id="site-header" class="navbar">	1 <header id="site-header" class="navbar">

2 <div class="navbar-wrapper clearfix">	2 <div class="navbar-wrapper">
juliandoucette 2017/07/19 18:03:33 I don't think we need clearfix here? --- (Unless I don't think we need clearfix here? --- (Unless we decide to move some floating elements above the site-header in the future?) ire 2017/07/21 10:23:26 You're right it's not needed. I must have added it Show quoted text On 2017/07/19 18:03:33, juliandoucette wrote: > I don't think we need clearfix here? > > --- > > (Unless we decide to move some floating elements above the site-header in the > future?) You're right it's not needed. I must have added it when I had a slightly different implementation.
3 <h1 class="site-title">	3 <h1 id="site-title">

4 {{ "index" \| linkify() }}	4 {{ "index" \| linkify() }}

5 <img src="/img/png/eyeo-help.png" srcset="/img/png/eyeo-help.png, /img/s vg/eyeo-help.svg 2x" alt="{{ "eyeo Help" \| translate("site-title", "Image alt te xt") }}">	5 <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt ="{{ "eyeo Help" \| translate("site-title", "Image alt text") }}">
juliandoucette 2017/07/19 18:03:33 I don't think we need the first /img/png/eyeo-help I don't think we need the first /img/png/eyeo-help.png in srcset? ire 2017/07/21 10:23:25 Done. Show quoted text On 2017/07/19 18:03:33, juliandoucette wrote: > I don't think we need the first /img/png/eyeo-help.png in srcset? Done.
6 </a>	6 </a>

7 </h1>	7 </h1>

8 <button id="toggle-searchform" class="unstyled tablet-and-mobile-only">	8
juliandoucette 2017/07/19 18:03:32 I think this control is misleading. This button to I think this control is misleading. This button toggles a menu that contains a search form and an external link. If our intention is to toggle a menu, I would suggest a menu icon (hamburger) and alike alt-text. If our intention is to toggle a search form, I would suggest expanding this search form out of the search icon to the right (above the site logo which would fade out during).
9 <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/s vg/searchIcon.svg 2x" alt="{{ "Toggle Search Bar" \| translate("search-toggle-alt ", "Image alt text") }}">	9 <button class="toggle-navbar-collapse unstyled phablet-and-mobile-only">
juliandoucette 2017/07/19 18:03:32 NIT: Please use dashes in image names. NIT: Please use dashes in image names. juliandoucette 2017/07/19 18:03:33 1x image is poor quality on low DPI screens. I thi 1x image is poor quality on low DPI screens. I think we can make this crisper and/or use real text for "eye/o \| help" (keeping in mind that we can't promise fonts will be downloaded quickly or applied). ire 2017/07/21 10:23:25 Done. Show quoted text On 2017/07/19 18:03:32, juliandoucette wrote: > NIT: Please use dashes in image names. Done.
	10 <img src="/img/png/search-icon.png" srcset="/img/svg/search-icon.svg 2x" a lt="{{ "Toggle Search Bar" \| translate("search-toggle-alt", "Image alt text") }} ">

10 </button>	11 </button>

11 <div id="site-search">	12

12 <? include searchform ?>	13 <div class="navbar-collapse">

13 <a id="product-website-link" href="https://adblockplus.org/">{{ "Adblock P lus" \| translate("abp-product-link", "Link") }}</a>	14 <form id="search-form" action="https://duckduckgo.com" method="GET">
juliandoucette 2017/07/19 18:03:32 NIT: I think this should have a `nav` around it; t NIT: I think this should have a `nav` around it; technically :/ juliandoucette 2017/07/19 18:03:33 Missing icon. Missing icon. ire 2017/07/21 10:23:25 Done. Show quoted text On 2017/07/19 18:03:32, juliandoucette wrote: > NIT: I think this should have a `nav` around it; technically :/ Done.
	15 <label for="search" class="sr-only">{{ "Search Adblock Plus Help" \| tran slate("search-form-label", "Input label") }}</label>

	16 <input id="search" name="q" type="search" placeholder="{{ "Search Adbloc k Plus Help" \| translate("search-form-label", "Input label") }}">

	17 <input type="hidden" name="sites" value="adblockplus.org">

	18 <button type="submit">

	19 <img src="/img/png/search-icon.png" srcset="/img/svg/search-icon.svg 2 x" alt="{{ "Search" \| translate("search-icon-alt", "Image alt text") }}">

	20 </button>

	21 </form>

	22

	23 <nav id="product-website-link">

	24 <a href="https://adblockplus.org/">

	25 {{ "Adblock Plus" \| translate("abp-product-link", "Link label") }}<img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" \| translate("external-link-icon", "Image alt text") }}">

	26 </a>

	27 </nav>

14 </div>	28 </div>

15 </div>	29 </div>

16 </header>	30 </header>

LEFT	RIGHT