includes/components/navbar.html - Issue 29587584: Issue 5635 - Implement website-default navbar component

Side by Side Diff: includes/components/navbar.html

Issue 29587584: Issue 5635 - Implement website-default navbar component (Closed) Base URL: https://hg.adblockplus.org/website-defaults

Patch Set: Add navigation <ul> links Created Nov. 10, 2017, 10:22 a.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 <div class="navbar bg-primary">

	2 <div class="container">

	3 <a href="index" class="navbar-branding float-start">

	4 <img src="/img/png/eyeo-logo.png" srcset="/img/svg/eyeo-logo.svg 2x" alt=" eyeo Website Defaults">
	juliandoucette 2017/11/10 11:42:50 SuperNIT: The image is about eyeo and the alt text SuperNIT: The image is about eyeo and the alt text is about website-defaults. I think the alt text should be about the image. juliandoucette 2017/11/10 11:42:51 This image is not aligned vertically center within This image is not aligned vertically center within this <a> ire 2017/11/13 17:57:38 Done. Show quoted text On 2017/11/10 11:42:51, juliandoucette wrote: > This image is not aligned vertically center within this <a> Done. ire 2017/11/13 17:57:39 Done. Show quoted text On 2017/11/10 11:42:50, juliandoucette wrote: > SuperNIT: The image is about eyeo and the alt text is about website-defaults. I > think the alt text should be about the image. Done.
	5 </a>

	6

	7 <div class="toggle-navbar-collapse float-end">
	juliandoucette 2017/11/10 11:42:51 NIT: "toggle-navbar-collapse" (verb-noun) is incon NIT: "toggle-navbar-collapse" (verb-noun) is inconsistent with "navbar-collapse" (noun-verb). I suggest "navbar-toggle" (Ack this is a little awkward either way) ire 2017/11/13 17:57:39 It would have to be navbar-collapse-toggle since t Show quoted text On 2017/11/10 11:42:51, juliandoucette wrote: > NIT: "toggle-navbar-collapse" (verb-noun) is inconsistent with "navbar-collapse" > (noun-verb). I suggest "navbar-toggle" (Ack this is a little awkward either way) It would have to be navbar-collapse-toggle since that it what it's targeting, not the whole navbar. I'm indifferent about whether it's better, do you think I should change to that? juliandoucette 2017/11/14 15:34:16 Ack :/ Show quoted text On 2017/11/13 17:57:39, ire wrote: > It would have to be navbar-collapse-toggle since that it what it's targeting, > not the whole navbar. Ack :/ Show quoted text > I'm indifferent about whether it's better, do you think I should change to that? Yes
	8 <button class="unstyled">

	9 <span class="open-label">Open</span>
	juliandoucette 2017/11/10 11:42:50 NIT/Suggest: "navbar-open" and "navbar-close" woul NIT/Suggest: "navbar-open" and "navbar-close" would apply more generally to an image or text juliandoucette 2017/11/10 11:42:51 NIT/Suggest: Use an image example instead of a tex NIT/Suggest: Use an image example instead of a text example. e.g. a hamburger that turns into an X or just a hamburger. ire 2017/11/13 17:57:39 1. It would have to be `navbar-collapse-open`/clos Show quoted text On 2017/11/10 11:42:50, juliandoucette wrote: > NIT/Suggest: "navbar-open" and "navbar-close" would apply more generally to an > image or text 1. It would have to be `navbar-collapse-open`/close since that's what it is targeting 2. I'm not sure `navbar-open` describes what this is. It seems like a class we would apply to the navbar when it is open to me 3. I think a label can apply to an image as well ire 2017/11/13 17:57:40 I think this is something that can/should be handl Show quoted text On 2017/11/10 11:42:51, juliandoucette wrote: > NIT/Suggest: Use an image example instead of a text example. e.g. a hamburger > that turns into an X or just a hamburger. I think this is something that can/should be handled for each site. For example the help center wouldn't use it. juliandoucette 2017/11/14 15:34:16 Ack :/ (Proceed as-is.) Show quoted text On 2017/11/13 17:57:39, ire wrote: > 1. It would have to be `navbar-collapse-open`/close since that's what it is > targeting > 2. I'm not sure `navbar-open` describes what this is. It seems like a class we > would apply to the navbar when it is open to me > 3. I think a label can apply to an image as well Ack :/ (Proceed as-is.) juliandoucette 2017/11/14 15:34:16 Ack, good point. I was thinking "the help centre u Show quoted text On 2017/11/13 17:57:40, ire wrote: > I think this is something that can/should be handled for each site. For example > the help center wouldn't use it. Ack, good point. I was thinking "the help centre uses an image too - so an image is better than text." But text is fine as long as we don't repeat ourselves overwriting it in the same way across websites (at which point we should probably re-think this).
	10 <span class="sr-only">/</span>

	11 <span class="close-label">Close</span>

	12 Menu

	13 </button>

	14 </div>

	15

	16 <nav class="nav navbar-collapse">
	juliandoucette 2017/11/10 11:42:51 NIT: It's inconsistent not to preface this "nav" c NIT: It's inconsistent not to preface this "nav" class with "navbar" ire 2017/11/13 17:57:39 Done. Show quoted text On 2017/11/10 11:42:51, juliandoucette wrote: > NIT: It's inconsistent not to preface this "nav" class with "navbar" Done.
	17 <ul>

	18 <li><a href="index">Navbar Menu Link</a></li>

	19 <li><a href="index">Navbar Menu Link</a></li>

	20 <li><a href="index">Navbar Menu Link</a></li>

	21 <li><a href="index">Navbar Menu Link</a></li>

	22 </ul>

	23 </nav>

	24 </div>

	25 </div>

OLD	NEW

« no previous file with comments | « no previous file | pages/index.html » ('j') | pages/navbar.md » ('J')