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: Unfix header, implement standard spacing units Created Aug. 16, 2017, 2:37 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">
ire 2017/08/16 14:41:26 I spoke with Jeen and she said the header shouldn' I spoke with Jeen and she said the header shouldn't be fixed to the top juliandoucette 2017/08/18 15:03:54 Acknowledged. Show quoted text On 2017/08/16 14:41:26, ire wrote: > I spoke with Jeen and she said the header shouldn't be fixed to the top Acknowledged.
2 <div class="navbar-wrapper">	2 <div class="navbar-wrapper">

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/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") }}">
juliandoucette 2017/08/18 15:03:52 NIT: This 1x image quality is unacceptable IMO. I' NIT: This 1x image quality is unacceptable IMO. I'll send a screenshot that may or may not demonstrate this. This doesn't need to block the review. But it wouldn't hurt if we updated it soon. juliandoucette 2017/08/18 15:03:55 Note: I'm not sure if no textContent within an h1 Note: I'm not sure if no textContent within an h1 has a negative impact on a document outline. Or, if it does, if that matters. I think this is fine for now. I just wanted to point this out. ire 2017/08/21 15:23:17 Acknowledged. Show quoted text On 2017/08/18 15:03:52, juliandoucette wrote: > NIT: This 1x image quality is unacceptable IMO. I'll send a screenshot that may > or may not demonstrate this. This doesn't need to block the review. But it > wouldn't hurt if we updated it soon. Acknowledged. ire 2017/08/21 15:23:17 Acknowledged. Show quoted text On 2017/08/18 15:03:55, juliandoucette wrote: > Note: I'm not sure if no textContent within an h1 has a negative impact on a > document outline. Or, if it does, if that matters. I think this is fine for now. > I just wanted to point this out. Acknowledged.
6 </a>	6 </a>

7 </h1>	7 </h1>

8 <button id="toggle-search-form" class="unstyled tablet-and-mobile-only">	8
juliandoucette 2017/08/18 15:03:54 Note: It seems super-confusing t ome that the togg Note: It seems super-confusing t ome that the toggle button is a search icon. I'm not sure if that matters though (sorry if I brought this up before). juliandoucette 2017/08/18 15:03:54 NIT: The click area of this button is too small IM NIT: The click area of this button is too small IMO. It's currently {h: 40, w: 15} and I think it should be {h: 40, w: 40} (or whatever em equivalent) ire 2017/08/21 15:23:17 Done. Show quoted text On 2017/08/18 15:03:54, juliandoucette wrote: > NIT: The click area of this button is too small IMO. It's currently {h: 40, w: > 15} and I think it should be {h: 40, w: 40} (or whatever em equivalent) Done. ire 2017/08/21 15:23:18 Yes it did come up, sorry that I didn't address it Show quoted text On 2017/08/18 15:03:54, juliandoucette wrote: > Note: It seems super-confusing t ome that the toggle button is a search icon. > I'm not sure if that matters though (sorry if I brought this up before). Yes it did come up, sorry that I didn't address it. I think the search icon makes sense because its toggling the search form. A hamburger menu would imply that there is a navigation there when there isn't. Also, searching is the primary action a user would want to take, so making it clear that a search bar would appear (and not just a menu) makes sense to me. juliandoucette 2017/08/21 16:28:38 Acknowledged. I agree. Show quoted text On 2017/08/21 15:23:18, ire wrote: > On 2017/08/18 15:03:54, juliandoucette wrote: > > Note: It seems super-confusing t ome that the toggle button is a search icon. > > I'm not sure if that matters though (sorry if I brought this up before). > > Yes it did come up, sorry that I didn't address it. > I think the search icon makes sense because its toggling the search form. A > hamburger menu would imply that there is a navigation there when there isn't. > Also, searching is the primary action a user would want to take, so making it > clear that a search bar would appear (and not just a menu) makes sense to me. Acknowledged. I agree.
	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

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

	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/18 15:03:54 NIT: I think this text is larger in the mock up. NIT: I think this text is larger in the mock up. ire 2017/08/21 15:23:17 Will ask Jeen for font sizes Show quoted text On 2017/08/18 15:03:54, juliandoucette wrote: > NIT: I think this text is larger in the mock up. Will ask Jeen for font sizes
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") }}">
juliandoucette 2017/08/18 15:03:51 Note: I'm assuming that you will replace "Image al Note: I'm assuming that you will replace "Image alt text" everywhere. (Sorry if this was already brought up) juliandoucette 2017/08/18 15:03:53 NIT: Isn't this the wrong icon? (The one in the sp NIT: Isn't this the wrong icon? (The one in the spec looks better IMO) ire 2017/08/21 15:23:18 Discussed with Jeen and she said we should use thi Show quoted text On 2017/08/18 15:03:53, juliandoucette wrote: > NIT: Isn't this the wrong icon? (The one in the spec looks better IMO) Discussed with Jeen and she said we should use this icon across the websites ire 2017/08/21 15:23:18 It wasn't brought up before. I didn't intent to re Show quoted text On 2017/08/18 15:03:51, juliandoucette wrote: > Note: I'm assuming that you will replace "Image alt text" everywhere. > > (Sorry if this was already brought up) It wasn't brought up before. I didn't intent to replace it. Isn't this the "content" part of the identifier, which is supposed to describe what type of string it is (e.g. heading, paragraph, image alt text)? juliandoucette 2017/08/21 16:28:38 Yup. I seem to have mistaken this for the alt text Show quoted text On 2017/08/21 15:23:18, ire wrote: > On 2017/08/18 15:03:51, juliandoucette wrote: > > Note: I'm assuming that you will replace "Image alt text" everywhere. > > > > (Sorry if this was already brought up) > > It wasn't brought up before. I didn't intent to replace it. Isn't this the > "content" part of the identifier, which is supposed to describe what type of > string it is (e.g. heading, paragraph, image alt text)? Yup. I seem to have mistaken this for the alt text itself.... Wow :D ire 2017/08/22 12:15:15 Haha :p Show quoted text On 2017/08/21 16:28:38, juliandoucette wrote: > On 2017/08/21 15:23:18, ire wrote: > > On 2017/08/18 15:03:51, juliandoucette wrote: > > > Note: I'm assuming that you will replace "Image alt text" everywhere. > > > > > > (Sorry if this was already brought up) > > > > It wasn't brought up before. I didn't intent to replace it. Isn't this the > > "content" part of the identifier, which is supposed to describe what type of > > string it is (e.g. heading, paragraph, image alt text)? > > Yup. I seem to have mistaken this for the alt text itself.... Wow :D Haha :p
17 </a>	26 </a>

18 </nav>	27 </nav>

19 </div>	28 </div>

20 </div>	29 </div>

21 </header>	30 </header>

LEFT	RIGHT