Issue 29587584: Issue 5635 - Implement website-default navbar component (Closed)
|
Created:
Oct. 24, 2017, 10:32 a.m. by ire Modified:
Jan. 9, 2018, 8 a.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 5635 - Implement website-default navbar component
Patch Set 1 #
Total comments: 26
Patch Set 2 : Rebased #
Total comments: 31
Patch Set 3 : Demo - Use table layout #
Total comments: 12
Patch Set 4 : Demo of both methods #
Total comments: 7
Patch Set 5 : Use float method #Patch Set 6 : Add navigation <ul> links #
Total comments: 39
Patch Set 7 : Addressed NITs #
Total comments: 10
Patch Set 8 : Use padding on navbar link, optimise svg #
Total comments: 8
Patch Set 9 : Remove separate open/close states #
Total comments: 6
Patch Set 10 : Variable for adding horizontal padding, link button class #
Total comments: 16
Patch Set 11 : Address scalability of navbar padding with logo size #Patch Set 12 : Rebase #
MessagesTotal messages: 46
Ready for review
Initial feedback. I didn't review _navbar.scss. Looks like we need to agree on the functionality of the MVP and then create issues to address other features separately (or not). https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:1: <div class="navbar"> suggest: navbar.tmpl (see template usage examples below) https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:2: <div class="container"> NIT: I suggest we point out that .container is optional via template comment {# #} https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:3: <h1 class="navbar-branding"> suggest: "navbar-brand" https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:3: <h1 class="navbar-branding"> Note: I'm not sure that an h1 is appropriate https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:4: <a href="index">Website Defaults</a> suggest: make this more extensible e.g. {{ "index" | linkify }} {% if navbar_logo %} ... {% if navbar_logo_text %} ... </a> https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:7: <button class="toggle-navbar-collapse"> suggest: "navbar-toggle" https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:8: <span class="open-label">Open</span> suggest: a shared hamberger icon https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:13: suggest: enableable navbar search https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:16: <a href="index">Navbar Menu Link</a> suggest: loop an array of pages and external links https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:16: <a href="index">Navbar Menu Link</a> suggest: More than one example link https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:19: </div> suggest: enableable locale menu (separate include - so that we could use it in the footer instead) https://codereview.adblockplus.org/29587584/diff/29587585/pages/navbar.md File pages/navbar.md (right): https://codereview.adblockplus.org/29587584/diff/29587585/pages/navbar.md#new... pages/navbar.md:10: Default navbar component for eyeo websites. NIT: Is the word "component" necessary here? https://codereview.adblockplus.org/29587584/diff/29587585/pages/navbar.md#new... pages/navbar.md:14: 1. Copy Navbar HTML from **includes/components/navbar.html** suggest: Use a configurable include template instead of suggesting copy/paste https://codereview.adblockplus.org/29587584/diff/29587585/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/js/main.js#n... static/js/main.js:1: (function() NIT/Suggest: You can push this separately (sooner) https://codereview.adblockplus.org/29587584/diff/29587585/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component suggest: keyboard and aria functionality from [examlpe](https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.html) https://codereview.adblockplus.org/29587584/diff/29587585/static/js/navbar.js... static/js/navbar.js:15: .addEventListener("click", this._onClick.bind(this), false); suggest: Allow click away to close? https://codereview.adblockplus.org/29587584/diff/29587585/static/js/navbar.js... static/js/navbar.js:28: var navbars = document.getElementsByClassName("navbar"); NIT: separate var and for by one line https://codereview.adblockplus.org/29587584/diff/29587585/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/scss/_utilit... static/scss/_utilities.scss:46: /** NIT/Suggest: You can push this separately (sooner) https://codereview.adblockplus.org/29587584/diff/29587585/templates/minimal.tmpl File templates/minimal.tmpl (right): https://codereview.adblockplus.org/29587584/diff/29587585/templates/minimal.t... templates/minimal.tmpl:1: <!DOCTYPE html> NIT: It's unclear why this change was necessary.
https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29587585/includes/components... includes/components/navbar.html:2: <div class="container"> On 2017/10/26 15:55:54, juliandoucette wrote: > NIT: I suggest we point out that .container is optional via template comment {# > #} This is inconsistent with my surrounding suggestions. We could make this configurable, but I suggest we do that separately.
I think you're right that we do need to discuss what we want this component to be, because we seemingly have different ideas. It seems like you want it to be something that can be configured and included in sites. I don't really think it should be this way. IMO the navbar contents are too different across the sites to have one file that is altered with variables. I think it would be too limiting to use the same template and just make certain things enableable. For example having an enableable local menu would mean that whenever a locale menu is present, it would have to be in X position and look like Y (unless of course overrides, which would defeat the purpose?). The way I think it should work is to just have the barest structure and allow each site to modify it in its own way. https://codereview.adblockplus.org/29587584/diff/29587585/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/js/main.js#n... static/js/main.js:1: (function() On 2017/10/26 15:55:56, juliandoucette wrote: > NIT/Suggest: You can push this separately (sooner) See https://codereview.adblockplus.org/29592626 https://codereview.adblockplus.org/29587584/diff/29587585/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/scss/_utilit... static/scss/_utilities.scss:46: /** On 2017/10/26 15:55:56, juliandoucette wrote: > NIT/Suggest: You can push this separately (sooner) See https://codereview.adblockplus.org/29592630/ https://codereview.adblockplus.org/29587584/diff/29587585/templates/minimal.tmpl File templates/minimal.tmpl (right): https://codereview.adblockplus.org/29587584/diff/29587585/templates/minimal.t... templates/minimal.tmpl:1: <!DOCTYPE html> On 2017/10/26 15:55:56, juliandoucette wrote: > NIT: It's unclear why this change was necessary. The default template (as we agreed before) will always include .container.content This template only has the basic html tags needed.
> I don't really think it should be this way. IMO the navbar contents are too > different across the sites to have one file that is altered with variables. I > think it would be too limiting to use the same template and just make certain > things enableable. For example having an enableable local menu would mean that > whenever a locale menu is present, it would have to be in X position and look > like Y (unless of course overrides, which would defeat the purpose?). The way I > think it should work is to just have the barest structure and allow each site to > modify it in its own way. Agreed. I'll take another look at this with that in mind. https://codereview.adblockplus.org/29587584/diff/29587585/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/js/main.js#n... static/js/main.js:1: (function() On 2017/10/31 08:04:00, ire wrote: > On 2017/10/26 15:55:56, juliandoucette wrote: > > NIT/Suggest: You can push this separately (sooner) > > See https://codereview.adblockplus.org/29592626 Acknowledged. https://codereview.adblockplus.org/29587584/diff/29587585/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29587584/diff/29587585/static/scss/_utilit... static/scss/_utilities.scss:46: /** On 2017/10/31 08:04:00, ire wrote: > On 2017/10/26 15:55:56, juliandoucette wrote: > > NIT/Suggest: You can push this separately (sooner) > > See https://codereview.adblockplus.org/29592630/ Acknowledged. https://codereview.adblockplus.org/29587584/diff/29587585/templates/minimal.tmpl File templates/minimal.tmpl (right): https://codereview.adblockplus.org/29587584/diff/29587585/templates/minimal.t... templates/minimal.tmpl:1: <!DOCTYPE html> On 2017/10/31 08:04:00, ire wrote: > The default template (as we agreed before) will always include > .container.content > > This template only has the basic html tags needed. That makes sense. Thank you for explaining.
Rebased
https://codereview.adblockplus.org/29587584/diff/29593565/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29593565/includes/components... includes/components/navbar.html:3: <h1 class="navbar-branding"> I don't know whether it's better to let the site title be h1 or the page heading be h1 these days. Have we discussed this before? / Have you looked into this lately? https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component Are you planning to add accessibility features like keyboard navigation separately? (I haven't checked/confirmed that this is necessary - have you?) https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js... static/js/navbar.js:4: (function() TOL: I'm not sure that we have to worry about polluting the global name space with these https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:23: padding-top: $small-space; - This doesn't apply to new abp.org - This will prevent full-height hover/focus states within https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:25: color: $primary-light; Suggest: we apply a bg utility class in HTML or extend a utility class in CSS instead https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:27: @extend .clearfix; This may be overkill because the .container within is already clearfixed? https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:31: .navbar button See comment above about background utility classes. On that note, we should probably style links within background utility classes. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:39: .navbar-branding Perhaps a float utility class or and ID class would be more appropriate if there is nothing else unique about a navbar brand. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:64: float: left; What about text-align? https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:77: .js .toggle-navbar-collapse .close-label I think that we will always use an icon; not a label. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:97: display: block; I think that this may be unnecessary because we are unlikely to add .navbar-collapse to an inline or inline-block element? https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:99: width: 100%; I think that this may be unnecessary because this element is probably already display:block; https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:100: padding-top: $small-space; I think that this may be unnecessary because the padding-top will probably be provided by the margin of the child elements. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:106: width: auto; I think that this will become unnecessary if I am right about the width above being unnecessary. The same applies to the line below (except about padding not width). https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:126: .expanded .navbar-collapse Suggest: Use aria expanded (unless you want to do that separately)
https://codereview.adblockplus.org/29587584/diff/29593565/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29593565/includes/components... includes/components/navbar.html:3: <h1 class="navbar-branding"> On 2017/10/31 15:18:37, juliandoucette wrote: > I don't know whether it's better to let the site title be h1 or the page heading > be h1 these days. Have we discussed this before? / Have you looked into this > lately? AFAICT page heading should be <h1> and navbar brand should be <a>. But I haven't found a reliable source.
This is kinda an experiment/demo of using table layout instead of my previous method of floats. I wanted to do this for vertical alignment. If we have differing heights of elements (e.g. the logo and the menu text), they weren't being centered vertically in my previous method. One issue I still can't find a solution to with this is the navbar-collapse element on mobile when it's expanded. For some reason the contents don't go full-width (see that the navbar links break). I can't figure out why its happening. Do you have any ideas? And what do you think of this method in general? https://codereview.adblockplus.org/29587584/diff/29593565/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29593565/includes/components... includes/components/navbar.html:3: <h1 class="navbar-branding"> On 2017/10/31 15:30:44, juliandoucette wrote: > On 2017/10/31 15:18:37, juliandoucette wrote: > > I don't know whether it's better to let the site title be h1 or the page > heading > > be h1 these days. Have we discussed this before? / Have you looked into this > > lately? > > AFAICT page heading should be <h1> and navbar brand should be <a>. But I haven't > found a reliable source. I think you're right. I haven't found a documented source either, but I spoke to a friend of mine who's an accessibility expert about it and he says <h1> should describe the unique content of the page. https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component On 2017/10/31 15:18:37, juliandoucette wrote: > Are you planning to add accessibility features like keyboard navigation > separately? (I haven't checked/confirmed that this is necessary - have you?) I haven't done much research either, but I would prefer to handle that in a separate issue. That's the only thing I can think of that is missing is trapping the keyboard focus within the collapsed section when it's open, but I'll look into it more. https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js... static/js/navbar.js:4: (function() On 2017/10/31 15:18:37, juliandoucette wrote: > TOL: I'm not sure that we have to worry about polluting the global name space > with these You're probably right. I'll remove it. https://codereview.adblockplus.org/29587584/diff/29593565/static/js/navbar.js... static/js/navbar.js:4: (function() On 2017/10/31 15:18:37, juliandoucette wrote: > TOL: I'm not sure that we have to worry about polluting the global name space > with these Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:25: color: $primary-light; On 2017/10/31 15:18:38, juliandoucette wrote: > Suggest: we apply a bg utility class in HTML or extend a utility class in CSS > instead Great idea! Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:27: @extend .clearfix; On 2017/10/31 15:18:38, juliandoucette wrote: > This may be overkill because the .container within is already clearfixed? .container isn't clearfixed? https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:31: .navbar button On 2017/10/31 15:18:38, juliandoucette wrote: > See comment above about background utility classes. On that note, we should > probably style links within background utility classes. Links are already styled because of the default `color: inherit`. The problem here is the button, but i'm not sure buttons should be (by default) styled for the background utility classes. This seems more like something specific to the navbar. We may not even have text in the navbar button, it'll likely be an icon. For now I'm setting `color: inherit` on the navbar button. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:39: .navbar-branding On 2017/10/31 15:18:37, juliandoucette wrote: > Perhaps a float utility class or and ID class would be more appropriate if there > is nothing else unique about a navbar brand. Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:64: float: left; On 2017/10/31 15:18:37, juliandoucette wrote: > What about text-align? The other text-align isn't actually needed. So I'm removing it. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:77: .js .toggle-navbar-collapse .close-label On 2017/10/31 15:18:38, juliandoucette wrote: > I think that we will always use an icon; not a label. We could have separate icons for open and close? Or are you suggesting that the name be close-icon instead of close-label? https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:97: display: block; On 2017/10/31 15:18:37, juliandoucette wrote: > I think that this may be unnecessary because we are unlikely to add > .navbar-collapse to an inline or inline-block element? Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:99: width: 100%; On 2017/10/31 15:18:38, juliandoucette wrote: > I think that this may be unnecessary because this element is probably already > display:block; Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:100: padding-top: $small-space; On 2017/10/31 15:18:37, juliandoucette wrote: > I think that this may be unnecessary because the padding-top will probably be > provided by the margin of the child elements. Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:106: width: auto; On 2017/10/31 15:18:38, juliandoucette wrote: > I think that this will become unnecessary if I am right about the width above > being unnecessary. > > The same applies to the line below (except about padding not width). Done. https://codereview.adblockplus.org/29587584/diff/29593565/static/scss/_navbar... static/scss/_navbar.scss:126: .expanded .navbar-collapse On 2017/10/31 15:18:37, juliandoucette wrote: > Suggest: Use aria expanded (unless you want to do that separately) I think I will do that separately when we implement aria-haspopup or whatever the correct role it would be here https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... includes/components/navbar.html:3: <a href="index" class="navbar-branding text-start">Website<br> Defaults</a> I added this line break to simulate the height of a logo being different from the menu https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... includes/components/navbar.html:5: <div class="toggle-navbar-collapse text-end"> Had to add a container because display: table-cell doesn't work on buttons https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... includes/components/navbar.html:16: <a href="index">Navbar Menu Link</a>, <a href="index">Navbar Menu Link</a>, <a href="index">Navbar Menu Link</a>, <a href="index">Navbar Menu Link</a> Added more links for testing, even though I know it won't ever look like this. I will change in later patch https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_navbar... static/scss/_navbar.scss:24: width: 100%; For some reason this didn't work when it was the other way around, i.e. $large-desktop-width as `width` and 100% as `max-width`. If we decide to go with this method for the .navbar, we can just change the .container to work this way instead since it's essentially the same for the other elements. https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_navbar... static/scss/_navbar.scss:125: .expanded .navbar-collapse:after This is the only way I could think of to add bottom spacing to the navbar-collapse
RE: <h1> and site title vs page title, you may be interested in this thread https://twitter.com/ireaderinokun/status/926018770426097665
> This is kinda an experiment/demo of using table layout instead of my previous > method of floats. I wanted to do this for vertical alignment. If we have > differing heights of elements (e.g. the logo and the menu text), they weren't > being centered vertically in my previous method. Can you create a demo of this so that we can experiment? > One issue I still can't find a solution to with this is the navbar-collapse > element on mobile when it's expanded. For some reason the contents don't go > full-width (see that the navbar links break). I can't figure out why its > happening. Do you have any ideas? The nav seems to be limited to the width of the first column within it's parent row (.navbar-collapse). I don't know how to get around this (off the top of my head). > And what do you think of this method in general? I'm skeptical. If I understand you correctly, you want to be able to vertically centre navigation items that are less tall than the navbar logo. I would suggest using margin if the height of the navbar logo is known. If not, I would suggest using absolute positioning. (I am probably bias because I have spent many hours frustrated about table spacing and responsiveness.) https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... includes/components/navbar.html:3: <a href="index" class="navbar-branding text-start">Website<br> Defaults</a> On 2017/11/02 11:15:28, ire wrote: > I added this line break to simulate the height of a logo being different from > the menu Acknowledged. https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... includes/components/navbar.html:5: <div class="toggle-navbar-collapse text-end"> On 2017/11/02 11:15:28, ire wrote: > Had to add a container because display: table-cell doesn't work on buttons Acknowledged. https://codereview.adblockplus.org/29587584/diff/29595567/includes/components... includes/components/navbar.html:16: <a href="index">Navbar Menu Link</a>, <a href="index">Navbar Menu Link</a>, <a href="index">Navbar Menu Link</a>, <a href="index">Navbar Menu Link</a> On 2017/11/02 11:15:28, ire wrote: > Added more links for testing, even though I know it won't ever look like this. I > will change in later patch Acknowledged. https://codereview.adblockplus.org/29587584/diff/29595567/pages/navbar.md File pages/navbar.md (right): https://codereview.adblockplus.org/29587584/diff/29595567/pages/navbar.md#new... pages/navbar.md:12: ## How to Use: suggest: Add this separately because other website-default components do not have a "How to Use" and we should be consistent https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_navbar... static/scss/_navbar.scss:24: width: 100%; On 2017/11/02 11:15:28, ire wrote: > For some reason this didn't work when it was the other way around, i.e. > $large-desktop-width as `width` and 100% as `max-width`. > > If we decide to go with this method for the .navbar, we can just change the > .container to work this way instead since it's essentially the same for the > other elements. Acknowledged. https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_navbar... static/scss/_navbar.scss:125: .expanded .navbar-collapse:after On 2017/11/02 11:15:28, ire wrote: > This is the only way I could think of to add bottom spacing to the > navbar-collapse You could add padding to the nav within instead. https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29587584/diff/29595567/static/scss/_utilit... static/scss/_utilities.scss:66: /* Floats These don't seem to be used.
On 2017/11/02 11:18:13, ire wrote: > RE: <h1> and site title vs page title, you may be interested in this thread > https://twitter.com/ireaderinokun/status/926018770426097665 *Like* PS: Epic twitter
On 2017/11/02 13:38:27, juliandoucette wrote: > On 2017/11/02 11:18:13, ire wrote: > > RE: <h1> and site title vs page title, you may be interested in this thread > > https://twitter.com/ireaderinokun/status/926018770426097665 > > *Like* > > PS: Epic twitter Haha :P
I created a demo of both methods. I just namespaced them so we can compare the difference. Let me know your thoughts.
> I created a demo of both methods. I just namespaced them so we can compare the > difference. Let me know your thoughts. Thanks :) - The floating pattern seems a little simpler to me - I don't know how to solve the table row column width issue - Have you found any issues using padding and fixed height to align the navigation items vertically (using the floating pattern)? e.g. [logo 2em tall] -- [text 1em tall padding 0.5em top/bottom] etc. https://codereview.adblockplus.org/29587584/diff/29599558/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29599558/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component BUG: You can't collapse the navbar if you expanded it while the window was resized down and then resized back up. https://codereview.adblockplus.org/29587584/diff/29599558/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29599558/static/scss/_navbar... static/scss/_navbar.scss:28: #table-navbar .navbar button NIT: Perhaps button should normally inherit color? https://codereview.adblockplus.org/29587584/diff/29599558/static/scss/_navbar... static/scss/_navbar.scss:61: #table-navbar .toggle-navbar-collapse button TOL: Perhaps we could use (extend) a (link button | unstyled button) instead.
On 2017/11/06 12:36:44, juliandoucette wrote: > > I created a demo of both methods. I just namespaced them so we can compare the > > difference. Let me know your thoughts. > > Thanks :) > > - The floating pattern seems a little simpler to me > - I don't know how to solve the table row column width issue > - Have you found any issues using padding and fixed height to align the > navigation items vertically (using the floating pattern)? > > e.g. > > [logo 2em tall] -- [text 1em tall padding 0.5em top/bottom] etc. I agree that the floating pattern is probably simpler/cleaner. I decided to go with that and use your suggestion of calculating padding to space the nav in the vertical middle. https://codereview.adblockplus.org/29587584/diff/29599558/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29599558/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component On 2017/11/06 12:36:43, juliandoucette wrote: > BUG: You can't collapse the navbar if you expanded it while the window was > resized down and then resized back up. I can't seem to reproduce this. If the navbar is opened and you resize the window up, it shows as normal. Could you give me some more information? https://codereview.adblockplus.org/29587584/diff/29599558/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29599558/static/scss/_navbar... static/scss/_navbar.scss:28: #table-navbar .navbar button On 2017/11/06 12:36:43, juliandoucette wrote: > NIT: Perhaps button should normally inherit color? Done. https://codereview.adblockplus.org/29587584/diff/29599558/static/scss/_navbar... static/scss/_navbar.scss:61: #table-navbar .toggle-navbar-collapse button On 2017/11/06 12:36:43, juliandoucette wrote: > TOL: Perhaps we could use (extend) a (link button | unstyled button) instead. Done.
How are you planning to handle block/100% navbar menu items? https://codereview.adblockplus.org/29587584/diff/29599558/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29599558/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component On 2017/11/08 17:14:46, ire wrote: > On 2017/11/06 12:36:43, juliandoucette wrote: > > BUG: You can't collapse the navbar if you expanded it while the window was > > resized down and then resized back up. > > I can't seem to reproduce this. If the navbar is opened and you resize the > window up, it shows as normal. Could you give me some more information? Just re-tested. This only applied to the table layout. Sorry for the confusion.
On 2017/11/09 14:34:52, juliandoucette wrote: > How are you planning to handle block/100% navbar menu items? (On mobile)
On 2017/11/09 14:35:25, juliandoucette wrote: > On 2017/11/09 14:34:52, juliandoucette wrote: > > How are you planning to handle block/100% navbar menu items? > > (On mobile) I expected that each site would handle how the nav items themselves work, since they'll likely be different. But perhaps we can have a simple example here, an <ul> of links which are side-by-side on desktop and full-width on mobile. I'll do that now to illustrate.
On 2017/11/10 10:06:58, ire wrote: > On 2017/11/09 14:35:25, juliandoucette wrote: > > On 2017/11/09 14:34:52, juliandoucette wrote: > > > How are you planning to handle block/100% navbar menu items? > > > > (On mobile) > > I expected that each site would handle how the nav items themselves work, since > they'll likely be different. But perhaps we can have a simple example here, an > <ul> of links which are side-by-side on desktop and full-width on mobile. I'll > do that now to illustrate. Done. New patch set uploaded.
Regarding the padding and active/hover/focus states that I suggested: I think the answer depends on whether or not these improve the help centre. These are already a part of the new adblockplus.org and I think that they will be part of other websites. I think that these suggestions could also improve the help centre. e.g. active/hover/focus background color on the eyeo logo, the website link floating right, and even around the black space around search field (it would make it pop out more when you hover/focus). https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:4: <img src="/img/png/eyeo-logo.png" srcset="/img/svg/eyeo-logo.svg 2x" alt="eyeo Website Defaults"> SuperNIT: The image is about eyeo and the alt text is about website-defaults. I think the alt text should be about the image. https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:4: <img src="/img/png/eyeo-logo.png" srcset="/img/svg/eyeo-logo.svg 2x" alt="eyeo Website Defaults"> This image is not aligned vertically center within this <a> https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:7: <div class="toggle-navbar-collapse float-end"> 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) https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:9: <span class="open-label">Open</span> NIT/Suggest: Use an image example instead of a text example. e.g. a hamburger that turns into an X or just a hamburger. https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:9: <span class="open-label">Open</span> NIT/Suggest: "navbar-open" and "navbar-close" would apply more generally to an image or text https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:16: <nav class="nav navbar-collapse"> NIT: It's inconsistent not to preface this "nav" class with "navbar" https://codereview.adblockplus.org/29587584/diff/29603563/pages/navbar.md File pages/navbar.md (right): https://codereview.adblockplus.org/29587584/diff/29603563/pages/navbar.md#new... pages/navbar.md:16: 1. Copy Navbar JavaScript from **static/js/navbar.js** NIT: Extra whitespace at the end of this line https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... File static/img/svg/eyeo-logo.svg (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... static/img/svg/eyeo-logo.svg:1: <?xml version="1.0" encoding="UTF-8"?> Detail: Don't forget to optimize this svg and png https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:29: padding-top: $navbar-padding-v + ($navbar-padding-v / 2); I think that this assumes that .toggle-navbar-collapse height is ( $navbar-branding-height - $navbar-padding-v ) because the total navbar height seems to be $navbar-branding-height + ( 2 * $navbar-padding-v ). If I am correct then you are assuming a height instead of setting a height and this will break/(be not helpful) if that height changes in a child website. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:34: ****************************************************************************/ NIT: There is supposed to be one space before the first "*" so that the first line "*" and the second line "*" align. And the line is supposed to be consistently 79 or 80 characters long. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:36: .navbar-branding NIT/Suggest: Add padding-x to the navbar-branding so that the focus box doesn't hug the sides of the logo image/text (e.g. margin-start: -1em; padding-start: 1em - like new abp.org) https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:36: .navbar-branding Suggest: Add a hover/active/focus background color https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:50: .toggle-navbar-collapse TOL: Are these properties that should apply to `.unstyled`? If so, toggle-navbar-collapse could extend .unstyled. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:83: /* .navbar .navbar-collapse Suggest: Add padding/space to the <a> themselves instead of their containers to make the navbar links more like buttons and less like links. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:83: /* .navbar .navbar-collapse Suggest: Add an active/hover/focus background color to <a> https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:119: /* .navbar .nav NIT/Suggest: Make the whole line and space above/below items clickable on mobile (like custom-select) (Make the <a> block and add padding above/below). https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_utilit... static/scss/_utilities.scss:66: /* Floats Detail: You can add this separately if you want https://codereview.adblockplus.org/29587584/diff/29603563/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29587584/diff/29603563/templates/default.t... templates/default.tmpl:1: {% extends "templates/minimal" %} Detail: You can add this change and minimal.tmpl separately if you want.
On 2017/11/10 11:42:54, juliandoucette wrote: > Regarding the padding and active/hover/focus states that I suggested: I think > the answer depends on whether or not these improve the help centre. These are > already a part of the new http://adblockplus.org and I think that they will be part of > other websites. I think that these suggestions could also improve the help > centre. e.g. active/hover/focus background color on the eyeo logo, the website > link floating right, and even around the black space around search field (it > would make it pop out more when you hover/focus). I'm not 100% sure about this. I don't really think the style of having an active/hover/focus background colour is how the Help Center was meant to be. I do see how it works for abp, but I don't necessarily think that all our other sites would be better having it work this way. I also don't think we should change the way one site works in order to use the same component across multiple sites. I don't know what the solution is here. I suggest we ask Jeen what she thinks on this. https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:4: <img src="/img/png/eyeo-logo.png" srcset="/img/svg/eyeo-logo.svg 2x" alt="eyeo Website Defaults"> 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. https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:4: <img src="/img/png/eyeo-logo.png" srcset="/img/svg/eyeo-logo.svg 2x" alt="eyeo Website Defaults"> On 2017/11/10 11:42:51, juliandoucette wrote: > This image is not aligned vertically center within this <a> Done. https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:7: <div class="toggle-navbar-collapse float-end"> 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? https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:9: <span class="open-label">Open</span> 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. https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:9: <span class="open-label">Open</span> 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 https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:16: <nav class="nav navbar-collapse"> On 2017/11/10 11:42:51, juliandoucette wrote: > NIT: It's inconsistent not to preface this "nav" class with "navbar" Done. https://codereview.adblockplus.org/29587584/diff/29603563/pages/navbar.md File pages/navbar.md (right): https://codereview.adblockplus.org/29587584/diff/29603563/pages/navbar.md#new... pages/navbar.md:16: 1. Copy Navbar JavaScript from **static/js/navbar.js** On 2017/11/10 11:42:51, juliandoucette wrote: > NIT: Extra whitespace at the end of this line Done. https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... File static/img/svg/eyeo-logo.svg (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... static/img/svg/eyeo-logo.svg:1: <?xml version="1.0" encoding="UTF-8"?> On 2017/11/10 11:42:52, juliandoucette wrote: > Detail: Don't forget to optimize this svg and png I will optimise the png, but we don't currently optimise svgs manually? https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:29: padding-top: $navbar-padding-v + ($navbar-padding-v / 2); On 2017/11/10 11:42:52, juliandoucette wrote: > I think that this assumes that .toggle-navbar-collapse height is ( > $navbar-branding-height - $navbar-padding-v ) because the total navbar height > seems to be $navbar-branding-height + ( 2 * $navbar-padding-v ). > > If I am correct then you are assuming a height instead of setting a height and > this will break/(be not helpful) if that height changes in a child website. You're right. The .toggle-navbar-collapse should actually be the <button> itself (which I've changed). This way, the height of the .toggle-navbar-collapse is the same as the .navbar itself. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:34: ****************************************************************************/ On 2017/11/10 11:42:52, juliandoucette wrote: > NIT: There is supposed to be one space before the first "*" so that the first > line "*" and the second line "*" align. And the line is supposed to be > consistently 79 or 80 characters long. Done. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:50: .toggle-navbar-collapse On 2017/11/10 11:42:52, juliandoucette wrote: > TOL: Are these properties that should apply to `.unstyled`? If so, > toggle-navbar-collapse could extend .unstyled. Done. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:83: /* .navbar .navbar-collapse On 2017/11/10 11:42:53, juliandoucette wrote: > Suggest: Add padding/space to the <a> themselves instead of their containers to > make the navbar links more like buttons and less like links. Do we want them to look more like buttons? I know we do on abp but I don't think this is the case for help center https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_navbar... static/scss/_navbar.scss:83: /* .navbar .navbar-collapse On 2017/11/10 11:42:53, juliandoucette wrote: > Suggest: Add an active/hover/focus background color to <a> Same as previously answer. background-color applies only if we want it to not look like a regular link. https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/scss/_utilit... static/scss/_utilities.scss:66: /* Floats On 2017/11/10 11:42:53, juliandoucette wrote: > Detail: You can add this separately if you want Done. https://codereview.adblockplus.org/29587584/diff/29603563/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29587584/diff/29603563/templates/default.t... templates/default.tmpl:1: {% extends "templates/minimal" %} On 2017/11/10 11:42:54, juliandoucette wrote: > Detail: You can add this change and minimal.tmpl separately if you want. Done.
> I'm not 100% sure about this. I don't really think the style of having an > active/hover/focus background colour is how the Help Center was meant to be. It's not how the help centre was meant to be (I didn't mean to give that impression). > I do see how it works for abp, but I don't necessarily think that all our other > sites would be better having it work this way. Why not? > I also don't think we should change the way one site works in order to use the > same component across multiple sites. (Unless that change makes sense across multiple sites?) > I don't know what the solution is here. I suggest we ask Jeen what she thinks on this. Agreed. PS: I don't know your opinion. Do you think that gray navbar item hover states will improve the help center? https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:7: <div class="toggle-navbar-collapse float-end"> 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 :/ > I'm indifferent about whether it's better, do you think I should change to that? Yes https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:9: <span class="open-label">Open</span> 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.) https://codereview.adblockplus.org/29587584/diff/29603563/includes/components... includes/components/navbar.html:9: <span class="open-label">Open</span> 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). https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... File static/img/svg/eyeo-logo.svg (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... static/img/svg/eyeo-logo.svg:1: <?xml version="1.0" encoding="UTF-8"?> On 2017/11/13 17:57:40, ire wrote: > I will optimise the png, but we don't currently optimise svgs manually? Technically we optimize both manually. I've been using svggo / https://jakearchibald.github.io/svgomg/ for SVG (perhaps we should create a shared svggo config for this).
message: On 2017/11/14 15:34:18, juliandoucette wrote: > > I'm not 100% sure about this. I don't really think the style of having an > > active/hover/focus background colour is how the Help Center was meant to be. > > It's not how the help centre was meant to be (I didn't mean to give that > impression). Ack. I didn't have that impression (and didn't mean to give you the impression that I had that impression :P). I meant that I don't think this change is in keeping with the design of the help center. > > I do see how it works for abp, but I don't necessarily think that all our > other > > sites would be better having it work this way. > > Why not? I think having the hover/focus/active background colour is a design choice. I don't think it's necessarily better, and I think having the links just look like plain links (i.e. not similar to buttons like abp) is equally as good. > > I also don't think we should change the way one site works in order to use the > > same component across multiple sites. > > (Unless that change makes sense across multiple sites?) Yes of course :) > > I don't know what the solution is here. I suggest we ask Jeen what she thinks > on this. > > Agreed. > > PS: I don't know your opinion. Do you think that gray navbar item hover states > will improve the help center? Going back to what I said above, I don't think it will make it better, just different. I do think that having some hover/focus/active state would be beneficial, but I don't think that using the same "button-like" design as abp will make it better.
On 2017/11/15 06:41:31, ire wrote: > Going back to what I said above, I don't think it will make it better, just > different. I do think that having some hover/focus/active state would be > beneficial, but I don't think that using the same "button-like" design as abp > will make it better. Ah man :( Let's proceed as-is then and address this separately if necessary.
https://codereview.adblockplus.org/29587584/diff/29606585/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29606585/includes/components... includes/components/navbar.html:7: <button class="toggle-navbar-collapse float-end"> NIT: It seems inconsistent that you use .float-end here and not below for navbar-nav/navbar-collapse. https://codereview.adblockplus.org/29587584/diff/29606585/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29606585/static/js/navbar.js... static/js/navbar.js:8: function Navbar(navbar) bug: If you resize down, open the collapse, then resize up: the collapse stays open and the close button disappears https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/_navbar... static/scss/_navbar.scss:125: margin-bottom: $navbar-padding-v; It's inconvenient that you chose to use margin-bottom here instead of padding top and bottom because this margin must be overridden in order to create a list with background-color hover state. Margin-bottom works well for this example because the navbar and the navbar-collapse are the same color. If they were not, then there would obviously be space missing above the first link. https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/_navbar... static/scss/_navbar.scss:143: .navbar .navbar-nav li:last-child Why use the paren't padding instead of the child's margin here? https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/forms/_... File static/scss/forms/_buttons.scss (right): https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/forms/_... static/scss/forms/_buttons.scss:27: color: inherit; Detail: You can push this separately.
New patchset uploaded https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... File static/img/svg/eyeo-logo.svg (right): https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... static/img/svg/eyeo-logo.svg:1: <?xml version="1.0" encoding="UTF-8"?> On 2017/11/14 15:34:17, juliandoucette wrote: > On 2017/11/13 17:57:40, ire wrote: > > I will optimise the png, but we don't currently optimise svgs manually? > > Technically we optimize both manually. I've been using svggo / > https://jakearchibald.github.io/svgomg/ for SVG (perhaps we should create a > shared svggo config for this). Ack. I had thought from the last meeting we had that we were to work on a build process for optimising images, instead of doing them manually. I agree that we should create a shared svgo configuration. https://codereview.adblockplus.org/29587584/diff/29603563/static/img/svg/eyeo... static/img/svg/eyeo-logo.svg:1: <?xml version="1.0" encoding="UTF-8"?> On 2017/11/14 15:34:17, juliandoucette wrote: > On 2017/11/13 17:57:40, ire wrote: > > I will optimise the png, but we don't currently optimise svgs manually? > > Technically we optimize both manually. I've been using svggo / > https://jakearchibald.github.io/svgomg/ for SVG (perhaps we should create a > shared svggo config for this). Done. https://codereview.adblockplus.org/29587584/diff/29606585/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29606585/includes/components... includes/components/navbar.html:7: <button class="toggle-navbar-collapse float-end"> On 2017/11/15 12:56:42, juliandoucette wrote: > NIT: It seems inconsistent that you use .float-end here and not below for > navbar-nav/navbar-collapse. The reason I did this was because, for the .navbar-nav, it changes based on screen size. So it's only .float-end on desktop. https://codereview.adblockplus.org/29587584/diff/29606585/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29606585/static/js/navbar.js... static/js/navbar.js:8: function Navbar(navbar) On 2017/11/15 12:56:42, juliandoucette wrote: > bug: If you resize down, open the collapse, then resize up: the collapse stays > open and the close button disappears Done. https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/_navbar... static/scss/_navbar.scss:125: margin-bottom: $navbar-padding-v; On 2017/11/15 12:56:42, juliandoucette wrote: > It's inconvenient that you chose to use margin-bottom here instead of padding > top and bottom because this margin must be overridden in order to create a list > with background-color hover state. Margin-bottom works well for this example > because the navbar and the navbar-collapse are the same color. If they were not, > then there would obviously be space missing above the first link. Done. https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/_navbar... static/scss/_navbar.scss:143: .navbar .navbar-nav li:last-child On 2017/11/15 12:56:42, juliandoucette wrote: > Why use the paren't padding instead of the child's margin here? Done. https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/forms/_... File static/scss/forms/_buttons.scss (right): https://codereview.adblockplus.org/29587584/diff/29606585/static/scss/forms/_... static/scss/forms/_buttons.scss:27: color: inherit; On 2017/11/15 12:56:42, juliandoucette wrote: > Detail: You can push this separately. Ack. https://codereview.adblockplus.org/29625608
Thanks Ire, I took a fresh look at this today. https://codereview.adblockplus.org/29587584/diff/29625611/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29625611/includes/components... includes/components/navbar.html:8: <span class="open-label">Open</span> It seems like a toggle button will be common to all websites but a toggle button with a separate open/close state will not. Perhaps the open/close state should be separate until it is used by more than one website when it can be added as a second example? https://codereview.adblockplus.org/29587584/diff/29625611/includes/components... includes/components/navbar.html:9: <span class="sr-only">/</span> I don't think that this makes sense. - If .open-label is visible than this reads "Open / Menu" - If .close-label is visible than this reads "/ Close Menu" (sr-only) https://codereview.adblockplus.org/29587584/diff/29625611/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29625611/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component Perhaps this should *just* be a collapse component? https://codereview.adblockplus.org/29587584/diff/29625611/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29625611/static/scss/_navbar... static/scss/_navbar.scss:18: * Navbar component I think that all paddings in this file should be separate (not website-default) or optional / configurable (variable) because (at least) abp.org doesn't share them. We could also provide two examples of how-to space this component (parent space / help centre vs child space / abp.org).
New patchset https://codereview.adblockplus.org/29587584/diff/29625611/includes/components... File includes/components/navbar.html (right): https://codereview.adblockplus.org/29587584/diff/29625611/includes/components... includes/components/navbar.html:8: <span class="open-label">Open</span> On 2017/12/01 16:30:43, juliandoucette wrote: > It seems like a toggle button will be common to all websites but a toggle button > with a separate open/close state will not. Perhaps the open/close state should > be separate until it is used by more than one website when it can be added as a > second example? You're right. Done. https://codereview.adblockplus.org/29587584/diff/29625611/includes/components... includes/components/navbar.html:9: <span class="sr-only">/</span> On 2017/12/01 16:30:43, juliandoucette wrote: > I don't think that this makes sense. > > - If .open-label is visible than this reads "Open / Menu" > - If .close-label is visible than this reads "/ Close Menu" > > (sr-only) Yeah I think I didn't get this correctly. It's no longer relevant since I removed the separate open/close labels https://codereview.adblockplus.org/29587584/diff/29625611/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29625611/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component On 2017/12/01 16:30:43, juliandoucette wrote: > Perhaps this should *just* be a collapse component? I think that depends on what we end up extending in the future. We may want to add more features to the Navbar component, in which case it makes sense to keep the toggling here. Or we may want other elements to have the toggle feature, in which case your suggestion would make sense. I think we should change this when one of these scenarios becomes true. https://codereview.adblockplus.org/29587584/diff/29625611/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29625611/static/scss/_navbar... static/scss/_navbar.scss:18: * Navbar component On 2017/12/01 16:30:43, juliandoucette wrote: > I think that all paddings in this file should be separate (not website-default) > or optional / configurable (variable) because (at least) http://abp.org doesn't share > them. > > We could also provide two examples of how-to space this component (parent space > / help centre vs child space / http://abp.org). (When you're free let's discuss how this would work)
On 2017/12/08 10:42:23, ire wrote: > (When you're free let's discuss how this would work) (Discussed in person)
Thanks :) https://codereview.adblockplus.org/29587584/diff/29633581/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29633581/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component NIT: Accessibility? (aria-hidden, aria-controlled-by) (Can be added separately... We're talking right now and you said you would do it separately.) https://codereview.adblockplus.org/29587584/diff/29633581/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29633581/static/scss/_navbar... static/scss/_navbar.scss:44: .toggle-navbar-collapse @extend .link? (Maybe separately) (border, background, cursor) https://codereview.adblockplus.org/29587584/diff/29633581/static/scss/_navbar... static/scss/_navbar.scss:70: clear: both; NIT: @extend .clearfix?
New patch set https://codereview.adblockplus.org/29587584/diff/29633581/static/js/navbar.js File static/js/navbar.js (right): https://codereview.adblockplus.org/29587584/diff/29633581/static/js/navbar.js... static/js/navbar.js:2: * Navbar Component On 2017/12/08 15:24:06, juliandoucette wrote: > NIT: Accessibility? (aria-hidden, aria-controlled-by) > > (Can be added separately... We're talking right now and you said you would do it > separately.) Will do this in a follow-up issue https://codereview.adblockplus.org/29587584/diff/29633581/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29633581/static/scss/_navbar... static/scss/_navbar.scss:44: .toggle-navbar-collapse On 2017/12/08 15:24:06, juliandoucette wrote: > @extend .link? > > (Maybe separately) > > (border, background, cursor) Done. https://codereview.adblockplus.org/29587584/diff/29633581/static/scss/_navbar... static/scss/_navbar.scss:70: clear: both; On 2017/12/08 15:24:07, juliandoucette wrote: > NIT: @extend .clearfix? I don't think more than `clear: both` is needed here.
https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:31: @if ($add-horizontal-navbar-padding) NIT/Suggest: Check if $navbar-padding-x (or similar) is truthy instead? https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:37: width: $container-width; TOL: It might be cleaner to create a container mixin the accepts width and padding and use it here/elsewhere https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:71: padding-top: $navbar-padding + ($navbar-padding / 2); This doesn't scale with the toggle button height. https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:97: padding-top: 0; Why is this necessary? https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:137: @if ($add-horizontal-navbar-padding) A vertical padding dependant on horizontal padding? https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:144: padding-bottom: 0; This is only necessary if the previous condition is true? https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:176: padding-top: $navbar-padding + ($navbar-padding / 2); It seems like both the navbar nav items and the toggle button are taller than the navbar brand. It also seems that they will not scale together because they are not based on the same values. Here is an example of "based on the same values": $navbar-brand-height = 2em; $navbar-item-height = $navbar-brand-height / 2; // or 1em $navbar-padding = 1em; $navbar-brand-height + ($navbar-padding * 2) = 4em; $navbar-item-height + ($navbar-padding * 2) + (($navbar-brand-height - $navbar-item-height / 2) * 2) = 4em; Assuming that $navbar-brand-height will be equal to or greater than $navbar-item-height.
Thanks Julian, new patch https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:31: @if ($add-horizontal-navbar-padding) On 2017/12/13 16:17:38, juliandoucette wrote: > NIT/Suggest: Check if $navbar-padding-x (or similar) is truthy instead? I ended up with this solution for a couple of reasons: 1. Both the $navbar-padding-x and $navbar-padding-y should be the same 2. The $add-horizontal-navbar-padding seems like it should be separate because its true/false value has effects for if there is vertical padding applied. 3. Setting $navbar-padding-x to something falsey doesn't actually mean there will be no padding applied to the left or right. It's more about *where* that padding is applied. e.g. see padding on .navbar-branding Maybe the $add-horizontal-navbar-padding should be renamed? Like I said, it's more to do with where that padding is applied. https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:37: width: $container-width; On 2017/12/13 16:17:37, juliandoucette wrote: > TOL: It might be cleaner to create a container mixin the accepts width and > padding and use it here/elsewhere Great idea! Done. https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:97: padding-top: 0; On 2017/12/13 16:17:36, juliandoucette wrote: > Why is this necessary? Looks like it isn't :/ Must've been from a previous version. Removed. https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:137: @if ($add-horizontal-navbar-padding) On 2017/12/13 16:17:36, juliandoucette wrote: > A vertical padding dependant on horizontal padding? Yes, that vertical padding is only needed if there is also the horizontal padding https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:144: padding-bottom: 0; On 2017/12/13 16:17:37, juliandoucette wrote: > This is only necessary if the previous condition is true? Done. https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:176: padding-top: $navbar-padding + ($navbar-padding / 2); On 2017/12/13 16:17:36, juliandoucette wrote: > It seems like both the navbar nav items and the toggle button are taller than > the navbar brand. > > It also seems that they will not scale together because they are not based on > the same values. > > Here is an example of "based on the same values": > > $navbar-brand-height = 2em; > $navbar-item-height = $navbar-brand-height / 2; // or 1em > $navbar-padding = 1em; > > $navbar-brand-height + ($navbar-padding * 2) = 4em; > $navbar-item-height + ($navbar-padding * 2) + (($navbar-brand-height - > $navbar-item-height / 2) * 2) = 4em; > > Assuming that $navbar-brand-height will be equal to or greater than > $navbar-item-height. I've made some changes to address this
https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:137: @if ($add-horizontal-navbar-padding) On 2017/12/14 10:26:37, ire wrote: > On 2017/12/13 16:17:36, juliandoucette wrote: > > A vertical padding dependant on horizontal padding? > > Yes, that vertical padding is only needed if there is also the horizontal > padding Acknowledged. https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:176: padding-top: $navbar-padding + ($navbar-padding / 2); On 2017/12/14 10:26:34, ire wrote: > On 2017/12/13 16:17:36, juliandoucette wrote: > > It seems like both the navbar nav items and the toggle button are taller than > > the navbar brand. > > > > It also seems that they will not scale together because they are not based on > > the same values. > > > > Here is an example of "based on the same values": > > > > $navbar-brand-height = 2em; > > $navbar-item-height = $navbar-brand-height / 2; // or 1em > > $navbar-padding = 1em; > > > > $navbar-brand-height + ($navbar-padding * 2) = 4em; > > $navbar-item-height + ($navbar-padding * 2) + (($navbar-brand-height - > > $navbar-item-height / 2) * 2) = 4em; > > > > Assuming that $navbar-brand-height will be equal to or greater than > > $navbar-item-height. > > I've made some changes to address this Would you mind explaining your logic?
https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... File static/scss/_navbar.scss (right): https://codereview.adblockplus.org/29587584/diff/29635563/static/scss/_navbar... static/scss/_navbar.scss:176: padding-top: $navbar-padding + ($navbar-padding / 2); On 2017/12/14 12:55:41, juliandoucette wrote: > On 2017/12/14 10:26:34, ire wrote: > > On 2017/12/13 16:17:36, juliandoucette wrote: > > > It seems like both the navbar nav items and the toggle button are taller > than > > > the navbar brand. > > > > > > It also seems that they will not scale together because they are not based > on > > > the same values. > > > > > > Here is an example of "based on the same values": > > > > > > $navbar-brand-height = 2em; > > > $navbar-item-height = $navbar-brand-height / 2; // or 1em > > > $navbar-padding = 1em; > > > > > > $navbar-brand-height + ($navbar-padding * 2) = 4em; > > > $navbar-item-height + ($navbar-padding * 2) + (($navbar-brand-height - > > > $navbar-item-height / 2) * 2) = 4em; > > > > > > Assuming that $navbar-brand-height will be equal to or greater than > > > $navbar-item-height. > > > > I've made some changes to address this > > Would you mind explaining your logic? Sure :) The issue previously was that if the $navbar-branding-height were to change, the rest of the items in the navbar (the links, the toggle button) would no longer be vertically aligned to the center. So I changed this by setting the padding around those items that vertically center it to be based on the $navbar-barding-height instead of only the $navbar-padding. So now, as you scale up/down the height of the navbar-branding, the other items should maintain their vertical alignment.
On 2017/12/16 09:51:36, ire wrote:
> Sure :)
>
> The issue previously was that if the $navbar-branding-height were to change,
the
> rest of the items in the navbar (the links, the toggle button) would no longer
> be vertically aligned to the center. So I changed this by setting the padding
> around those items that vertically center it to be based on the
> $navbar-barding-height instead of only the $navbar-padding.
>
> So now, as you scale up/down the height of the navbar-branding, the other
items
> should maintain their vertical alignment.
I understood that. I was looking for a more specific example.
e.g.
```
155 .navbar .navbar-nav a
156 {
157 display: block;
158 padding-top: $navbar-padding / 2;
159 padding-bottom: $navbar-padding / 2;
160
161 @if ($add-horizontal-navbar-padding != true)
162 {
163 padding-right: $navbar-padding;
164 padding-left: $navbar-padding;
165 }
166
167 @media (min-width: $tablet-breakpoint)
168 {
169 padding-top: ($navbar-branding-height / 2) + ($navbar-padding / 2);
170 padding-right: $navbar-padding / 2;
171 padding-bottom: ($navbar-branding-height / 2) + ($navbar-padding / 2);
172 padding-left: $navbar-padding / 2;
173 }
174 }
```
AFAICT (in the example above):
1. The padding Y is not scaleable under tablet breakpoint
2. The padding Y scales according to the navbar branding height, assuming that
the inner height of .navbar-nav a is $navbar-branding-height / 2. But
.navbar-nav a inner height (line height) is not based on
$navbar-branding-height. Therefore, if you scale up $navbar-branding-height,
then these will not align?
On 2017/12/16 14:44:30, juliandoucette wrote:
> On 2017/12/16 09:51:36, ire wrote:
> > Sure :)
> >
> > The issue previously was that if the $navbar-branding-height were to change,
> the
> > rest of the items in the navbar (the links, the toggle button) would no
longer
> > be vertically aligned to the center. So I changed this by setting the
padding
> > around those items that vertically center it to be based on the
> > $navbar-barding-height instead of only the $navbar-padding.
> >
> > So now, as you scale up/down the height of the navbar-branding, the other
> items
> > should maintain their vertical alignment.
>
> I understood that. I was looking for a more specific example.
>
> e.g.
>
> ```
> 155 .navbar .navbar-nav a
> 156 {
> 157 display: block;
> 158 padding-top: $navbar-padding / 2;
> 159 padding-bottom: $navbar-padding / 2;
> 160
> 161 @if ($add-horizontal-navbar-padding != true)
> 162 {
> 163 padding-right: $navbar-padding;
> 164 padding-left: $navbar-padding;
> 165 }
> 166
> 167 @media (min-width: $tablet-breakpoint)
> 168 {
> 169 padding-top: ($navbar-branding-height / 2) + ($navbar-padding / 2);
> 170 padding-right: $navbar-padding / 2;
> 171 padding-bottom: ($navbar-branding-height / 2) + ($navbar-padding /
2);
> 172 padding-left: $navbar-padding / 2;
> 173 }
> 174 }
> ```
>
> AFAICT (in the example above):
>
> 1. The padding Y is not scaleable under tablet breakpoint
Under $tablet-breakpoint, the nav is no longer on the same line as the
navbar-branding, so the padding isn't applicable there (since the padding
top/bottom is applied for the purpose of vertically centering the nav links)
> 2. The padding Y scales according to the navbar branding height, assuming that
> the inner height of .navbar-nav a is $navbar-branding-height / 2. But
> .navbar-nav a inner height (line height) is not based on
> $navbar-branding-height. Therefore, if you scale up $navbar-branding-height,
> then these will not align?
The .navbar-nav a inner height isn't ever changed though? The only case in which
the navbar links and the navbar branding height will not align is if you change
the line-height on the navbar links. I don't know of the case in which we do or
will want to do that.
Have you tried scaling up the $navbar-branding-height and the links no longer
align? From my own tests they do.
> Under $tablet-breakpoint, the nav is no longer on the same line as the > navbar-branding, so the padding isn't applicable there (since the padding > top/bottom is applied for the purpose of vertically centering the nav links) Right. Sorry about that. > The .navbar-nav a inner height isn't ever changed though? The only case in which > the navbar links and the navbar branding height will not align is if you change > the line-height on the navbar links. I don't know of the case in which we do or > will want to do that. This is only correct when $navbar-padding === .navbar-nav a inner-height (1em). e.g. change $navbar-branding-height and $navbar-padding to 2em and they will not align. > Have you tried scaling up the $navbar-branding-height and the links no longer > align? From my own tests they do. I didn't before you asked. If I had tested myself than I could have anticipated the confusion above. Sorry about that :(
On 2017/12/18 17:48:58, juliandoucette wrote: > > Under $tablet-breakpoint, the nav is no longer on the same line as the > > navbar-branding, so the padding isn't applicable there (since the padding > > top/bottom is applied for the purpose of vertically centering the nav links) > > Right. Sorry about that. No problem :) On 2017/12/18 17:48:58, juliandoucette wrote: > > The .navbar-nav a inner height isn't ever changed though? The only case in > which > > the navbar links and the navbar branding height will not align is if you > change > > the line-height on the navbar links. I don't know of the case in which we do > or > > will want to do that. > > This is only correct when $navbar-padding === .navbar-nav a inner-height (1em). > > e.g. change $navbar-branding-height and $navbar-padding to 2em and they will not > align. Is this an issue relevant for abp? I can't find a way to accommodate for different sizes that would be simple. On 2017/12/18 17:48:58, juliandoucette wrote: > > Have you tried scaling up the $navbar-branding-height and the links no longer > > align? From my own tests they do. > > I didn't before you asked. If I had tested myself than I could have anticipated > the confusion above. Sorry about that :( No problem :)
> I can't find a way to accommodate for different sizes that would be simple. It depends on what you base the height of the navbar. If you base the height of the navbar on a fixed height then you can base paddings on the navbar height minus the height of the element within divided by two. If you base the height on an element within then you do the same, except you derive the navbar height from the outer height of that element within.
On 2018/01/04 17:29:24, juliandoucette wrote: > > I can't find a way to accommodate for different sizes that would be simple. > > It depends on what you base the height of the navbar. > > If you base the height of the navbar on a fixed height then you can base > paddings on the navbar height minus the height of the element within divided by > two. > > If you base the height on an element within then you do the same, except you > derive the navbar height from the outer height of that element within. I don't see how this solves the issue? My understanding is that, the issue you brought up was to do with, if we change the $navbar-branding-height and $navbar-padding to certain values, the elements would become misaligned. If I change those values to be based off a $navbar-height value, this wouldn't solve the issue of them being able to be misaligned if their values are changed. But I may have misunderstood the original issue you brought up. Could you demo the change you are suggesting?
On 2018/01/05 08:23:56, ire wrote: > Could you demo the change you are suggesting? Yes. I will do so separately.
LGTM - Let's move forward with this. I will demo/propose improvements separately.
On 2018/01/08 12:19:10, juliandoucette wrote: > LGTM - Let's move forward with this. I will demo/propose improvements > separately. Ack. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
