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 alignment of search form, reformat JS Created Aug. 22, 2017, 12:14 p.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">	2 <div class="navbar-wrapper">
juliandoucette 2017/08/23 14:39:02 See comment [here](https://codereview.adblockplus. See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode68) ire 2017/08/25 08:27:05 Acknowledged. Show quoted text On 2017/08/23 14:39:02, juliandoucette wrote: > See comment > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode68) Acknowledged.
3 <h1 class="site-title">	3 <h1 id="site-title">

4 {{ "index" \| linkify() }}	4 {{ "index" \| linkify() }}
juliandoucette 2017/08/23 14:39:03 See comment [here](https://codereview.adblockplus. See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/_header.scss#newcode24) ire 2017/08/25 08:27:06 Acknowledged. Show quoted text On 2017/08/23 14:39:03, juliandoucette wrote: > See comment > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/_header.scss#newcode24) Acknowledged.
5 <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt ="{{ "eyeo Help" \| translate("site-title", "Image alt text") }}">	5 <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt ="{{ "eyeo Help" \| translate("site-title", "Image alt text") }}">

6 </a>	6 </a>

7 </h1>	7 </h1>

8 <button id="toggle-search-form" class="unstyled tablet-and-mobile-only">	8

	9 <button class="toggle-navbar-collapse unstyled phablet-and-mobile-only">

9 <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 <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
juliandoucette 2017/08/23 14:39:02 I think that it's kindof confusing that this eleme I think that it's kindof confusing that this element has an ID "site-search" and contains a nav that is not part of the "search" ~area on desktop. I think that the purpose of this element is to contain children that are collapsed on mobile/tablet. If that is the case, why not make it more generic e.g. `.navbar-collapse` (or equivilant) (our navbars on other websites will also have elements that collapse like this). ire 2017/08/25 08:27:06 Done. Show quoted text On 2017/08/23 14:39:02, juliandoucette wrote: > I think that it's kindof confusing that this element has an ID "site-search" and > contains a nav that is not part of the "search" ~area on desktop. I think that > the purpose of this element is to contain children that are collapsed on > mobile/tablet. If that is the case, why not make it more generic e.g. > `.navbar-collapse` (or equivilant) (our navbars on other websites will also have > elements that collapse like this). Done.
12 <? include search-form ?>	13 <div class="navbar-collapse">
juliandoucette 2017/08/23 14:39:03 It seems inconsistent that this form is an include It seems inconsistent that this form is an include and the nav below is not. I would suggest that this form should only be an include if it appears elsewhere outside of the header. ire 2017/08/25 08:27:05 Done. Show quoted text On 2017/08/23 14:39:03, juliandoucette wrote: > It seems inconsistent that this form is an include and the nav below is not. I > would suggest that this form should only be an include if it appears elsewhere > outside of the header. Done.
	14 <form id="search-form" action="https://duckduckgo.com" method="GET">

	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

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

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

15 {{ "Adblock Plus" \| translate("abp-product-link", "Link") }}	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") }}">
juliandoucette 2017/08/23 14:39:03 NIT/Suggest: "Link text" or "Link label" (it's not NIT/Suggest: "Link text" or "Link label" (it's not a URL) ire 2017/08/25 08:27:05 Done. Show quoted text On 2017/08/23 14:39:03, juliandoucette wrote: > NIT/Suggest: "Link text" or "Link label" (it's not a URL) Done.
16 <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.s vg 2x" alt="{{ "External link icon" \| translate("external-link-icon", "Image alt text") }}">

17 </a>	26 </a>

18 </nav>	27 </nav>

19 </div>	28 </div>

20 </div>	29 </div>

21 </header>	30 </header>

LEFT	RIGHT