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

Side by Side Diff

Use n/p to move between diff chunks; N/P to move between comments.

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Side by Side Diff: includes/layout/header.tmpl

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

Patch Set: Fix alignment of search form, reformat JS Created Aug. 22, 2017, 12:14 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <header id="site-header" class="navbar">

	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">

	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") }}">

	6 </a>

	7 </h1>

	8 <button id="toggle-search-form" class="unstyled tablet-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 </button>

	11 <div id="site-search">
	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 ?>
	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.
	13 <nav id="product-website-link">

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

	15 {{ "Adblock Plus" \| translate("abp-product-link", "Link") }}
	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>

	18 </nav>

	19 </div>

	20 </div>

	21 </header>

OLD	NEW

« no previous file with comments | « no previous file | includes/search-form.tmpl » ('j') | includes/search-form.tmpl » ('J')