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

Delta Between Two Patch Sets: includes/components/navbar.html

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

Left Patch Set: Add navigation <ul> links Created Nov. 10, 2017, 10:22 a.m.

Right Patch Set: Rebase Created Jan. 4, 2018, 7:40 a.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 <div class="navbar bg-primary">	1 <div class="navbar bg-primary">

2 <div class="container">	2 <div class="navbar-container">

3 <a href="index" class="navbar-branding float-start">	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">	4 <img src="/img/png/eyeo-logo.png" srcset="/img/svg/eyeo-logo.svg 2x" alt=" eyeo">
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>	5 </a>

6	6

7 <div class="toggle-navbar-collapse float-end">	7 <button class="toggle-navbar-collapse button-link 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">	8 Toggle Menu

9 <span class="open-label">Open</span>	9 </button>
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	10

16 <nav class="nav navbar-collapse">	11 <nav class="navbar-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>	12 <ul>

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

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

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

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

22 </ul>	17 </ul>

23 </nav>	18 </nav>

24 </div>	19 </div>

25 </div>	20 </div>

LEFT	RIGHT