Issue 29485575: Issue 5385 - Create Site Header Component for Help Center (Closed)
|
Created:
July 10, 2017, 4:08 p.m. by ire Modified:
Sept. 4, 2017, 1:34 p.m. Reviewers:
juliandoucette Visibility:
Public. |
DescriptionIssue 5385 - Create Site Header Component for Help Center
Patch Set 1 #
Total comments: 39
Patch Set 2 : Fix svgs, Implement search, Show searchbar for no-js #
Total comments: 92
Patch Set 3 : Addressed smaller comments #Patch Set 4 : Rebase #Patch Set 5 : Unfix header, implement standard spacing units #
Total comments: 51
Patch Set 6 : Addressed NITs #Patch Set 7 : Fix alignment of search form, reformat JS #
Total comments: 57
Patch Set 8 : Implement navbar-collapse #
Total comments: 16
Patch Set 9 : Change mobile to tablet-breakpoint, add polyfill for classList, re-order rtl styles #Patch Set 10 : Include site variables before website-defaults #
MessagesTotal messages: 25
First attempt. I was mostly concerned with the layout of things on mobile vs desktop. Functionality will come in a later patch. https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore#newcode3 .hgignore:3: .stylelintrc.json I'm testing out the stylelint from the other codereview. Ignoring for now because I'll likely be making changes. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:27: <a href="/" id="toggle-searchform" class="tablet-and-mobile-only"> I am thinking that this would link to a DuckDuck Go page (specific for our site), which will provide a fallback for users without JavaScript. I haven't looked into whether that is something possible yet though. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:32: <a id="product-website-link" href="https://adblockplus.org/">Adblock Plus</a> This will be displayed conditionally depending on the type of page. https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_f... File static/scss/base/_font.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_f... static/scss/base/_font.scss:24: local("Source Sans Pro Light"), Changes made from including stylelint
First impressions - I hope they help :) https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore#newcode3 .hgignore:3: .stylelintrc.json On 2017/07/10 16:18:01, ire wrote: > I'm testing out the stylelint from the other codereview. Ignoring for now > because I'll likely be making changes. You can put this in your home directory can't you? https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/foo... includes/layout/footer.tmpl:1: {# I don't think we need this yet. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:1: {# I'm not sure if we need this - based on my discussion with Kai that I forwarded to you. What do you think? https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:20: <div class="navbar-wrapper cf"> cf? https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:22: {{ "index" | linkify() }} You are using linkify correctly here - but I don't think this template needs to be a template because there is no template logic inside of it. If this were HTML then you could just <a href="index">. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:23: <span class="sr-only">eyeo Help</span> Why span.sr-only and not img.alt? https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:24: <img src="img/png/eyeo-help.png" srcset="img/png/eyeo-help.png, img/svg/eyeo-help.svg 2x" alt=""> These images are not resolving correctly for me... Are they for you? In my experience: - src=**/* will convert to src=/en/**/* - locales/en/**/* will convert to /en/**/* - /en/**/* will not resolve to /**/* (Place language neutral resources in static/**/* and language dependant resources in locales/*/**/* and adjust paths accordingly) https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:24: <img src="img/png/eyeo-help.png" srcset="img/png/eyeo-help.png, img/svg/eyeo-help.svg 2x" alt=""> Missing img.alt - if one was not provided then you may write it yourself if you feel confident. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:27: <a href="/" id="toggle-searchform" class="tablet-and-mobile-only"> On 2017/07/10 16:18:02, ire wrote: > I am thinking that this would link to a DuckDuck Go page (specific for our > site), which will provide a fallback for users without JavaScript. I haven't > looked into whether that is something possible yet though. I think it's a better idea to just show the collapsed navigation by default on tablet/mobile and hide it if JavaScript is enabled. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:28: <img src="img/png/searchIcon.png" srcset="img/png/searchIcon.png, img/svg/searchIcon.svg 2x" alt="Toggle Search Bar"> Text is not translated (I'm guessing you did this on purpose since this is not the only occurrence). https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:32: <a id="product-website-link" href="https://adblockplus.org/">Adblock Plus</a> On 2017/07/10 16:18:01, ire wrote: > This will be displayed conditionally depending on the type of page. Disregard my comment above about .tmpl vs html if you want to add logic for this here instead of inside another include. I'm guessing this will become something like? {% if product %} <a ...> {{ get_string("global", product) }} </a> {% endif %} This is how we have been doing global strings so far (I'm not sure that's a good thing :D) - by creating a locale file manually in the default language and using get_string to retrieve a string from it. https://codereview.adblockplus.org/29485575/diff/29485576/includes/searchform... File includes/searchform.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/searchform... includes/searchform.tmpl:1: <form id="searchform" action=""> You can fill in the action and make this work now can't you? https://codereview.adblockplus.org/29485575/diff/29485576/includes/searchform... includes/searchform.tmpl:2: <label for="search" class="sr-only">Search Adblock Plus Help</label> I'm not sure this is necessary considering the placeholder, search button alt, and aria-label. What do you think? https://codereview.adblockplus.org/29485575/diff/29485576/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/js/main.js#n... static/js/main.js:6: document.getElementById('site-search').classList.toggle('open'); I don't remember if we agreed upon browser requirements for this website already, but one might argue that we should support the same browsers as ABP if we are developing a help center for ABP. - https://adblockplus.org/requirements - http://caniuse.com/#search=classList Let's have a talk with Ollie about the future of IE8 support before we do anything too crazy though :D https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_r... File static/scss/base/_reset.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_r... static/scss/base/_reset.scss:28: time, mark, audio, video It's not clear to me how these are grouped and why these are included and not others. Did you have any of this in mind? https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_u... static/scss/base/_utilities.scss:39: .cf:after NIT: We usually spell this out. https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/compone... File static/scss/components/_searchform.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/compone... static/scss/components/_searchform.scss:22: &:not(.open) http://caniuse.com/#feat=css-sel3 https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/content... static/scss/content/_typography.scss:23: line-height: 150%; https://developer.mozilla.org/en/docs/Web/CSS/line-height#Prefer_unitless_num...
> First impressions - I hope they help :) Thanks they did :) I think I addressed everything. A couple of comments will be addressed after we settle on support for IE 8. https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore#newcode3 .hgignore:3: .stylelintrc.json On 2017/07/14 14:11:28, juliandoucette wrote: > On 2017/07/10 16:18:01, ire wrote: > > I'm testing out the stylelint from the other codereview. Ignoring for now > > because I'll likely be making changes. > > You can put this in your home directory can't you? Yup you're right. Done. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/foo... includes/layout/footer.tmpl:1: {# On 2017/07/14 14:11:28, juliandoucette wrote: > I don't think we need this yet. Done. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:1: {# On 2017/07/14 14:11:28, juliandoucette wrote: > I'm not sure if we need this - based on my discussion with Kai that I forwarded > to you. What do you think? Yeah you're right. I'll remove it. On another note I think we should have a policy about this written somewhere, so it's clear which types of files we have decided to put the note. For example, based on the discussion, I'm still not sure if SCSS files should have the header or not. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:20: <div class="navbar-wrapper cf"> On 2017/07/14 14:11:28, juliandoucette wrote: > cf? clearfix. Will change it to be the full word. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:22: {{ "index" | linkify() }} On 2017/07/14 14:11:28, juliandoucette wrote: > You are using linkify correctly here - but I don't think this template needs to > be a template because there is no template logic inside of it. If this were HTML > then you could just <a href="index">. Seen comment below. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:23: <span class="sr-only">eyeo Help</span> On 2017/07/14 14:11:28, juliandoucette wrote: > Why span.sr-only and not img.alt? So I looked more into this and I had misunderstood a way screen readers work. Basically, I thought that there would be a difference in the way a screen reader would read out plain text vs the alternative text for an image. e.g. in this case I thought it would say something like "image, eyeo Help", instead of just "eyeo Help". So I was trying to avoid that by having an empty alt text (which would make the SR just skip over the image) and having the label for the site be in plain text. But I just tried out both using Chrome Vox and it looks like the SR just reads out the alt text without saying its an image beforehand. Long story short, I've changed it to using the alt text instead. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:24: <img src="img/png/eyeo-help.png" srcset="img/png/eyeo-help.png, img/svg/eyeo-help.svg 2x" alt=""> On 2017/07/14 14:11:28, juliandoucette wrote: > These images are not resolving correctly for me... Are they for you? > > In my experience: > - src=**/* will convert to src=/en/**/* > - locales/en/**/* will convert to /en/**/* > - /en/**/* will not resolve to /**/* > > (Place language neutral resources in static/**/* and language dependant > resources in locales/*/**/* and adjust paths accordingly) Got it. The issue with these images was actually a problem with the SVG. Fixed it. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:27: <a href="/" id="toggle-searchform" class="tablet-and-mobile-only"> On 2017/07/14 14:11:28, juliandoucette wrote: > On 2017/07/10 16:18:02, ire wrote: > > I am thinking that this would link to a DuckDuck Go page (specific for our > > site), which will provide a fallback for users without JavaScript. I haven't > > looked into whether that is something possible yet though. > > I think it's a better idea to just show the collapsed navigation by default on > tablet/mobile and hide it if JavaScript is enabled. Done. I've changed this to a button as well. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:28: <img src="img/png/searchIcon.png" srcset="img/png/searchIcon.png, img/svg/searchIcon.svg 2x" alt="Toggle Search Bar"> On 2017/07/14 14:11:28, juliandoucette wrote: > Text is not translated (I'm guessing you did this on purpose since this is not > the only occurrence). Yes sorry i should have mentioned. I was trying to do this incrementally so translations would come after. I've added them now though. https://codereview.adblockplus.org/29485575/diff/29485576/includes/layout/hea... includes/layout/header.tmpl:32: <a id="product-website-link" href="https://adblockplus.org/">Adblock Plus</a> On 2017/07/14 14:11:29, juliandoucette wrote: > On 2017/07/10 16:18:01, ire wrote: > > This will be displayed conditionally depending on the type of page. > > Disregard my comment above about .tmpl vs html if you want to add logic for this > here instead of inside another include. I'm guessing this will become something > like? > > {% if product %} > <a ...> > {{ get_string("global", product) }} > </a> > {% endif %} > > This is how we have been doing global strings so far (I'm not sure that's a good > thing :D) - by creating a locale file manually in the default language and using > get_string to retrieve a string from it. Yes this was my intention (same with the searchform.tmpl), although I hadn't figured out exactly how to execute yet. I will likely ask some followup questions on this via IRC. https://codereview.adblockplus.org/29485575/diff/29485576/includes/searchform... File includes/searchform.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29485576/includes/searchform... includes/searchform.tmpl:1: <form id="searchform" action=""> On 2017/07/14 14:11:29, juliandoucette wrote: > You can fill in the action and make this work now can't you? Done. https://codereview.adblockplus.org/29485575/diff/29485576/includes/searchform... includes/searchform.tmpl:2: <label for="search" class="sr-only">Search Adblock Plus Help</label> On 2017/07/14 14:11:29, juliandoucette wrote: > I'm not sure this is necessary considering the placeholder, search button alt, > and aria-label. What do you think? I think the label is more important that the others actually. When the input field is in focus, that's the text that is read out first. Although the placeholder is usually also read out, I'm not sure it is as reliable as the label itself. Following on from my comment about image alts, I think the `aria-label` might be what is overkill here. https://codereview.adblockplus.org/29485575/diff/29485576/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/js/main.js#n... static/js/main.js:6: document.getElementById('site-search').classList.toggle('open'); On 2017/07/14 14:11:29, juliandoucette wrote: > I don't remember if we agreed upon browser requirements for this website > already, but one might argue that we should support the same browsers as ABP if > we are developing a help center for ABP. > > - https://adblockplus.org/requirements > - http://caniuse.com/#search=classList > > Let's have a talk with Ollie about the future of IE8 support before we do > anything too crazy though :D It was my impression the support was ie 9 and up as is in the browserlist. Sure let's chat with Ollie to decide. https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_r... File static/scss/base/_reset.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_r... static/scss/base/_reset.scss:28: time, mark, audio, video On 2017/07/14 14:11:29, juliandoucette wrote: > It's not clear to me how these are grouped and why these are included and not > others. Did you have any of this in mind? I don't think they are necessarily grouped in a particularly meaningful way. Do they need to be? I also didn't think much about why others weren't included. There isn't anything that I would want to be here that isn't. Although there is plenty that will likely never be used. Do you think I should cut this down to only the elements we are likely to use? https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/base/_u... static/scss/base/_utilities.scss:39: .cf:after On 2017/07/14 14:11:29, juliandoucette wrote: > NIT: We usually spell this out. Done. https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/compone... File static/scss/components/_searchform.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/compone... static/scss/components/_searchform.scss:22: &:not(.open) On 2017/07/14 14:11:29, juliandoucette wrote: > http://caniuse.com/#feat=css-sel3 The only issue is IE8. So this will also be dependent on the conversation with Ollie. https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29485576/static/scss/content... static/scss/content/_typography.scss:23: line-height: 150%; On 2017/07/14 14:11:29, juliandoucette wrote: > https://developer.mozilla.org/en/docs/Web/CSS/line-height#Prefer_unitless_num... Done.
https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_u... static/scss/base/_utilities.scss:82: .unstyled It just occurred to me that I'm replicating the reset here :/ But I'd like to get your opinion on whether this style is still needed. The button element is not included in the reset, but in the case of the #toggle-searchform element I want it to be unstyled. If I add the button element to the reset, then it will apply to *all* buttons, which isn't necessarily what we want. What do you think?
Full review this time. Let me know if you want to meet to go over this. I know it can be daunting to address many comments. https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:2: <div class="navbar-wrapper clearfix"> I don't think we need clearfix here? --- (Unless we decide to move some floating elements above the site-header in the future?) https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:5: <img src="/img/png/eyeo-help.png" srcset="/img/png/eyeo-help.png, /img/svg/eyeo-help.svg 2x" alt="{{ "eyeo Help" | translate("site-title", "Image alt text") }}"> I don't think we need the first /img/png/eyeo-help.png in srcset? https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:8: <button id="toggle-searchform" class="unstyled tablet-and-mobile-only"> I think this control is misleading. This button toggles a menu that contains a search form and an external link. If our intention is to toggle a menu, I would suggest a menu icon (hamburger) and alike alt-text. If our intention is to toggle a search form, I would suggest expanding this search form out of the search icon to the right (above the site logo which would fade out during). https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:9: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg 2x" alt="{{ "Toggle Search Bar" | translate("search-toggle-alt", "Image alt text") }}"> NIT: Please use dashes in image names. https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:9: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg 2x" alt="{{ "Toggle Search Bar" | translate("search-toggle-alt", "Image alt text") }}"> 1x image is poor quality on low DPI screens. I think we can make this crisper and/or use real text for "eye/o | help" (keeping in mind that we can't promise fonts will be downloaded quickly or applied). https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:13: <a id="product-website-link" href="https://adblockplus.org/">{{ "Adblock Plus" | translate("abp-product-link", "Link") }}</a> NIT: I think this should have a `nav` around it; technically :/ https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:13: <a id="product-website-link" href="https://adblockplus.org/">{{ "Adblock Plus" | translate("abp-product-link", "Link") }}</a> Missing icon. https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform... File includes/searchform.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform... includes/searchform.tmpl:6: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg 2x" alt="{{ "Search" | translate("search-icon-alt", "Image alt text") }}"> NIT: I wonder if "Submit (search|query)" makes more sense (or if they are both equally fine) considering the label above. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_f... File static/scss/base/_font.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_f... static/scss/base/_font.scss:26: url("../fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.woff2") format("woff2"), These fonts are missing from this patchset. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_r... File static/scss/base/_reset.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_r... static/scss/base/_reset.scss:17: html, body, div, span, iframe, Note: I'm still leaning away from this style (reset vs normalize). If we do allow it, I think we aught to move it to website-defaults. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_u... static/scss/base/_utilities.scss:1: // This file is part of help.eyeo.com. I think that everything in this file belongs in website-defaults. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_u... static/scss/base/_utilities.scss:82: .unstyled On 2017/07/17 09:31:37, ire wrote: > It just occurred to me that I'm replicating the reset here :/ But I'd like to > get your opinion on whether this style is still needed. The button element is > not included in the reset, but in the case of the #toggle-searchform element I > want it to be unstyled. If I add the button element to the reset, then it will > apply to *all* buttons, which isn't necessarily what we want. What do you think? I think we want to completely standardize all form element styles across website/browser in website-defaults to avoid unexpected behaviour ( especially relevant to gnu/linux and dark themes like mine O:) ). I think that an .unstyled class is very useful for content. I think that we should avoid using .unstyled on components ( where we want to keep all applicable styles together e.g. the whole point of BEM; I think :/ ) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... File static/scss/base/_variables.scss (left): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:15: // along with acceptableads.org. If not, see <http://www.gnu.org/licenses/>. NIT: Let's fix this in the prior commit before we push it to the help center repository :D https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:57: $xsmall-font: 0.8em; NIT: "x" -> "extra" ... This one is hard for me to point out :D - I don't like "$extra-small-font" | "$very-small-font" ... Maybe $tiny-font? ;) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:67: $content-max-width: 90%; It seems like we should support the same max content with across websites in the form of a $large-desktop-width or similar. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:68: $content-width: $desktop-width; Note: This isn't currently being used. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:77: $searchform-width: 320px; NIT/Suggest: "search-" or "search-form" ... "searchform" is not a word? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:86: $mobile-breakpoint: 544px; Is the width provided by website-defaults wrong? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... File static/scss/components/_searchform.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:20: width: 100%; NIT: I think this width is unnecessary? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:22: &:not(.open) NIT: I think it may be clearer if we just make display:none by default and display:block in #site-search.open. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:52: .no-js #product-website-link I think this is misleading as .no-js #product-website-link is display:none. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:81: vertical-align: middle; Should every child of `.navbar` be vertical-align middle? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:85: max-width: $searchform-width; NIT/Suggest: $search-max-width https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:89: #site-search.open #searchform, It seems like this should apply to a container of #searchform and #product-website-link rather than each of them individually. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/content... static/scss/content/_typography.scss:17: body Seems like this should have been part of the boilerplate or we should have implemented the styleguide first. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_body.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_body.scss:19: min-height: 100vh; // For testing purposes. Will remove. Ack. Seems like this should be part of another commit. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:19: position: fixed; Where is it specified that this header is fixed? --- (I think that our navbars should be consistently fixed or not across websites. And I would suggest not fixed personally.) (If we do decide that it should be fixed then I would suggest using a more generic class in `navbar.scss` e.g. `.navbar-fixed`) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:23: min-height: $site-header-height; Is this height specified? It doesn't seem to line up with the screenshots in the spec (which seem rather large). https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:24: font-size: $small-font; This doesn't seem to apply to anything? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:25: line-height: 40px; It seems like this is supposed to match the height of the logo and searchform. If so, I would suggest using a variable. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:31: margin-right: 50px; It seems like this should correspond to some sort of standard spacing unit throughout this website. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:36: height: 25px; Is it just me, or does this look bigger (less space above/below) in the spec :/ https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:53: html[dir="rtl"] I suggest separating rtl styles ~like we separate media queries. I think they are more readable when grouped together than at the bottom of files. --- (This applies to other files too) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:19: padding-top: 12px; I suggest using em here so that this padding scales with font-size. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:21: color: $white; I think it makes sense to implement generic things like `.navbar` in website-defaults. With that in mind, I suggest that color is too specific for this class. I would suggest applying it to #site-header or something like .navbar-inverse, .navbar-black, etc. The same goes for color properties within. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:30: .navbar-wrapper Why .navbar-wrapper and not a more generic .container? (Assuming you are familiar with this pattern.) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:32: width: $navbar-width; Where is this width specified? --- (I think it's too small) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:33: max-width: $content-max-width; Where is this max-width specified? --- (And why use effectively 10% margin left/right < $navbar-width instead of e.g. 1em) https://codereview.adblockplus.org/29485575/diff/29490566/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/templates/default.t... templates/default.tmpl:47: <div class="outer"> I think that all of these changes except for <? include layout/header ?> belong in another commit.
Thanks Julian! I've done my best to address most of the smaller comments that I could. I think we should have a meeting to discuss some of the bigger issues. I saw that you're on holiday today, so we can schedule something on Monday/Tuesday. You may also want to go through this patch before the meeting. https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:2: <div class="navbar-wrapper clearfix"> On 2017/07/19 18:03:33, juliandoucette wrote: > I don't think we need clearfix here? > > --- > > (Unless we decide to move some floating elements above the site-header in the > future?) You're right it's not needed. I must have added it when I had a slightly different implementation. https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:5: <img src="/img/png/eyeo-help.png" srcset="/img/png/eyeo-help.png, /img/svg/eyeo-help.svg 2x" alt="{{ "eyeo Help" | translate("site-title", "Image alt text") }}"> On 2017/07/19 18:03:33, juliandoucette wrote: > I don't think we need the first /img/png/eyeo-help.png in srcset? Done. https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:9: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg 2x" alt="{{ "Toggle Search Bar" | translate("search-toggle-alt", "Image alt text") }}"> On 2017/07/19 18:03:32, juliandoucette wrote: > NIT: Please use dashes in image names. Done. https://codereview.adblockplus.org/29485575/diff/29490566/includes/layout/hea... includes/layout/header.tmpl:13: <a id="product-website-link" href="https://adblockplus.org/">{{ "Adblock Plus" | translate("abp-product-link", "Link") }}</a> On 2017/07/19 18:03:32, juliandoucette wrote: > NIT: I think this should have a `nav` around it; technically :/ Done. https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform... File includes/searchform.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform... includes/searchform.tmpl:6: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg 2x" alt="{{ "Search" | translate("search-icon-alt", "Image alt text") }}"> On 2017/07/19 18:03:33, juliandoucette wrote: > NIT: I wonder if "Submit (search|query)" makes more sense (or if they are both > equally fine) considering the label above. I think the submit buttons on most search forms just say "Search" https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_f... File static/scss/base/_font.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_f... static/scss/base/_font.scss:26: url("../fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.woff2") format("woff2"), On 2017/07/19 18:03:33, juliandoucette wrote: > These fonts are missing from this patchset. They were included in the previous boilerplate commit https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:57: $xsmall-font: 0.8em; On 2017/07/19 18:03:34, juliandoucette wrote: > NIT: "x" -> "extra" ... This one is hard for me to point out :D - I don't like > "$extra-small-font" | "$very-small-font" ... Maybe $tiny-font? ;) I acknowledge that the x is hard to read. I think tiny works. But the lack of consistency might bother me :P . Are we going to stop using "extra" everywhere? E.g. xlarge, etc. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:67: $content-max-width: 90%; On 2017/07/19 18:03:34, juliandoucette wrote: > It seems like we should support the same max content with across websites in the > form of a $large-desktop-width or similar. I assumed this is what the $dekstop-width was, which is why I used it below. This $content-max-width is to ensure the content on the page never spans full width (essentially padding). https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:68: $content-width: $desktop-width; On 2017/07/19 18:03:35, juliandoucette wrote: > Note: This isn't currently being used. Yes its for the main part of the page. The $navbar-width was actually supposed to reference the $content-width variable, not the $desktop-width directly. Will change. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:77: $searchform-width: 320px; On 2017/07/19 18:03:35, juliandoucette wrote: > NIT/Suggest: "search-" or "search-form" ... "searchform" is not a word? I think "searchform" is one of those names like "navbar" I see used. But I don't feel particularly strongly about it so I've changed it to "search-form" everywhere https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:86: $mobile-breakpoint: 544px; On 2017/07/19 18:03:35, juliandoucette wrote: > Is the width provided by website-defaults wrong? There is no $mobile-breakpoint variable in website-defaults https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... File static/scss/components/_searchform.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:20: width: 100%; On 2017/07/19 18:03:36, juliandoucette wrote: > NIT: I think this width is unnecessary? Done. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:22: &:not(.open) On 2017/07/19 18:03:35, juliandoucette wrote: > NIT: I think it may be clearer if we just make display:none by default and > display:block in #site-search.open. Done. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:52: .no-js #product-website-link On 2017/07/19 18:03:35, juliandoucette wrote: > I think this is misleading as .no-js #product-website-link is display:none. I don't understand. `.no-js #product-website-link` is not `display:none`? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:81: vertical-align: middle; On 2017/07/19 18:03:36, juliandoucette wrote: > Should every child of `.navbar` be vertical-align middle? No, considering my comment about .navbar also being used for the footer. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:85: max-width: $searchform-width; On 2017/07/19 18:03:35, juliandoucette wrote: > NIT/Suggest: $search-max-width I'm not sure about this. On mobile, the search form does get bigger than this size since it becomes 100% https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:89: #site-search.open #searchform, On 2017/07/19 18:03:37, juliandoucette wrote: > It seems like this should apply to a container of #searchform and > #product-website-link rather than each of them individually. The margin-top on both these elements, although they are the same, are actually serving different purposes. The top margin on the #searchform is providing space between it and the logo. The top margin on the #product-website-link is to align the text to the bottom line of the search form. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_body.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_body.scss:19: min-height: 100vh; // For testing purposes. Will remove. On 2017/07/19 18:03:37, juliandoucette wrote: > Ack. > > Seems like this should be part of another commit. You're referring to the min-height style right? This will be removed before I commit this patch. It shouldn't be part of any commit, or am I misunderstanding what you're saying? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:19: position: fixed; On 2017/07/19 18:03:37, juliandoucette wrote: > Where is it specified that this header is fixed? > > --- > > (I think that our navbars should be consistently fixed or not across websites. > And I would suggest not fixed personally.) > > (If we do decide that it should be fixed then I would suggest using a more > generic class in `navbar.scss` e.g. `.navbar-fixed`) It wasn't specified, I guess I made an assumption that it was supposed to be fixed. I'm not sure putting it in the navbar will work, since the navbar class is also used in the footer (maybe this was a poor choice of class name? :/) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:23: min-height: $site-header-height; On 2017/07/19 18:03:38, juliandoucette wrote: > Is this height specified? It doesn't seem to line up with the screenshots in the > spec (which seem rather large). It looks about right to me. Perhaps we should consult Jeen for some actual figures to be certain. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:24: font-size: $small-font; On 2017/07/19 18:03:38, juliandoucette wrote: > This doesn't seem to apply to anything? The font-size? It applies to all text in the header, e.g. the #product-website-link https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:25: line-height: 40px; On 2017/07/19 18:03:38, juliandoucette wrote: > It seems like this is supposed to match the height of the logo and searchform. > If so, I would suggest using a variable. Done. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:36: height: 25px; On 2017/07/19 18:03:37, juliandoucette wrote: > Is it just me, or does this look bigger (less space above/below) in the spec :/ This also looks pretty accurate to be but will ask Jeen for actual figures. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:53: html[dir="rtl"] On 2017/07/19 18:03:38, juliandoucette wrote: > I suggest separating rtl styles ~like we separate media queries. I think they > are more readable when grouped together than at the bottom of files. > > --- > > (This applies to other files too) This is what I've done already? Or is the issue with the .no-js styles below? I'll move them above. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:19: padding-top: 12px; On 2017/07/19 18:03:39, juliandoucette wrote: > I suggest using em here so that this padding scales with font-size. This will interfere with the fixed height. However, if we aren't making the header position:fixed then it doesn't matter. We will discuss in a meeting. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:30: .navbar-wrapper On 2017/07/19 18:03:38, juliandoucette wrote: > Why .navbar-wrapper and not a more generic .container? (Assuming you are > familiar with this pattern.) Because this container is specific to the navbar only. If I were to use container, the selector would be `.navbar .container` instead of just `.navbar-container`/`.navbar-wrapper`. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:32: width: $navbar-width; On 2017/07/19 18:03:39, juliandoucette wrote: > Where is this width specified? > > --- > > (I think it's too small) It isn't specified anywhere. I used the $dekstop-width from website-defaults plus some extra spacing because the navbar extends further than the content. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:33: max-width: $content-max-width; On 2017/07/19 18:03:38, juliandoucette wrote: > Where is this max-width specified? > > --- > > (And why use effectively 10% margin left/right < $navbar-width instead of e.g. > 1em) It isn't specified anywhere. (Are these values supposed to be specified in the spec or somewhere?) Using percentages in this way is just my personal preference. It feels more responsive to me because the size adapts as the screen size changes. Though if the way you mentioned is the preferred method, I'm happy to change it.
Quick note: Let's make sure our search form supports https://developers.google.com/search/docs/data-types/sitelinks-searchbox
Thanks Ire! Partial reviews/responses below. https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform... File includes/searchform.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform... includes/searchform.tmpl:6: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg 2x" alt="{{ "Search" | translate("search-icon-alt", "Image alt text") }}"> On 2017/07/21 10:23:26, ire wrote: > On 2017/07/19 18:03:33, juliandoucette wrote: > > NIT: I wonder if "Submit (search|query)" makes more sense (or if they are both > > equally fine) considering the label above. > > I think the submit buttons on most search forms just say "Search" Acknowledged. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_f... File static/scss/base/_font.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_f... static/scss/base/_font.scss:26: url("../fonts/Source-Sans-Pro-300/Source-Sans-Pro-300.woff2") format("woff2"), On 2017/07/21 10:23:26, ire wrote: > On 2017/07/19 18:03:33, juliandoucette wrote: > > These fonts are missing from this patchset. > > They were included in the previous boilerplate commit Acknowledged. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:57: $xsmall-font: 0.8em; On 2017/07/21 10:23:27, ire wrote: > On 2017/07/19 18:03:34, juliandoucette wrote: > > NIT: "x" -> "extra" ... This one is hard for me to point out :D - I don't like > > "$extra-small-font" | "$very-small-font" ... Maybe $tiny-font? ;) > > I acknowledge that the x is hard to read. I think tiny works. But the lack of > consistency might bother me :P . Are we going to stop using "extra" everywhere? > E.g. xlarge, etc. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:67: $content-max-width: 90%; On 2017/07/21 10:23:26, ire wrote: > I assumed this is what the $dekstop-width was, which is why I used it below. > This $content-max-width is to ensure the content on the page never spans full > width (essentially padding). Why should we use a width percentage instead of actual padding? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:68: $content-width: $desktop-width; On 2017/07/21 10:23:27, ire wrote: > On 2017/07/19 18:03:35, juliandoucette wrote: > > Note: This isn't currently being used. > > Yes its for the main part of the page. The $navbar-width was actually supposed > to reference the $content-width variable, not the $desktop-width directly. Will > change. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:77: $searchform-width: 320px; On 2017/07/21 10:23:27, ire wrote: > On 2017/07/19 18:03:35, juliandoucette wrote: > > NIT/Suggest: "search-" or "search-form" ... "searchform" is not a word? > > I think "searchform" is one of those names like "navbar" I see used. But I don't > feel particularly strongly about it so I've changed it to "search-form" > everywhere Acknowledged. I'm not familiar with this pattern (that doesn't mean it's not common). https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_v... static/scss/base/_variables.scss:86: $mobile-breakpoint: 544px; On 2017/07/21 10:23:27, ire wrote: > On 2017/07/19 18:03:35, juliandoucette wrote: > > Is the width provided by website-defaults wrong? > > There is no $mobile-breakpoint variable in website-defaults I did that on purpose so that we would develop mobile-first ;) (Acknowledging that my original comment was invalid) https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... File static/scss/components/_searchform.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/compone... static/scss/components/_searchform.scss:52: .no-js #product-website-link On 2017/07/21 10:23:28, ire wrote: > On 2017/07/19 18:03:35, juliandoucette wrote: > > I think this is misleading as .no-js #product-website-link is display:none. > > I don't understand. `.no-js #product-website-link` is not `display:none`? Acknowledged. I don't know what I was thinking :/ https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_body.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_body.scss:19: min-height: 100vh; // For testing purposes. Will remove. On 2017/07/21 10:23:29, ire wrote: > On 2017/07/19 18:03:37, juliandoucette wrote: > > Ack. > > > > Seems like this should be part of another commit. > > You're referring to the min-height style right? This will be removed before I > commit this patch. It shouldn't be part of any commit, or am I misunderstanding > what you're saying? Acknowledged. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:19: position: fixed; On 2017/07/21 10:23:30, ire wrote: > I'm not sure putting it in the navbar will work, since the navbar class is also > used in the footer (maybe this was a poor choice of class name? :/) I meant in an additional class e.g. <nav class="navbar navbar-fixed-top"> https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:23: min-height: $site-header-height; On 2017/07/21 10:23:29, ire wrote: > On 2017/07/19 18:03:38, juliandoucette wrote: > > Is this height specified? It doesn't seem to line up with the screenshots in > the > > spec (which seem rather large). > > It looks about right to me. Perhaps we should consult Jeen for some actual > figures to be certain. Agreed. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:24: font-size: $small-font; On 2017/07/21 10:23:29, ire wrote: > On 2017/07/19 18:03:38, juliandoucette wrote: > > This doesn't seem to apply to anything? > > The font-size? It applies to all text in the header, e.g. the > #product-website-link Nope... #product-website-link is inheriting it's font-size from your CSS reset :D https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:36: height: 25px; On 2017/07/21 10:23:29, ire wrote: > On 2017/07/19 18:03:37, juliandoucette wrote: > > Is it just me, or does this look bigger (less space above/below) in the spec > :/ > > This also looks pretty accurate to be but will ask Jeen for actual figures. Agreed. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:53: html[dir="rtl"] On 2017/07/21 10:23:30, ire wrote: > This is what I've done already? Or is the issue with the .no-js styles below? > I'll move them above. In this specific case I'm suggesting that you move html[dir="rtl"] .site-title above #toggle-searchform. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:19: padding-top: 12px; On 2017/07/21 10:23:31, ire wrote: > On 2017/07/19 18:03:39, juliandoucette wrote: > > I suggest using em here so that this padding scales with font-size. > > This will interfere with the fixed height. However, if we aren't making the > header position:fixed then it doesn't matter. We will discuss in a meeting. Acknowledged. Good point. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:30: .navbar-wrapper On 2017/07/21 10:23:31, ire wrote: > Because this container is specific to the navbar only. If I were to use > container, the selector would be `.navbar .container` instead of just > `.navbar-container`/`.navbar-wrapper`. What is specific about the navbar's container? Aren't the navbar's contents contained in the same width as the primary content? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:32: width: $navbar-width; On 2017/07/21 10:23:31, ire wrote: > It isn't specified anywhere. I used the $dekstop-width from website-defaults > plus some extra spacing because the navbar extends further than the content. They look the same to me? https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:33: max-width: $content-max-width; On 2017/07/21 10:23:30, ire wrote: > It isn't specified anywhere. (Are these values supposed to be specified in the > spec or somewhere?) On the one hand: yes On the other hand: that which is not specified is open to interpretation What matters to me: that we support the same widths across websites > Using percentages in this way is just my personal preference. It feels more > responsive to me because the size adapts as the screen size changes. Though if > the way you mentioned is the preferred method, I'm happy to change it. This relates to one of my comments in another file. I prefer actual margin/padding personally. But I'd rather base the decision on something other than our personal preferences if possible.
Can you rebase this onto the tip of help.eyeo.com?
> Can you rebase this onto the tip of help.eyeo.com? Done + plus some smaller fixes. Just to note here (so you know I haven't forgotten), some of the issues you mentioned before I haven't gotten to yet: 1. Quality of the logo and possibility of using real text instead 2. Reset vs Normalize styles 3. Implement standard spacing units If there's anything I may have overlooked please bring it to my attention. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:19: position: fixed; On 2017/07/24 21:08:09, juliandoucette wrote: > On 2017/07/21 10:23:30, ire wrote: > > I'm not sure putting it in the navbar will work, since the navbar class is > also > > used in the footer (maybe this was a poor choice of class name? :/) > > I meant in an additional class e.g. > > <nav class="navbar navbar-fixed-top"> Done. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_header.scss:53: html[dir="rtl"] On 2017/07/24 21:08:09, juliandoucette wrote: > On 2017/07/21 10:23:30, ire wrote: > > This is what I've done already? Or is the issue with the .no-js styles below? > > I'll move them above. > > In this specific case I'm suggesting that you move html[dir="rtl"] .site-title > above #toggle-searchform. Done. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:32: width: $navbar-width; On 2017/07/24 21:08:10, juliandoucette wrote: > On 2017/07/21 10:23:31, ire wrote: > > It isn't specified anywhere. I used the $dekstop-width from website-defaults > > plus some extra spacing because the navbar extends further than the content. > > They look the same to me? In this image (https://bytebucket.org/adblockplus/spec/raw/6eb0fc6e906a60ac76d93cf1ba9ae770e...), the width of the navbar seems to be wider than the width of the content. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:33: max-width: $content-max-width; On 2017/07/24 21:08:10, juliandoucette wrote: > On 2017/07/21 10:23:30, ire wrote: > > Using percentages in this way is just my personal preference. It feels more > > responsive to me because the size adapts as the screen size changes. Though if > > the way you mentioned is the preferred method, I'm happy to change it. > > This relates to one of my comments in another file. I prefer actual > margin/padding personally. But I'd rather base the decision on something other > than our personal preferences if possible. Agreed. I haven't been able to find any objective evidence that one way is better than the other. Have you got any particular justifications? If there are no objective reasons for either way, then I'm happy to go with your way for consistency with other sites.
I spoke with Jeen to clarify some of the things you asked about. Notes below. > 1. Quality of the logo and possibility of using real text instead Can you go over again what your issue with the logo is? Because I think using an svg/image is better than real text. Will your issue be fixed by adding another size of image? > 2. Reset vs Normalize styles I'm not sure if this is something that necessarily has to be addressed in this issue. Would you mind if I created a separate issue to discuss and address this? > 3. Implement standard spacing units Done. https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:32: width: $navbar-width; On 2017/08/10 16:54:43, ire wrote: > On 2017/07/24 21:08:10, juliandoucette wrote: > > On 2017/07/21 10:23:31, ire wrote: > > > It isn't specified anywhere. I used the $dekstop-width from website-defaults > > > plus some extra spacing because the navbar extends further than the content. > > > > > They look the same to me? > > In this image > (https://bytebucket.org/adblockplus/spec/raw/6eb0fc6e906a60ac76d93cf1ba9ae770e...), > the width of the navbar seems to be wider than the width of the content. I spoke with Jeen and she confirmed that the navbar width should be wider than the content width https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/layout/... static/scss/layout/_navbar.scss:33: max-width: $content-max-width; On 2017/08/10 16:54:43, ire wrote: > On 2017/07/24 21:08:10, juliandoucette wrote: > > On 2017/07/21 10:23:30, ire wrote: > > > Using percentages in this way is just my personal preference. It feels more > > > responsive to me because the size adapts as the screen size changes. Though > if > > > the way you mentioned is the preferred method, I'm happy to change it. > > > > This relates to one of my comments in another file. I prefer actual > > margin/padding personally. But I'd rather base the decision on something other > > than our personal preferences if possible. > > Agreed. I haven't been able to find any objective evidence that one way is > better than the other. Have you got any particular justifications? > > If there are no objective reasons for either way, then I'm happy to go with your > way for consistency with other sites. > Done. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:1: <header id="site-header" class="navbar"> I spoke with Jeen and she said the header shouldn't be fixed to the top
Thanks Ire, All minor issues IMO. Don't worry about updating this soon if there are other parts of this website that you would rather work on. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:1: <header id="site-header" class="navbar"> On 2017/08/16 14:41:26, ire wrote: > I spoke with Jeen and she said the header shouldn't be fixed to the top Acknowledged. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:5: <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt="{{ "eyeo Help" | translate("site-title", "Image alt text") }}"> Note: I'm not sure if no textContent within an h1 has a negative impact on a document outline. Or, if it does, if that matters. I think this is fine for now. I just wanted to point this out. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:5: <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt="{{ "eyeo Help" | translate("site-title", "Image alt text") }}"> NIT: This 1x image quality is unacceptable IMO. I'll send a screenshot that may or may not demonstrate this. This doesn't need to block the review. But it wouldn't hurt if we updated it soon. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:8: <button id="toggle-search-form" class="unstyled tablet-and-mobile-only"> Note: It seems super-confusing t ome that the toggle button is a search icon. I'm not sure if that matters though (sorry if I brought this up before). https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:8: <button id="toggle-search-form" class="unstyled tablet-and-mobile-only"> NIT: The click area of this button is too small IMO. It's currently {h: 40, w: 15} and I think it should be {h: 40, w: 40} (or whatever em equivalent) https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:15: {{ "Adblock Plus" | translate("abp-product-link", "Link") }} NIT: I think this text is larger in the mock up. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | translate("external-link-icon", "Image alt text") }}"> NIT: Isn't this the wrong icon? (The one in the spec looks better IMO) https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | translate("external-link-icon", "Image alt text") }}"> Note: I'm assuming that you will replace "Image alt text" everywhere. (Sorry if this was already brought up) https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:1: <form id="search-form" action="https://duckduckgo.com" method="GET"> Note: Regarding size differences between the implementation and the mockup: These are not black and white because the images provided are obviously too big (the full width of the design cannot fit on a standard loptop screen at 100% zoom). I think we should request a 1280x800 rendering. But this kind of polishing doesn't have to block the review. We can push without addressing NITs as long as we document them in low priority tickets. https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:3: <input id="search" name="q" type="search" placeholder="{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}"> NIT: The placeholder text is not properly centered on my screen [debian, chromium, lowdpi] https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:4: <input type="hidden" name="sites" value="adblockplus.org"> NIT: I think there is more border radius in the mockup. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:8: document.getElementById('toggle-search-form').addEventListener('click', function(e) { NIT: This doesn't seem to fit on one line. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:10: document.getElementById('site-search').classList.toggle('open'); NIT: I usually chain on newlines e.g. document .getElementById(...) .classList.toggle(...) But I don't think our code style guide says anything about this. So it's up-to-you. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:10: document.getElementById('site-search').classList.toggle('open'); NIT: Our code style specifies double quotes :( (I like single quotes) https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:105: height: $search-form-height - 2px; Why -2px? https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:106: padding-right: $search-form-height + 5px; NIT: It seems strange to calculate an x value using a y. And why not use ems? Should this not scale with font-size? https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:126: height: 15px; NIT: Shouldn't this be an em value to match the font? https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/layout/... static/scss/layout/_navbar.scss:19: padding-top: $md; Note: Does our code style guide prefer specific direction properties over shorthand syntax? I like the way you are doing this personally. Although I would usually use shorthand for the sake of filesize. NBD (No big deal) anyway. https://codereview.adblockplus.org/29485575/diff/29517664/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/templates/default.t... templates/default.tmpl:46: <div class="outer"> NIT: Does .outer do anything in this patchset?
> Thanks Ire, > > All minor issues IMO. Don't worry about updating this soon if there are > other parts of this website that you would rather work on. You're welcome. No worries I would rather have this able to push so I'm not building on top of too many unfinished issues than move on too far. So here is my update :) https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:5: <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt="{{ "eyeo Help" | translate("site-title", "Image alt text") }}"> On 2017/08/18 15:03:55, juliandoucette wrote: > Note: I'm not sure if no textContent within an h1 has a negative impact on a > document outline. Or, if it does, if that matters. I think this is fine for now. > I just wanted to point this out. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:5: <img src="/img/png/eyeo-help.png" srcset="/img/svg/eyeo-help.svg 2x" alt="{{ "eyeo Help" | translate("site-title", "Image alt text") }}"> On 2017/08/18 15:03:52, juliandoucette wrote: > NIT: This 1x image quality is unacceptable IMO. I'll send a screenshot that may > or may not demonstrate this. This doesn't need to block the review. But it > wouldn't hurt if we updated it soon. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:8: <button id="toggle-search-form" class="unstyled tablet-and-mobile-only"> On 2017/08/18 15:03:54, juliandoucette wrote: > Note: It seems super-confusing t ome that the toggle button is a search icon. > I'm not sure if that matters though (sorry if I brought this up before). Yes it did come up, sorry that I didn't address it. I think the search icon makes sense because its toggling the search form. A hamburger menu would imply that there is a navigation there when there isn't. Also, searching is the primary action a user would want to take, so making it clear that a search bar would appear (and not just a menu) makes sense to me. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:8: <button id="toggle-search-form" class="unstyled tablet-and-mobile-only"> On 2017/08/18 15:03:54, juliandoucette wrote: > NIT: The click area of this button is too small IMO. It's currently {h: 40, w: > 15} and I think it should be {h: 40, w: 40} (or whatever em equivalent) Done. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:15: {{ "Adblock Plus" | translate("abp-product-link", "Link") }} On 2017/08/18 15:03:54, juliandoucette wrote: > NIT: I think this text is larger in the mock up. Will ask Jeen for font sizes https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | translate("external-link-icon", "Image alt text") }}"> On 2017/08/18 15:03:53, juliandoucette wrote: > NIT: Isn't this the wrong icon? (The one in the spec looks better IMO) Discussed with Jeen and she said we should use this icon across the websites https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | translate("external-link-icon", "Image alt text") }}"> On 2017/08/18 15:03:51, juliandoucette wrote: > Note: I'm assuming that you will replace "Image alt text" everywhere. > > (Sorry if this was already brought up) It wasn't brought up before. I didn't intent to replace it. Isn't this the "content" part of the identifier, which is supposed to describe what type of string it is (e.g. heading, paragraph, image alt text)? https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:1: <form id="search-form" action="https://duckduckgo.com" method="GET"> On 2017/08/18 15:03:55, juliandoucette wrote: > Note: Regarding size differences between the implementation and the mockup: > These are not black and white because the images provided are obviously too big > (the full width of the design cannot fit on a standard loptop screen at 100% > zoom). I think we should request a 1280x800 rendering. But this kind of > polishing doesn't have to block the review. We can push without addressing NITs > as long as we document them in low priority tickets. Ack. Will address this in a different issue https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:3: <input id="search" name="q" type="search" placeholder="{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}"> On 2017/08/18 15:03:55, juliandoucette wrote: > NIT: The placeholder text is not properly centered on my screen [debian, > chromium, lowdpi] Centered within the input? It isn't supposed to be centered, it is aligned left. https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:4: <input type="hidden" name="sites" value="adblockplus.org"> On 2017/08/18 15:03:55, juliandoucette wrote: > NIT: I think there is more border radius in the mockup. Done. It was 1px more round :) https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:8: document.getElementById('toggle-search-form').addEventListener('click', function(e) { On 2017/08/18 15:03:56, juliandoucette wrote: > NIT: This doesn't seem to fit on one line. Is it supposed to? That depends on the size of the IDE being used. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:10: document.getElementById('site-search').classList.toggle('open'); On 2017/08/18 15:03:56, juliandoucette wrote: > NIT: Our code style specifies double quotes :( (I like single quotes) Done. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:10: document.getElementById('site-search').classList.toggle('open'); On 2017/08/18 15:03:56, juliandoucette wrote: > NIT: I usually chain on newlines e.g. > > document > .getElementById(...) > .classList.toggle(...) > > But I don't think our code style guide says anything about this. So it's > up-to-you. Okay. I like to use new lines like that for things like Promises. But for a line that's as short as this one I prefer one line. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:105: height: $search-form-height - 2px; On 2017/08/18 15:03:57, juliandoucette wrote: > Why -2px? It was slightly misaligned if I used the same height. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:106: padding-right: $search-form-height + 5px; On 2017/08/18 15:03:57, juliandoucette wrote: > NIT: It seems strange to calculate an x value using a y. And why not use ems? > Should this not scale with font-size? This padding is used to create an exact square around the submit button (plus 5px for extra space with any text). This is why it uses the height of the search form. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:126: height: 15px; On 2017/08/18 15:03:56, juliandoucette wrote: > NIT: Shouldn't this be an em value to match the font? Yes you're right. Done. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/layout/... static/scss/layout/_navbar.scss:19: padding-top: $md; On 2017/08/18 15:03:57, juliandoucette wrote: > Note: Does our code style guide prefer specific direction properties over > shorthand syntax? I like the way you are doing this personally. Although I would > usually use shorthand for the sake of filesize. NBD (No big deal) anyway. From the coding style guide: > CSS shorthand properties usage is optional. I decided to use specific direction properties in this case in particular because it's sort of a utility class, and I don't want to style properties unnecessarily. The only properties that *need* to be styled in this case is the top and bottom, so I didn't want to override the left and right as well. https://codereview.adblockplus.org/29485575/diff/29517664/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/templates/default.t... templates/default.tmpl:46: <div class="outer"> On 2017/08/18 15:03:58, juliandoucette wrote: > NIT: Does .outer do anything in this patchset? Nope. I'll remove it.
Here are my responses to your comments. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:8: <button id="toggle-search-form" class="unstyled tablet-and-mobile-only"> On 2017/08/21 15:23:18, ire wrote: > On 2017/08/18 15:03:54, juliandoucette wrote: > > Note: It seems super-confusing t ome that the toggle button is a search icon. > > I'm not sure if that matters though (sorry if I brought this up before). > > Yes it did come up, sorry that I didn't address it. > I think the search icon makes sense because its toggling the search form. A > hamburger menu would imply that there is a navigation there when there isn't. > Also, searching is the primary action a user would want to take, so making it > clear that a search bar would appear (and not just a menu) makes sense to me. Acknowledged. I agree. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | translate("external-link-icon", "Image alt text") }}"> On 2017/08/21 15:23:18, ire wrote: > On 2017/08/18 15:03:51, juliandoucette wrote: > > Note: I'm assuming that you will replace "Image alt text" everywhere. > > > > (Sorry if this was already brought up) > > It wasn't brought up before. I didn't intent to replace it. Isn't this the > "content" part of the identifier, which is supposed to describe what type of > string it is (e.g. heading, paragraph, image alt text)? Yup. I seem to have mistaken this for the alt text itself.... Wow :D https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:1: <form id="search-form" action="https://duckduckgo.com" method="GET"> On 2017/08/21 15:23:19, ire wrote: > On 2017/08/18 15:03:55, juliandoucette wrote: > > Note: Regarding size differences between the implementation and the mockup: > > These are not black and white because the images provided are obviously too > big > > (the full width of the design cannot fit on a standard loptop screen at 100% > > zoom). I think we should request a 1280x800 rendering. But this kind of > > polishing doesn't have to block the review. We can push without addressing > NITs > > as long as we document them in low priority tickets. > > Ack. Will address this in a different issue Acknowledged. https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:3: <input id="search" name="q" type="search" placeholder="{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}"> On 2017/08/21 15:23:19, ire wrote: > On 2017/08/18 15:03:55, juliandoucette wrote: > > NIT: The placeholder text is not properly centered on my screen [debian, > > chromium, lowdpi] > > Centered within the input? It isn't supposed to be centered, it is aligned left. Sorry. I meant vertically. https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:4: <input type="hidden" name="sites" value="adblockplus.org"> On 2017/08/21 15:23:19, ire wrote: > On 2017/08/18 15:03:55, juliandoucette wrote: > > NIT: I think there is more border radius in the mockup. > > Done. It was 1px more round :) Acknowledged. I wasn't sure. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:8: document.getElementById('toggle-search-form').addEventListener('click', function(e) { On 2017/08/21 15:23:20, ire wrote: > On 2017/08/18 15:03:56, juliandoucette wrote: > > NIT: This doesn't seem to fit on one line. > > Is it supposed to? That depends on the size of the IDE being used. We follow Mozilla's rule regarding the line length of JavaScript. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... The same rules do not apply to HTML. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:105: height: $search-form-height - 2px; On 2017/08/21 15:23:22, ire wrote: > On 2017/08/18 15:03:57, juliandoucette wrote: > > Why -2px? > > It was slightly misaligned if I used the same height. This seems to ~cause the placeholder text alignment issue I mentioned elsewhere because the text is vertically aligned inside a form element that has less height than the white area behind it. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:106: padding-right: $search-form-height + 5px; On 2017/08/21 15:23:22, ire wrote: > On 2017/08/18 15:03:57, juliandoucette wrote: > > NIT: It seems strange to calculate an x value using a y. And why not use ems? > > Should this not scale with font-size? > > This padding is used to create an exact square around the submit button (plus > 5px for extra space with any text). This is why it uses the height of the search > form. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/layout/... static/scss/layout/_navbar.scss:19: padding-top: $md; On 2017/08/21 15:23:23, ire wrote: > On 2017/08/18 15:03:57, juliandoucette wrote: > > Note: Does our code style guide prefer specific direction properties over > > shorthand syntax? I like the way you are doing this personally. Although I > would > > usually use shorthand for the sake of filesize. NBD (No big deal) anyway. > > From the coding style guide: > > > CSS shorthand properties usage is optional. > > I decided to use specific direction properties in this case in particular > because it's sort of a utility class, and I don't want to style properties > unnecessarily. The only properties that *need* to be styled in this case is the > top and bottom, so I didn't want to override the left and right as well. Acknowledged.
https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/hea... includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | translate("external-link-icon", "Image alt text") }}"> On 2017/08/21 16:28:38, juliandoucette wrote: > On 2017/08/21 15:23:18, ire wrote: > > On 2017/08/18 15:03:51, juliandoucette wrote: > > > Note: I'm assuming that you will replace "Image alt text" everywhere. > > > > > > (Sorry if this was already brought up) > > > > It wasn't brought up before. I didn't intent to replace it. Isn't this the > > "content" part of the identifier, which is supposed to describe what type of > > string it is (e.g. heading, paragraph, image alt text)? > > Yup. I seem to have mistaken this for the alt text itself.... Wow :D Haha :p https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/search-for... includes/search-form.tmpl:3: <input id="search" name="q" type="search" placeholder="{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}"> On 2017/08/21 16:28:39, juliandoucette wrote: > On 2017/08/21 15:23:19, ire wrote: > > On 2017/08/18 15:03:55, juliandoucette wrote: > > > NIT: The placeholder text is not properly centered on my screen [debian, > > > chromium, lowdpi] > > > > Centered within the input? It isn't supposed to be centered, it is aligned > left. > > Sorry. I meant vertically. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/js/main.js#n... static/js/main.js:8: document.getElementById('toggle-search-form').addEventListener('click', function(e) { On 2017/08/21 16:28:40, juliandoucette wrote: > On 2017/08/21 15:23:20, ire wrote: > > On 2017/08/18 15:03:56, juliandoucette wrote: > > > NIT: This doesn't seem to fit on one line. > > > > Is it supposed to? That depends on the size of the IDE being used. > > We follow Mozilla's rule regarding the line length of JavaScript. > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... > > The same rules do not apply to HTML. Okay. I will switch to your style of new lines then. https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29517664/static/scss/compone... static/scss/components/_search-form.scss:105: height: $search-form-height - 2px; On 2017/08/21 16:28:40, juliandoucette wrote: > On 2017/08/21 15:23:22, ire wrote: > > On 2017/08/18 15:03:57, juliandoucette wrote: > > > Why -2px? > > > > It was slightly misaligned if I used the same height. > > This seems to ~cause the placeholder text alignment issue I mentioned elsewhere > because the text is vertically aligned inside a form element that has less > height than the white area behind it. Oh okay, I've removed it. Doesn't seem to cause a significant issue on my end.
Is this ready for review again?
On 2017/08/22 15:02:53, juliandoucette wrote: > Is this ready for review again? Yes
I didn't go into _searchform.scs or _header.scss because these may change significantly if you agree with my comments elsewhere. https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:2: <div class="navbar-wrapper"> See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode68) https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:4: {{ "index" | linkify() }} See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/_header.scss#newcode24) https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:11: <div id="site-search"> 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). https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:12: <? include search-form ?> 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. https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:15: {{ "Adblock Plus" | translate("abp-product-link", "Link") }} NIT/Suggest: "Link text" or "Link label" (it's not a URL) https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-for... includes/search-form.tmpl:2: <label for="search" class="sr-only">{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}</label> Note: I don't know if "Label" is descriptive enough. My intuition says "Input Label", "Text box label", "Search form label". Because one might translate a label differently than a button label differently than an input label. But I don't actually know if that's true. https://codereview.adblockplus.org/29485575/diff/29523654/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/js/main.js#n... static/js/main.js:8: document If you agree with my comments elsewhere then I think it logically follows to make this more generic so that we can re-use it on other websites. In that case, I suggest listening for a toggle button click on the body and toggeling that toggle button's parent or sibling toggle area. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_r... File static/scss/base/_reset.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_r... static/scss/base/_reset.scss:17: html, body, div, span, iframe, Note: I think we should add a reset to website-defaults as a result of our previous discussion and then namespace default-content styles accordingly. After we do that then we can include website-default's reset instead of this one. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:50: @media (min-width: $mobile-breakpoint) See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92). https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:56: .tablet-and-mobile-only The website-default grid considers anything less than $desktop-breakpoint to be tablet. Also see comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92) The same applies below. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:72: .desktop-only The website-default grid considers anything more than $desktop-breakpoint to be desktop. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:68: $navbar-width: $content-width + 100px; I think it's a better idea to start from the $desktop-width and move inwards instead of outwards. e.g. use .container inside .navbar instead of .navbar-wrapper and make $content-width = $desktop-width - 100px. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:72: $site-header-height: 70px; If these variables only apply to the site header like their name implies then I think we can move them to _header.scss. The same applies to $search-form-* below. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:92: $mobile-breakpoint: 544px; The website-default grid considers anything less than tablet-breakpoint to be mobile. We don't currently support phablets (less than 768px and more than 544px) differently than mobile devices (between 320px - 544px). https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/compone... static/scss/components/_search-form.scss:58: @media (min-width: $mobile-breakpoint) See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92) The same applies below. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/content... static/scss/content/_typography.scss:17: body Most of this is already applied by website-defaults _base.scss. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_header.scss:24: .site-title I think these properties seem navbar specific and this name seems site specific. I get the impression that there will not be several #site-title's on a page. Therefore I would suggest writing a more specific ID or a less/more specific class e.g. #site-title or .navbar-header or .navbar-logo. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:21: color: $white; I don't think `.navbar` should have color because "navbar" just describes a layout component which can be different colors (e.g. all of our product websites will have different colour navbars). I suggest using an ID or a more specific class for this e.g. #site-header { color: ... background: color: ... } or .navbar-light, .navbar-dark, .navbar-primary, .navbar-secondary (shades, colors) https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:25: .navbar a See comment above. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:30: .navbar-wrapper See comment [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode68)
Thanks Julian :) Made some changes https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:2: <div class="navbar-wrapper"> 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. https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:4: {{ "index" | linkify() }} 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. https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:11: <div id="site-search"> 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. https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:12: <? include search-form ?> 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. https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/hea... includes/layout/header.tmpl:15: {{ "Adblock Plus" | translate("abp-product-link", "Link") }} On 2017/08/23 14:39:03, juliandoucette wrote: > NIT/Suggest: "Link text" or "Link label" (it's not a URL) Done. https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-for... includes/search-form.tmpl:2: <label for="search" class="sr-only">{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}</label> On 2017/08/23 14:39:04, juliandoucette wrote: > Note: I don't know if "Label" is descriptive enough. My intuition says "Input > Label", "Text box label", "Search form label". Because one might translate a > label differently than a button label differently than an input label. But I > don't actually know if that's true. I'm not sure if it's the case that they may be translated differently, but we don't lose anything being more specific. Done. https://codereview.adblockplus.org/29485575/diff/29523654/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/js/main.js#n... static/js/main.js:8: document On 2017/08/23 14:39:04, juliandoucette wrote: > If you agree with my comments elsewhere then I think it logically follows to > make this more generic so that we can re-use it on other websites. In that case, > I suggest listening for a toggle button click on the body and toggeling that > toggle button's parent or sibling toggle area. Done. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_r... File static/scss/base/_reset.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_r... static/scss/base/_reset.scss:17: html, body, div, span, iframe, On 2017/08/23 14:39:04, juliandoucette wrote: > Note: I think we should add a reset to website-defaults as a result of our > previous discussion and then namespace default-content styles accordingly. After > we do that then we can include website-default's reset instead of this one. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:50: @media (min-width: $mobile-breakpoint) On 2017/08/23 14:39:05, juliandoucette wrote: > See comment > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92). Acknowledged. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:56: .tablet-and-mobile-only On 2017/08/23 14:39:05, juliandoucette wrote: > The website-default grid considers anything less than $desktop-breakpoint to be > tablet. > > Also see comment > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92) > > The same applies below. Ack. See my reply on the quoted comment. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:72: .desktop-only On 2017/08/23 14:39:05, juliandoucette wrote: > The website-default grid considers anything more than $desktop-breakpoint to be > desktop. Ack. See my reply on the quoted comment. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:68: $navbar-width: $content-width + 100px; On 2017/08/23 14:39:07, juliandoucette wrote: > I think it's a better idea to start from the $desktop-width and move inwards > instead of outwards. e.g. use .container inside .navbar instead of > .navbar-wrapper and make $content-width = $desktop-width - 100px. I like the idea of moving inwards instead of outwards, but I think the .container class is a bit restrictive. Since it uses fixed widths, it doesn't really take advantage of the space available like the current implementation does. There are viewport sizes in which the container could/should be wider, but maintains a smaller fixed width. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:72: $site-header-height: 70px; On 2017/08/23 14:39:07, juliandoucette wrote: > If these variables only apply to the site header like their name implies then I > think we can move them to _header.scss. The same applies to $search-form-* > below. I'm undecided on whether it's better to have all the variables in one file, or to have them closer to where they are used. One reason I kept the $site-header-height variable in this file in particular was because the variable was also used in the layout file (when the header was stuck to the top). Even though this is the $site-header-height, I can imagine cases where the variable may be referenced elsewhere (and it would still be correct to call it $site-header-height). Also, I think it's easier to change (and share) when all variables are in one file. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:92: $mobile-breakpoint: 544px; On 2017/08/23 14:39:06, juliandoucette wrote: > The website-default grid considers anything less than tablet-breakpoint to be > mobile. We don't currently support phablets (less than 768px and more than > 544px) differently than mobile devices (between 320px - 544px). What is the reason for not having a difference? Because those are significant differences in size, a mobile phone versus an iPad, for example. I think at the size of tablets (greater than 544px), there is still a lot of screen space to be used and switching to mobile styles (full widths etc) may be underutilising the space available. Also, I think the variable names in themselves are more for us to to easily understand the size, and could as easily be something like $small-breakpoint, $medium-breakpoint, and $large-breakpoint. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/compone... static/scss/components/_search-form.scss:58: @media (min-width: $mobile-breakpoint) On 2017/08/23 14:39:08, juliandoucette wrote: > See comment > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92) > > The same applies below. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/content... static/scss/content/_typography.scss:17: body On 2017/08/23 14:39:08, juliandoucette wrote: > Most of this is already applied by website-defaults _base.scss. Ack. Removed font-size, font-weight and line-height https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_header.scss:24: .site-title On 2017/08/23 14:39:08, juliandoucette wrote: > I think these properties seem navbar specific and this name seems site specific. > I get the impression that there will not be several #site-title's on a page. > Therefore I would suggest writing a more specific ID or a less/more specific > class e.g. #site-title or .navbar-header or .navbar-logo. Done. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:21: color: $white; On 2017/08/23 14:39:09, juliandoucette wrote: > I don't think `.navbar` should have color because "navbar" just describes a > layout component which can be different colors (e.g. all of our product websites > will have different colour navbars). I suggest using an ID or a more specific > class for this e.g. > > #site-header > { > color: ... > background: color: ... > } > > or > > .navbar-light, .navbar-dark, .navbar-primary, .navbar-secondary (shades, colors) I agree with you that "navbar" is more a description on layout. Here is the issue: 1. There are only two "navbars" in this site (header and footer), and they both have this colour 2. If I move this colour to a different class, there will be an additional class (e..g navbar-primary) being applied to both of these elements (or the same style to both IDs). Considering that there is no other navbar in this site, it seems like an abstraction that is not necessary 3. I can imagine the "navbar" class being moved to website-defaults. That class would not have a colour applied (or would have some default colour to be overriden). But that doesn't stop us from applying the colour we want for navbars across a particular site to the same "navbar" class (sort of like an extension) What do you think? https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:25: .navbar a On 2017/08/23 14:39:09, juliandoucette wrote: > See comment above. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:30: .navbar-wrapper On 2017/08/23 14:39:09, juliandoucette wrote: > See comment > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode68) Acknowledged.
https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-for... File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-for... includes/search-form.tmpl:2: <label for="search" class="sr-only">{{ "Search Adblock Plus Help" | translate("search-form-label", "Label") }}</label> On 2017/08/25 08:27:07, ire wrote: > On 2017/08/23 14:39:04, juliandoucette wrote: > > Note: I don't know if "Label" is descriptive enough. My intuition says "Input > > Label", "Text box label", "Search form label". Because one might translate a > > label differently than a button label differently than an input label. But I > > don't actually know if that's true. > > I'm not sure if it's the case that they may be translated differently, but we > don't lose anything being more specific. > > Done. Acknowledged. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:46: // Responsive /////////////////////////////////////////////////////////////// It may be worth proposing that we rename $size-breakpoint to $size-min-width based on the miscommunications below. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:50: @media (min-width: $mobile-breakpoint) On 2017/08/25 08:27:08, ire wrote: > On 2017/08/23 14:39:05, juliandoucette wrote: > > See comment > > > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92). > > Acknowledged. Note: This should be min-width $tablet-breakpoint or min-width: $phablet-breakpoint if we keep the existing naming scheme. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:56: .tablet-and-mobile-only On 2017/08/25 08:27:08, ire wrote: Ack. See my reply on the quoted comment. Note: This should be min-width: $desktop-breakpoint if we keep the existing naming scheme. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:72: .desktop-only On 2017/08/25 08:27:09, ire wrote: > Ack. See my reply on the quoted comment. Note: This should be min-width: $desktop-breakpoint if we keep the existing naming scheme. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:68: $navbar-width: $content-width + 100px; On 2017/08/25 08:27:10, ire wrote: > I like the idea of moving inwards instead of outwards, but I think the > .container class is a bit restrictive. Since it uses fixed widths, it doesn't > really take advantage of the space available like the current implementation > does. There are viewport sizes in which the container could/should be wider, but > maintains a smaller fixed width. Acknowledged. I see what you mean now. And I agree with your implementation. It may be useful to add a comment about this. I'll leave it up to you to decide whether you think this is necessary. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:72: $site-header-height: 70px; On 2017/08/25 08:27:11, ire wrote: > On 2017/08/23 14:39:07, juliandoucette wrote: > > If these variables only apply to the site header like their name implies then > I > > think we can move them to _header.scss. The same applies to $search-form-* > > below. > > I'm undecided on whether it's better to have all the variables in one file, or > to have them closer to where they are used. One reason I kept the > $site-header-height variable in this file in particular was because the variable > was also used in the layout file (when the header was stuck to the top). Even > though this is the $site-header-height, I can imagine cases where the variable > may be referenced elsewhere (and it would still be correct to call it > $site-header-height). Also, I think it's easier to change (and share) when all > variables are in one file. Acknowledged. I concede this point. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:92: $mobile-breakpoint: 544px; On 2017/08/25 08:27:09, ire wrote: > What is the reason for not having a difference? Because those are significant > differences in size, a mobile phone versus an iPad, for example. I think at the > size of tablets (greater than 544px), there is still a lot of screen space to be > used and switching to mobile styles (full widths etc) may be underutilising the > space available. There seems to be a miscommunication here about where a phone ends and a tablet begins. You are saying that a phone ends and a tablet begins at 544. I am saying that a phone ends and a tablet begins at 768. I chose the tablet breakpoint (minimum width) (768) based on [the width of the browser screen on the first ipad when held portrait, the width at which you can comfortably fit two columns in most layouts]. I will certainly acknowledge that there is room between 544 and 768 for optimization. And encourage you to optimize for this size (which I would call "phablet size") if you can. (I didn't include a phablet width or breakpoint in website-defaults grid because none of our existing website designs explicitly support these sizes.) > Also, I think the variable names in themselves are more for us to to easily > understand the size, and could as easily be something like $small-breakpoint, > $medium-breakpoint, and $large-breakpoint. I agree. But I'm not sure that [small, medium, large] are better than [mobile, tablet, desktop]; yet? https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:21: color: $white; On 2017/08/25 08:27:13, ire wrote: > On 2017/08/23 14:39:09, juliandoucette wrote: > > I don't think `.navbar` should have color because "navbar" just describes a > > layout component which can be different colors (e.g. all of our product > websites > > will have different colour navbars). I suggest using an ID or a more specific > > class for this e.g. > > > > #site-header > > { > > color: ... > > background: color: ... > > } > > > > or > > > > .navbar-light, .navbar-dark, .navbar-primary, .navbar-secondary (shades, > colors) > > I agree with you that "navbar" is more a description on layout. Here is the > issue: > > 1. There are only two "navbars" in this site (header and footer), and they both > have this colour > 2. If I move this colour to a different class, there will be an additional class > (e..g navbar-primary) being applied to both of these elements (or the same style > to both IDs). Considering that there is no other navbar in this site, it seems > like an abstraction that is not necessary > 3. I can imagine the "navbar" class being moved to website-defaults. That class > would not have a colour applied (or would have some default colour to be > overriden). But that doesn't stop us from applying the colour we want for > navbars across a particular site to the same "navbar" class (sort of like an > extension) > > What do you think? I think you have the right idea. If we don't add color to `.navbar` then we will end up extending `.navbar` by class or ID per website or adding an additional class. I think that we could optimize this by providing default colors based on the primary color scheme e.g. we could set .navbar background to $primary-background etc or do this by proxy by $navbar-background with default of $primary-background etc. https://codereview.adblockplus.org/29485575/diff/29527569/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29527569/includes/layout/hea... includes/layout/header.tmpl:24: <a href="https://adblockplus.org/"> NIT: You can't see the text-decoration under the image. As a result, we have an empty space underlined beside the image. https://codereview.adblockplus.org/29485575/diff/29527569/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/js/main.js#n... static/js/main.js:2: document.addEventListener("DOMContentLoaded", function() NIT: We can support this in IE8 via https://github.com/WebReflection/ie8 https://codereview.adblockplus.org/29485575/diff/29527569/static/js/main.js#n... static/js/main.js:15: .classList.toggle("open") NIT: We can support this in IE < 10 via https://github.com/eligrey/classList.js https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/compone... static/scss/components/_search-form.scss:107: html[dir="rtl"] NIT: We should separate these by the same logic we separate @media queries. https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... static/scss/content/_typography.scss:19: color: $font-color-default; NIT: color and font-family are set for HTML by website-default _base.scss. It's not clear why they should be overwritten here? https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/layout/... static/scss/layout/_header.scss:32: height: $lg - 5px; NIT: It seem strange to shrink the logo and not the navbar-height on smaller screens :/ https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/layout/... static/scss/layout/_header.scss:58: #site-title NIT: Move rtl #site-title above .toggle-navbar-collapse
Updates ready. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:50: @media (min-width: $mobile-breakpoint) On 2017/08/29 16:17:24, juliandoucette wrote: > On 2017/08/25 08:27:08, ire wrote: > > On 2017/08/23 14:39:05, juliandoucette wrote: > > > See comment > > > > > > [here](https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_variables.scss#newcode92). > > > > Acknowledged. > > Note: This should be min-width $tablet-breakpoint or min-width: > $phablet-breakpoint if we keep the existing naming scheme. Done. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:56: .tablet-and-mobile-only On 2017/08/29 16:17:23, juliandoucette wrote: > On 2017/08/25 08:27:08, ire wrote: > Ack. See my reply on the quoted comment. > > Note: This should be min-width: $desktop-breakpoint if we keep the existing > naming scheme. This is changed to .phablet-and-mobile-only, width min-width: $tablet-breakpoint https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:72: .desktop-only On 2017/08/29 16:17:24, juliandoucette wrote: > On 2017/08/25 08:27:09, ire wrote: > > Ack. See my reply on the quoted comment. > > Note: This should be min-width: $desktop-breakpoint if we keep the existing > naming scheme. This is max-width: $desktop-breakpoint, because I'm using max-width instead so i don't have to apply a display property other than none (we don't know what the display property of the element this class is applied to should be) https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:68: $navbar-width: $content-width + 100px; On 2017/08/29 16:17:27, juliandoucette wrote: > On 2017/08/25 08:27:10, ire wrote: > > I like the idea of moving inwards instead of outwards, but I think the > > .container class is a bit restrictive. Since it uses fixed widths, it doesn't > > really take advantage of the space available like the current implementation > > does. There are viewport sizes in which the container could/should be wider, > but > > maintains a smaller fixed width. > > Acknowledged. > > I see what you mean now. And I agree with your implementation. It may be useful > to add a comment about this. I'll leave it up to you to decide whether you think > this is necessary. I don't think my comment would be more explanatory than "navbar width is content width plus 100px", so I'll leave it out https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_v... static/scss/base/_variables.scss:92: $mobile-breakpoint: 544px; On 2017/08/29 16:17:26, juliandoucette wrote: > On 2017/08/25 08:27:09, ire wrote: > > What is the reason for not having a difference? Because those are significant > > differences in size, a mobile phone versus an iPad, for example. I think at > the > > size of tablets (greater than 544px), there is still a lot of screen space to > be > > used and switching to mobile styles (full widths etc) may be underutilising > the > > space available. > > There seems to be a miscommunication here about where a phone ends and a tablet > begins. You are saying that a phone ends and a tablet begins at 544. I am saying > that a phone ends and a tablet begins at 768. > > I chose the tablet breakpoint (minimum width) (768) based on [the width of the > browser screen on the first ipad when held portrait, the width at which you can > comfortably fit two columns in most layouts]. > > I will certainly acknowledge that there is room between 544 and 768 for > optimization. And encourage you to optimize for this size (which I would call > "phablet size") if you can. > > (I didn't include a phablet width or breakpoint in website-defaults grid because > none of our existing website designs explicitly support these sizes.) > > > Also, I think the variable names in themselves are more for us to to easily > > understand the size, and could as easily be something like $small-breakpoint, > > $medium-breakpoint, and $large-breakpoint. > > I agree. But I'm not sure that [small, medium, large] are better than [mobile, > tablet, desktop]; yet? Changed to $phablet-breakpoint https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/layout/... static/scss/layout/_navbar.scss:21: color: $white; On 2017/08/29 16:17:27, juliandoucette wrote: > On 2017/08/25 08:27:13, ire wrote: > > On 2017/08/23 14:39:09, juliandoucette wrote: > > > I don't think `.navbar` should have color because "navbar" just describes a > > > layout component which can be different colors (e.g. all of our product > > websites > > > will have different colour navbars). I suggest using an ID or a more > specific > > > class for this e.g. > > > > > > #site-header > > > { > > > color: ... > > > background: color: ... > > > } > > > > > > or > > > > > > .navbar-light, .navbar-dark, .navbar-primary, .navbar-secondary (shades, > > colors) > > > > I agree with you that "navbar" is more a description on layout. Here is the > > issue: > > > > 1. There are only two "navbars" in this site (header and footer), and they > both > > have this colour > > 2. If I move this colour to a different class, there will be an additional > class > > (e..g navbar-primary) being applied to both of these elements (or the same > style > > to both IDs). Considering that there is no other navbar in this site, it seems > > like an abstraction that is not necessary > > 3. I can imagine the "navbar" class being moved to website-defaults. That > class > > would not have a colour applied (or would have some default colour to be > > overriden). But that doesn't stop us from applying the colour we want for > > navbars across a particular site to the same "navbar" class (sort of like an > > extension) > > > > What do you think? > > I think you have the right idea. If we don't add color to `.navbar` then we will > end up extending `.navbar` by class or ID per website or adding an additional > class. I think that we could optimize this by providing default colors based on > the primary color scheme e.g. we could set .navbar background to > $primary-background etc or do this by proxy by $navbar-background with default > of $primary-background etc. I think this is a good idea we can implement when we add the navbar to website-defaults. https://codereview.adblockplus.org/29485575/diff/29527569/includes/layout/hea... File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29527569/includes/layout/hea... includes/layout/header.tmpl:24: <a href="https://adblockplus.org/"> On 2017/08/29 16:17:28, juliandoucette wrote: > NIT: You can't see the text-decoration under the image. As a result, we have an > empty space underlined beside the image. Done. Removed the empty space between the text and the image https://codereview.adblockplus.org/29485575/diff/29527569/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/js/main.js#n... static/js/main.js:2: document.addEventListener("DOMContentLoaded", function() On 2017/08/29 16:17:29, juliandoucette wrote: > NIT: We can support this in IE8 via https://github.com/WebReflection/ie8 This was already added, but I had an incorrect filename :/ https://codereview.adblockplus.org/29485575/diff/29527569/static/js/main.js#n... static/js/main.js:15: .classList.toggle("open") On 2017/08/29 16:17:29, juliandoucette wrote: > NIT: We can support this in IE < 10 via https://github.com/eligrey/classList.js Done. https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/compone... File static/scss/components/_search-form.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/compone... static/scss/components/_search-form.scss:107: html[dir="rtl"] On 2017/08/29 16:17:30, juliandoucette wrote: > NIT: We should separate these by the same logic we separate @media queries. Done. https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... static/scss/content/_typography.scss:19: color: $font-color-default; On 2017/08/29 16:17:30, juliandoucette wrote: > NIT: color and font-family are set for HTML by website-default _base.scss. It's > not clear why they should be overwritten here? For some reason (not sure why), the variables aren't being updated. So if I don't include this, the color and font-family and those set by the variables in website-defaults. I have changed this selector to html to be consistent https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/layout/... File static/scss/layout/_header.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/layout/... static/scss/layout/_header.scss:32: height: $lg - 5px; On 2017/08/29 16:17:31, juliandoucette wrote: > NIT: It seem strange to shrink the logo and not the navbar-height on smaller > screens :/ You're right. I've kept the logo the same size. Didn't seem necessary to change all the sizes everywhere for just a 5px difference in size https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/layout/... static/scss/layout/_header.scss:58: #site-title On 2017/08/29 16:17:32, juliandoucette wrote: > NIT: Move rtl #site-title above .toggle-navbar-collapse Done.
I think we should push this after you address the issues below. I found a few NITs that managed to slip through the cracks of this and the last review. But I'd rather address them later or submit a Patchset for your review since they are non-blocking. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:72: .desktop-only On 2017/09/01 10:57:17, ire wrote: > On 2017/08/29 16:17:24, juliandoucette wrote: > > On 2017/08/25 08:27:09, ire wrote: > > > Ack. See my reply on the quoted comment. > > > > Note: This should be min-width: $desktop-breakpoint if we keep the existing > > naming scheme. > > This is max-width: $desktop-breakpoint, because I'm using max-width instead so i > don't have to apply a display property other than none (we don't know what the > display property of the element this class is applied to should be) Acknowledged. But it should be $tablet-breakpoint - 1 (otherwise you will create a 1px grey area). https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... static/scss/content/_typography.scss:19: color: $font-color-default; On 2017/09/01 10:57:23, ire wrote: > On 2017/08/29 16:17:30, juliandoucette wrote: > > NIT: color and font-family are set for HTML by website-default _base.scss. > It's > > not clear why they should be overwritten here? > > For some reason (not sure why), the variables aren't being updated. So if I > don't include this, the color and font-family and those set by the variables in > website-defaults. > > I have changed this selector to html to be consistent You must include your custom variables before website-defaults variables so that !default works properly (!default =~ set this to that if this is not already set).
> I think we should push this after you address the issues below. I found a few > NITs that managed to slip through the cracks of this and the last review. But > I'd rather address them later or submit a Patchset for your review since they > are non-blocking. Okay https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_u... static/scss/base/_utilities.scss:72: .desktop-only On 2017/09/01 11:43:44, juliandoucette wrote: > On 2017/09/01 10:57:17, ire wrote: > > On 2017/08/29 16:17:24, juliandoucette wrote: > > > On 2017/08/25 08:27:09, ire wrote: > > > > Ack. See my reply on the quoted comment. > > > > > > Note: This should be min-width: $desktop-breakpoint if we keep the existing > > > naming scheme. > > > > This is max-width: $desktop-breakpoint, because I'm using max-width instead so > i > > don't have to apply a display property other than none (we don't know what the > > display property of the element this class is applied to should be) > > Acknowledged. But it should be $tablet-breakpoint - 1 (otherwise you will create > a 1px grey area). Done. https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29485575/diff/29527569/static/scss/content... static/scss/content/_typography.scss:19: color: $font-color-default; On 2017/09/01 11:43:44, juliandoucette wrote: > On 2017/09/01 10:57:23, ire wrote: > > On 2017/08/29 16:17:30, juliandoucette wrote: > > > NIT: color and font-family are set for HTML by website-default _base.scss. > > It's > > > not clear why they should be overwritten here? > > > > For some reason (not sure why), the variables aren't being updated. So if I > > don't include this, the color and font-family and those set by the variables > in > > website-defaults. > > > > I have changed this selector to html to be consistent > > You must include your custom variables before website-defaults variables so that > !default works properly (!default =~ set this to that if this is not already > set). Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
