Issue 29488555: Issue 5406 - Create Site Footer Component for Help Center (Closed)
|
Created:
July 13, 2017, 7:57 a.m. by ire Modified:
Sept. 18, 2017, 3:19 p.m. Reviewers:
juliandoucette Visibility:
Public. |
DescriptionIssue 5406 - Create Site Footer Component for Help Center
Patch Set 1 #
Total comments: 3
Patch Set 2 : Updates with Site Header Component review #
Total comments: 1
Patch Set 3 : Rebase #
Total comments: 63
Patch Set 4 : Rebased and updated #
Total comments: 37
Patch Set 5 : Use menu pattern on custom select, horizontal list utliity class, etc #
Total comments: 11
Patch Set 6 : No-js fixes #
Total comments: 5
Patch Set 7 : Fix spacing in custom-select-options li #Patch Set 8 : Add default no-js class to html #
Total comments: 7
Patch Set 9 : Remove langnames commit, use website-default breakpoint variables #
Total comments: 1
MessagesTotal messages: 21
First patch :) There are some files here that are basically the same as the issue for the header, e.g. the navbar. Please leave comments for those files in that review. https://codereview.adblockplus.org/29488555/diff/29488556/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29488556/static/scss/layout/... static/scss/layout/_footer.scss:22: #site-footer .one-fourth I'm not sure about styling the grid class here (".one-fourth") https://codereview.adblockplus.org/29488555/diff/29488556/static/scss/layout/... static/scss/layout/_footer.scss:24: margin-bottom: 10px; I intend to abstract some of these sizes into variables like in acceptableads. I just haven't figured out what those sizes should be yet. https://codereview.adblockplus.org/29488555/diff/29488556/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29488556/static/scss/layout/... static/scss/layout/_grid.scss:15: // along with help.eyeo.com. If not, see <http://www.gnu.org/licenses/>. I had to change the way the grid from website-defaults works a bit here.
Minor updates based on review of Site Header Component https://codereview.adblockplus.org/29488555/diff/29490585/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29488555/diff/29490585/static/scss/content... static/scss/content/_typography.scss:37: .muted Changed this class name to go with the style guide format
On what base does this latest patchset apply?
On 2017/07/25 15:34:06, juliandoucette wrote: > On what base does this latest patchset apply? The boilerplate
Can you rebase this onto the tip of help.eyeo.com?
(Or the header component)
On 2017/08/09 15:49:43, juliandoucette wrote: > Can you rebase this onto the tip of help.eyeo.com? Done.
Looks like many of these styles may update with the header component. I hope that these comments are helpful anyway. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> NIT: I think this should be hidden by default and shown when JS is enabled https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:8: <ul id="language-options" class="custom-select-options" aria-hidden="true"> NIT: aria-hidden should be added by js because this is not hidden by default without js https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:21: <ul> NIT: This list should stretch the footer if it's heavily populated and JavaScript is disabled. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:22: <li><a href="">Legal</a></li> Note: This will probably be the "legal" or "impressum" page. I don't really care if you add an href here or not. https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js#n... static/js/main.js:2: document.addEventListener("DOMContentLoaded", function() { NIT: We are supposed to put brackets on the next line :( (We could bring this up in a frontend dev meeting because I think it makes sense for these to be on the same line.) https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js#n... static/js/main.js:23: customSelects[i].addEventListener("click", onClickCustomSelect, false); NIT: We could handle this event on the body instead. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/base/_v... static/scss/base/_variables.scss:54: // Font sizes (general) Note: I think that these changes will be irrelevant if you base this patch on the header component. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected NIT: It seem strange to have a class that ends in "selected" always visible https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:27: padding: 3px 10px; NIT: Padding should probably be in em https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:28: border: 1px solid $gray-medium; NIT: Suggest: 2px border. I don't think this passes the width/contrast accessibility ratio. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:32: font-family: $primary-font; NIT: why bind this control to one font and size? (why not inherit these properties) https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:40: bottom: 120%; NIT: Could we use 100% + margin in em effectively instead? https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:42: padding: 10px; NIT: I think em makes more sense here? https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:71: .no-js NIT: I suggest adding no-js styles below items like we do rtl styles https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/content... static/scss/content/_typography.scss:19: body Note: I'm assuming that these changes will be irrelevant if this patchset is rebased on top of the header component. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/content... static/scss/content/_typography.scss:35: // Font Colours //////////////////////////////////////////////////////////// I think these classes belong in a utility SCSS file; not typography. Strong opinion; weakly held (if I'm using that expression correctly :/). https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:19: font-size: $small-font; I don't think this does anything after all other styles are considered. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:22: #site-footer .one-fourth I think it makes more sense to add margin (in em not px) to the custom-select; not the row. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:58: &:not(:last-child) not is not supported by IE https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:67: &:last-child last-child is not supported by IE https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:76: [dir="rtl"] NIT: Please move rtl styles directly below ltr styles https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_grid.scss:17: .column Note: I'm assuming that these will not differ from website-defaults grid https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_navbar.scss:17: .navbar Note: I'm assuming that these changes will be irrelevant if this Patchset is rebased on top of the header component. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/main.sc... static/scss/main.scss:15: // along with acceptableads.org. If not, see <http://www.gnu.org/licenses/>. Note: I'm assuming these changes will be irrelevant if this Patchset is rebased on top of the header component.
On 2017/08/22 14:33:44, juliandoucette wrote: > Looks like many of these styles may update with the header component. I hope > that these comments are helpful anyway. Thanks Julian! I think you're right. If you don't mind I would like to pause on this until the header component is ready since much of the styles depend on that commit
> Thanks Julian! I think you're right. If you don't mind I would like to pause on this until the header component is ready since much of the styles depend on that commit No problamo. Hope you're enjoying conferences now :)
I've rebased this on top of the site header commit, and made changes based on the comments. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> On 2017/08/22 14:33:41, juliandoucette wrote: > NIT: I think this should be hidden by default and shown when JS is enabled This is sort of what I've done already, since I'm hiding/showing it based on the .no-js class. The alternative would be to change my CSS from: .custom-select-selected { display: inline-block; } .no-js .custom-select-selected { display: none; } to: .custom-select-selected { display: none; } .js .custom-select-selected { display: inline-block; } Which is basically the same https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:8: <ul id="language-options" class="custom-select-options" aria-hidden="true"> On 2017/08/22 14:33:41, juliandoucette wrote: > NIT: aria-hidden should be added by js because this is not hidden by default > without js Done. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:21: <ul> On 2017/08/22 14:33:41, juliandoucette wrote: > NIT: This list should stretch the footer if it's heavily populated and > JavaScript is disabled. It doesn't seem like this list will be heavily populated. I think we can modify how it works if/when more content is added to it. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:22: <li><a href="">Legal</a></li> On 2017/08/22 14:33:41, juliandoucette wrote: > Note: This will probably be the "legal" or "impressum" page. I don't really care > if you add an href here or not. Acknowledged. I will add it when the page is added. https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js#n... static/js/main.js:2: document.addEventListener("DOMContentLoaded", function() { On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: We are supposed to put brackets on the next line :( > > (We could bring this up in a frontend dev meeting because I think it makes sense > for these to be on the same line.) Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js#n... static/js/main.js:23: customSelects[i].addEventListener("click", onClickCustomSelect, false); On 2017/08/22 14:33:41, juliandoucette wrote: > NIT: We could handle this event on the body instead. Agreed. I would like to change the way both this and the event listener on the navbarCollapse work so I'll create a separate issue for this https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/base/_v... File static/scss/base/_variables.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/base/_v... static/scss/base/_variables.scss:54: // Font sizes (general) On 2017/08/22 14:33:42, juliandoucette wrote: > Note: I think that these changes will be irrelevant if you base this patch on > the header component. Yes, they are https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: It seem strange to have a class that ends in "selected" always visible The "selected" refers to the fact that it is the currently selected option, and there will always be a currently selected option in a select field (even if its a blank option) https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:27: padding: 3px 10px; On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: Padding should probably be in em Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:28: border: 1px solid $gray-medium; On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: Suggest: 2px border. I don't think this passes the width/contrast > accessibility ratio. Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:32: font-family: $primary-font; On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: why bind this control to one font and size? (why not inherit these > properties) Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:40: bottom: 120%; On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: Could we use 100% + margin in em effectively instead? Great idea! Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:42: padding: 10px; On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: I think em makes more sense here? Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:71: .no-js On 2017/08/22 14:33:42, juliandoucette wrote: > NIT: I suggest adding no-js styles below items like we do rtl styles In general I agree. In this case, there is actually no styling for the particular selectors (besides .custom-select-options), so they would be placed in the same position anyway. The only difference would be ungrouping the .no-js styles (which could be what we want, I'm not sure on that). https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/content... static/scss/content/_typography.scss:19: body On 2017/08/22 14:33:43, juliandoucette wrote: > Note: I'm assuming that these changes will be irrelevant if this patchset is > rebased on top of the header component. Yes most of them are https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/content... static/scss/content/_typography.scss:35: // Font Colours //////////////////////////////////////////////////////////// On 2017/08/22 14:33:43, juliandoucette wrote: > I think these classes belong in a utility SCSS file; not typography. > > Strong opinion; weakly held (if I'm using that expression correctly :/). Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:19: font-size: $small-font; On 2017/08/22 14:33:43, juliandoucette wrote: > I don't think this does anything after all other styles are considered. Which other styles? From my test, this makes a difference to the font size. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:22: #site-footer .one-fourth On 2017/08/22 14:33:43, juliandoucette wrote: > I think it makes more sense to add margin (in em not px) to the custom-select; > not the row. The margin is more to separate the first row from the second. The margin should be applied even if there was more content in that row than just the custom-select https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:58: &:not(:last-child) On 2017/08/22 14:33:43, juliandoucette wrote: > not is not supported by IE Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:67: &:last-child On 2017/08/22 14:33:43, juliandoucette wrote: > last-child is not supported by IE :last-child is supported by IE 9+ (http://caniuse.com/#feat=css-sel3) https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:76: [dir="rtl"] On 2017/08/22 14:33:43, juliandoucette wrote: > NIT: Please move rtl styles directly below ltr styles Done. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_grid.scss:17: .column On 2017/08/22 14:33:43, juliandoucette wrote: > Note: I'm assuming that these will not differ from website-defaults grid These do differ from the website-defaults grid in a couple ways: 1. Removes the horizontal padding 2. Sets the breakpoints for for the columns to happen earlier. (e.g. in website-defaults the changes I made happen at $tablet-breakpoint happen at $desktop-breakpoint) I'm not sure what we should do about this. I agree with what you've said from the previous review that just simply overriding the website-defaults in this way is probably not the best method. Perhaps there can be a modifier class instead? https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_navbar.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_navbar.scss:17: .navbar On 2017/08/22 14:33:44, juliandoucette wrote: > Note: I'm assuming that these changes will be irrelevant if this Patchset is > rebased on top of the header component. Yes they are https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/main.sc... static/scss/main.scss:15: // along with acceptableads.org. If not, see <http://www.gnu.org/licenses/>. On 2017/08/22 14:33:44, juliandoucette wrote: > Note: I'm assuming these changes will be irrelevant if this Patchset is rebased > on top of the header component. Yes they are
Thanks Ire! https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> On 2017/09/04 20:28:06, ire wrote: > This is sort of what I've done already, since I'm hiding/showing it based on the > .no-js class. The alternative would be to change my CSS from: > > .custom-select-selected { display: inline-block; } > .no-js .custom-select-selected { display: none; } > > to: > > .custom-select-selected { display: none; } > .js .custom-select-selected { display: inline-block; } > > Which is basically the same Acknowledged. But we should be consistent. Doesn't the same logic that applies to progressive enhancement instead of graceful degradation, and mobile-first, also apply to no-js-first and IE-first? aka start with the worst case and improve from there (implying that our non-classed styles should be no-js styles in most cases). https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:21: <ul> On 2017/09/04 20:28:06, ire wrote: > It doesn't seem like this list will be heavily populated. I think we can modify > how it works if/when more content is added to it. - I think that we are planning to translate the help center into many languages - I don't mind if you address this later, as long as you document it in an issue https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:22: <li><a href="">Legal</a></li> On 2017/09/04 20:28:04, ire wrote: > On 2017/08/22 14:33:41, juliandoucette wrote: > > Note: This will probably be the "legal" or "impressum" page. I don't really > care > > if you add an href here or not. > > Acknowledged. I will add it when the page is added. Acknowledged. https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/js/main.js#n... static/js/main.js:23: customSelects[i].addEventListener("click", onClickCustomSelect, false); On 2017/09/04 20:28:06, ire wrote: > On 2017/08/22 14:33:41, juliandoucette wrote: > > NIT: We could handle this event on the body instead. > > Agreed. I would like to change the way both this and the event listener on the > navbarCollapse work so I'll create a separate issue for this Acknowledged. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected On 2017/09/04 20:28:07, ire wrote: > The "selected" refers to the fact that it is the currently selected option, and > there will always be a currently selected option in a select field (even if its > a blank option) Acknowledged. *embarrassed face* :D https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:71: .no-js On 2017/09/04 20:28:07, ire wrote: > In general I agree. In this case, there is actually no styling for the > particular selectors (besides .custom-select-options), so they would be placed > in the same position anyway. The only difference would be ungrouping the .no-js > styles (which could be what we want, I'm not sure on that). There is for .custom-select-selected and .custom-select-options (which caused me to notice). https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:19: font-size: $small-font; On 2017/09/04 20:28:08, ire wrote: > On 2017/08/22 14:33:43, juliandoucette wrote: > > I don't think this does anything after all other styles are considered. > > Which other styles? From my test, this makes a difference to the font size. I was mislead by the effect of font-size: 100%; sorry :D https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:67: &:last-child On 2017/09/04 20:28:08, ire wrote: > On 2017/08/22 14:33:43, juliandoucette wrote: > > last-child is not supported by IE > > :last-child is supported by IE 9+ (http://caniuse.com/#feat=css-sel3) I suggest that we support IE 8 by default or document that we don't support it in an issue which we may later resolve or reject ( It's hard to have a strong opinion in favour of supporting IE 8 :D ) ( because ABP currently supports IE 8 and so does our browserlistrc; currently ). https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_grid.scss:17: .column On 2017/09/04 20:28:08, ire wrote: > On 2017/08/22 14:33:43, juliandoucette wrote: > > Note: I'm assuming that these will not differ from website-defaults grid > > These do differ from the website-defaults grid in a couple ways: > > 1. Removes the horizontal padding > 2. Sets the breakpoints for for the columns to happen earlier. (e.g. in > website-defaults the changes I made happen at $tablet-breakpoint happen at > $desktop-breakpoint) > > I'm not sure what we should do about this. I agree with what you've said from > the previous review that just simply overriding the website-defaults in this way > is probably not the best method. Perhaps there can be a modifier class instead? See comments in latest patchset. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:1: <footer id="site-footer" class="navbar"> Note: (Added after the comments below) It looks like we should considering using a menubar / menu / menu button pattern as described here https://www.w3.org/TR/wai-aria-practices/#menu, https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.... https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> Is there a reason that you didn't add aria-haspopup and/or aria-expanded? https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:8: <ul id="language-options" class="custom-select-options"> Is there a reason that you didn't add aria-labelledby? https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:9: {% for lang in available_locales %} NIT: These are locale codes not languages or language codes? ( <- that might be a stretch ) https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:12: {{config.get("langnames", lang)}} NIT: You are spacing {{ }} inconsistently in this file. I suggest always adding one space after opening and before closing. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... static/scss/base/_utilities.scss:101: text-decoration: underline; It seems strange for a color named class to apply an underline? https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected NIT: Do help-center buttons share some of these styles? (Ack that you can separate them later if [they do, you want]). https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/content... static/scss/content/_typography.scss:21: color: inherit; I think that you are trying to override the default link color because links outside of content areas are not accent colored. If that is the case then doesn't it make more sense to namespace website-default _content.scss styles and apply this style inside website-default _reset.scss? https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:32: #site-footer .three-fourths NIT: This seems too general. We could conceivably add another row with three-fourths before one-fourth. For that reason, I would be more specific e.g. #site-footer-nav or .align-right. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:56: #site-footer-nav ul NIT: Shouldn't we do this with a horizontal list utility class? https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:72: border-right: 1px solid $gray; NIT: Why not border-right the first-child or border-left the last-child instead of border-righting everyone and unborder-righting the last-child? The same logic may apply to spacing. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:19: padding: 0; Why eliminate the gutter width? Note: You didn't overwrite the negative margin on .row :S https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:22: @media(min-width: $phablet-breakpoint) Note: It seems like we could make sizes conditional (in addition to configurable) in website-defaults. I think this is a good idea.
> Thanks Ire! You're welcome :) . Updates ready. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> On 2017/09/06 17:48:17, juliandoucette wrote: > On 2017/09/04 20:28:06, ire wrote: > > This is sort of what I've done already, since I'm hiding/showing it based on > the > > .no-js class. The alternative would be to change my CSS from: > > > > .custom-select-selected { display: inline-block; } > > .no-js .custom-select-selected { display: none; } > > > > to: > > > > .custom-select-selected { display: none; } > > .js .custom-select-selected { display: inline-block; } > > > > Which is basically the same > > Acknowledged. But we should be consistent. Doesn't the same logic that applies > to progressive enhancement instead of graceful degradation, and mobile-first, > also apply to no-js-first and IE-first? aka start with the worst case and > improve from there (implying that our non-classed styles should be no-js styles > in most cases). This makes sense, you're right. Will update. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/foo... includes/layout/footer.tmpl:21: <ul> On 2017/09/06 17:48:17, juliandoucette wrote: > On 2017/09/04 20:28:06, ire wrote: > > It doesn't seem like this list will be heavily populated. I think we can > modify > > how it works if/when more content is added to it. > > - I think that we are planning to translate the help center into many languages > - I don't mind if you address this later, as long as you document it in an issue I think you meant to leave this comment on the language list? This list only contains the Legal and Privacy policy, which shouldn't be affected by the number of translated languages. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected On 2017/09/06 17:48:19, juliandoucette wrote: > On 2017/09/04 20:28:07, ire wrote: > > The "selected" refers to the fact that it is the currently selected option, > and > > there will always be a currently selected option in a select field (even if > its > > a blank option) > > Acknowledged. *embarrassed face* :D :P https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/compone... static/scss/components/_select.scss:71: .no-js On 2017/09/06 17:48:20, juliandoucette wrote: > On 2017/09/04 20:28:07, ire wrote: > > In general I agree. In this case, there is actually no styling for the > > particular selectors (besides .custom-select-options), so they would be placed > > in the same position anyway. The only difference would be ungrouping the > .no-js > > styles (which could be what we want, I'm not sure on that). > > There is for .custom-select-selected and .custom-select-options (which caused me > to notice). I meant in this particular block. But I've changed the order a bit. https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:19: font-size: $small-font; On 2017/09/06 17:48:20, juliandoucette wrote: > On 2017/09/04 20:28:08, ire wrote: > > On 2017/08/22 14:33:43, juliandoucette wrote: > > > I don't think this does anything after all other styles are considered. > > > > Which other styles? From my test, this makes a difference to the font size. > > I was mislead by the effect of font-size: 100%; sorry :D No worries :) https://codereview.adblockplus.org/29488555/diff/29512700/static/scss/layout/... static/scss/layout/_footer.scss:67: &:last-child On 2017/09/06 17:48:20, juliandoucette wrote: > On 2017/09/04 20:28:08, ire wrote: > > On 2017/08/22 14:33:43, juliandoucette wrote: > > > last-child is not supported by IE > > > > :last-child is supported by IE 9+ (http://caniuse.com/#feat=css-sel3) > > I suggest that we support IE 8 by default or document that we don't support it > in an issue which we may later resolve or reject ( It's hard to have a strong > opinion in favour of supporting IE 8 :D ) ( because ABP currently supports IE 8 > and so does our browserlistrc; currently ). The current browserlist for this site supports IE9+, which is why I thought we weren't supporting IE 8. It seems like browser support is still somewhat uncertain :/ https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:1: <footer id="site-footer" class="navbar"> On 2017/09/06 17:48:20, juliandoucette wrote: > Note: (Added after the comments below) It looks like we should considering using > a menubar / menu / menu button pattern as described here > https://www.w3.org/TR/wai-aria-practices/#menu, > https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.... Good idea! Done. There are improvements I can make to this component to satisfy the keyboard support for a menu. I would like to make these changes in a separate issue though. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> On 2017/09/06 17:48:21, juliandoucette wrote: > Is there a reason that you didn't add aria-haspopup and/or aria-expanded? See comment above. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:8: <ul id="language-options" class="custom-select-options"> On 2017/09/06 17:48:21, juliandoucette wrote: > Is there a reason that you didn't add aria-labelledby? See comment above. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:9: {% for lang in available_locales %} On 2017/09/06 17:48:20, juliandoucette wrote: > NIT: These are locale codes not languages or language codes? ( <- that might be > a stretch ) Not sure what you mean. Am I using the wrong variable here? https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:12: {{config.get("langnames", lang)}} On 2017/09/06 17:48:21, juliandoucette wrote: > NIT: You are spacing {{ }} inconsistently in this file. I suggest always adding > one space after opening and before closing. Done. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... static/scss/base/_utilities.scss:101: text-decoration: underline; On 2017/09/06 17:48:21, juliandoucette wrote: > It seems strange for a color named class to apply an underline? It is specific to link elements. We want all muted link elements to have the underline. What else would you suggest? https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected On 2017/09/06 17:48:21, juliandoucette wrote: > NIT: Do help-center buttons share some of these styles? (Ack that you can > separate them later if [they do, you want]). That is probably the case. If I find they do I will separate the common styles. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/content... File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/content... static/scss/content/_typography.scss:21: color: inherit; On 2017/09/06 17:48:21, juliandoucette wrote: > I think that you are trying to override the default link color because links > outside of content areas are not accent colored. If that is the case then > doesn't it make more sense to namespace website-default _content.scss styles and > apply this style inside website-default _reset.scss? Yes that is what I was trying to do, and your suggestion makes sense. I will change this when website-default updates have been made. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:32: #site-footer .three-fourths On 2017/09/06 17:48:22, juliandoucette wrote: > NIT: This seems too general. We could conceivably add another row with > three-fourths before one-fourth. For that reason, I would be more specific e.g. > #site-footer-nav or .align-right. Done. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:56: #site-footer-nav ul On 2017/09/06 17:48:22, juliandoucette wrote: > NIT: Shouldn't we do this with a horizontal list utility class? Done. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:72: border-right: 1px solid $gray; On 2017/09/06 17:48:22, juliandoucette wrote: > NIT: Why not border-right the first-child or border-left the last-child instead > of border-righting everyone and unborder-righting the last-child? > > The same logic may apply to spacing. I did it this way because if we added another list item, we would want there to be a border-right and margin on all items besides the last child. What you are suggesting will work well now, but not if we added another item. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:19: padding: 0; On 2017/09/06 17:48:22, juliandoucette wrote: > Why eliminate the gutter width? > > Note: You didn't overwrite the negative margin on .row :S In the case of the footer, the gutter isn't needed and causes the content to not be aligned with the header. But I should have just restricted this to *only* in the footer, which I've done now. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:22: @media(min-width: $phablet-breakpoint) On 2017/09/06 17:48:22, juliandoucette wrote: > Note: It seems like we could make sizes conditional (in addition to > configurable) in website-defaults. I think this is a good idea. How would that work? In this case, the sizes themselves aren't being changed, it's the breakpoints.
https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:1: <footer id="site-footer" class="navbar"> On 2017/09/08 09:53:33, ire wrote: > On 2017/09/06 17:48:20, juliandoucette wrote: > > Note: (Added after the comments below) It looks like we should considering > using > > a menubar / menu / menu button pattern as described here > > https://www.w3.org/TR/wai-aria-practices/#menu, > > > https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.... > > Good idea! Done. > > There are improvements I can make to this component to satisfy the keyboard > support for a menu. I would like to make these changes in a separate issue > though. Acknowledged. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:9: {% for lang in available_locales %} On 2017/09/08 09:53:34, ire wrote: > Not sure what you mean. Am I using the wrong variable here? I was suggesting that you use a variable like "available_locale" instead of "lang" because "locale" is more accurate than "language" in this case. (I was being too picky.) https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... static/scss/base/_utilities.scss:101: text-decoration: underline; On 2017/09/08 09:53:34, ire wrote: > It is specific to link elements. We want all muted link elements to have the > underline. What else would you suggest? I think what you did here makes sense. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/compone... static/scss/components/_select.scss:24: .custom-select-selected On 2017/09/08 09:53:35, ire wrote: > On 2017/09/06 17:48:21, juliandoucette wrote: > > NIT: Do help-center buttons share some of these styles? (Ack that you can > > separate them later if [they do, you want]). > > That is probably the case. If I find they do I will separate the common styles. Acknowledged. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_footer.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_footer.scss:72: border-right: 1px solid $gray; On 2017/09/08 09:53:36, ire wrote: > I did it this way because if we added another list item, we would want there to > be a border-right and margin on all items besides the last child. What you are > suggesting will work well now, but not if we added another item. Good point. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:19: padding: 0; On 2017/09/08 09:53:36, ire wrote: > In the case of the footer, the gutter isn't needed and causes the content to not > be aligned with the header. But I should have just restricted this to *only* in > the footer, which I've done now. I agree. But this does beg the question of why you chose to use the grid in the footer (which I don't yet have an opinion about). https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:22: @media(min-width: $phablet-breakpoint) On 2017/09/08 09:53:36, ire wrote: > On 2017/09/06 17:48:22, juliandoucette wrote: > > Note: It seems like we could make sizes conditional (in addition to > > configurable) in website-defaults. I think this is a good idea. > > How would that work? In this case, the sizes themselves aren't being changed, > it's the breakpoints. Good question. Conditionals aren't quite the way to go it seems. I suggest something like this: https://codereview.adblockplus.org/29539827 https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:10: <li role="none"> Shouldn't this role be "presentation"? :/ https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:19: <div id="site-footer-nav" class="column three-fourths"> The nav and small are not vertically aligned middle with the language selector. https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:19: <div id="site-footer-nav" class="column three-fourths"> This section is aligned left between phablet and tablet sizes when JavaScript is disabled because of the absolute positioning of the custom select. This looks like a grid bug :/ https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:26: <small class="muted"> "eyeo GmbH" breaks oddly between phablet and tablet sizes. https://codereview.adblockplus.org/29488555/diff/29539636/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29539636/static/scss/compone... static/scss/components/_select.scss:42: .custom-select-options This covers content when JavaScript is disabled. Perhaps it should be statically positioned by default and then absolutely position when JavaScript is enabled?
Thanks! Updates uploaded. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/foo... includes/layout/footer.tmpl:9: {% for lang in available_locales %} On 2017/09/08 17:02:52, juliandoucette wrote: > On 2017/09/08 09:53:34, ire wrote: > > Not sure what you mean. Am I using the wrong variable here? > > I was suggesting that you use a variable like "available_locale" instead of > "lang" because "locale" is more accurate than "language" in this case. (I was > being too picky.) Ah I see. "available_locale" makes more sense, not too picky :) Done. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/base/_u... static/scss/base/_utilities.scss:101: text-decoration: underline; On 2017/09/08 17:02:52, juliandoucette wrote: > On 2017/09/08 09:53:34, ire wrote: > > It is specific to link elements. We want all muted link elements to have the > > underline. What else would you suggest? > > I think what you did here makes sense. Acknowledged. https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:19: padding: 0; On 2017/09/08 17:02:53, juliandoucette wrote: > On 2017/09/08 09:53:36, ire wrote: > > In the case of the footer, the gutter isn't needed and causes the content to > not > > be aligned with the header. But I should have just restricted this to *only* > in > > the footer, which I've done now. > > I agree. But this does beg the question of why you chose to use the grid in the > footer (which I don't yet have an opinion about). Most of the other styles are still valid. I didn't think it necessary to rewrite most of the grid styles only to override the one style https://codereview.adblockplus.org/29488555/diff/29535608/static/scss/layout/... static/scss/layout/_grid.scss:22: @media(min-width: $phablet-breakpoint) On 2017/09/08 17:02:53, juliandoucette wrote: > On 2017/09/08 09:53:36, ire wrote: > > On 2017/09/06 17:48:22, juliandoucette wrote: > > > Note: It seems like we could make sizes conditional (in addition to > > > configurable) in website-defaults. I think this is a good idea. > > > > How would that work? In this case, the sizes themselves aren't being changed, > > it's the breakpoints. > > Good question. Conditionals aren't quite the way to go it seems. I suggest > something like this: > > https://codereview.adblockplus.org/29539827 Thanks! I would like to update website-defaults in a separate issue/noissue since it will change more than just this (adds the reset, changes the $mobile variable name to $phone, etc). So if you don't mind, I will update this part after this has been committed. https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:10: <li role="none"> On 2017/09/08 17:02:53, juliandoucette wrote: > Shouldn't this role be "presentation"? :/ According to the example, it should be "none" in order to remove the implied "listitem" role. See this table https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1... https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:19: <div id="site-footer-nav" class="column three-fourths"> On 2017/09/08 17:02:54, juliandoucette wrote: > The nav and small are not vertically aligned middle with the language selector. I wasn't intended for it to. The content in both are aligned top. When the screen is resized, the content here flows onto more than one line, and I wanted the language selector to remain aligned top. https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:19: <div id="site-footer-nav" class="column three-fourths"> On 2017/09/08 17:02:53, juliandoucette wrote: > This section is aligned left between phablet and tablet sizes when JavaScript is > disabled because of the absolute positioning of the custom select. This looks > like a grid bug :/ I wasn't able to replicate this, but the .custom-select-options shouldn't be absolutely positioned with no javascript, so I've changed that and hopefully the issue is fixed https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:26: <small class="muted"> On 2017/09/08 17:02:54, juliandoucette wrote: > "eyeo GmbH" breaks oddly between phablet and tablet sizes. Done. https://codereview.adblockplus.org/29488555/diff/29539636/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29539636/static/scss/compone... static/scss/components/_select.scss:42: .custom-select-options On 2017/09/08 17:02:54, juliandoucette wrote: > This covers content when JavaScript is disabled. Perhaps it should be statically > positioned by default and then absolutely position when JavaScript is enabled? Done.
FWIW it looks good if I add the no-js class :D https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/foo... includes/layout/footer.tmpl:10: <li role="none"> On 2017/09/12 08:57:55, ire wrote: > On 2017/09/08 17:02:53, juliandoucette wrote: > > Shouldn't this role be "presentation"? :/ > > According to the example, it should be "none" in order to remove the implied > "listitem" role. > > See this table > https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1... Acknowledged. I looked into this further and forgot to rescind this comment. https://codereview.adblockplus.org/29488555/diff/29542559/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29542559/includes/layout/foo... includes/layout/footer.tmpl:11: {{ page | linkify(available_locale) }} NIT: This line break and indentation causes there to be a space before the comma in this list when JavaScript is disabled https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/compone... static/scss/components/_select.scss:74: .no-js .custom-select-options I think that you removed `.no-js` when you changed this to be no-js first.
> FWIW it looks good if I add the no-js class :D That's good :P https://codereview.adblockplus.org/29488555/diff/29542559/includes/layout/foo... File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29542559/includes/layout/foo... includes/layout/footer.tmpl:11: {{ page | linkify(available_locale) }} On 2017/09/12 11:41:12, juliandoucette wrote: > NIT: This line break and indentation causes there to be a space before the comma > in this list when JavaScript is disabled Done. https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/compone... static/scss/components/_select.scss:74: .no-js .custom-select-options On 2017/09/12 11:41:13, juliandoucette wrote: > I think that you removed `.no-js` when you changed this to be no-js first. I didn't remove it here. I thought this way was more efficient since the no-js styles are more (and not default/initial states), and if I made them the default I would have to write more CSS to override the styles.
https://codereview.adblockplus.org/29488555/diff/29542722/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29542722/templates/default.t... templates/default.tmpl:22: class="no-js"> This was missing for some reason :/
LGTM + NITs https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/compone... File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/compone... static/scss/components/_select.scss:74: .no-js .custom-select-options On 2017/09/12 15:04:02, ire wrote: > On 2017/09/12 11:41:13, juliandoucette wrote: > > I think that you removed `.no-js` when you changed this to be no-js first. > > I didn't remove it here. I thought this way was more efficient since the no-js > styles are more (and not default/initial states), and if I made them the default > I would have to write more CSS to override the styles. Acknowledged. https://codereview.adblockplus.org/29488555/diff/29542722/settings.ini File settings.ini (right): https://codereview.adblockplus.org/29488555/diff/29542722/settings.ini#newcode5 settings.ini:5: [langnames] NIT: Can you push this in a separate commit please (it can be Noissue) https://codereview.adblockplus.org/29488555/diff/29542722/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29542722/static/scss/layout/... static/scss/layout/_grid.scss:17: @media(min-width: $phablet-breakpoint) NIT: This can be partially accomplished by setting $half-breakpoint to $phablet-breakpoint. After that, I would suggest breaking one-fourth and three-fourths in the footer specifically because it's unclear whether that will be the desired functionality everywhere. And I suggest adding thirds when/if you use thirds. https://codereview.adblockplus.org/29488555/diff/29542722/static/scss/layout/... static/scss/layout/_grid.scss:29: @media(min-width: $tablet-breakpoint) NIT: This can be accomplished by setting $third-breakpoint and $fourth-breakpoint to $tablet-breakpoint.
Thanks Julian! Fixed the NITs and will push soon. https://codereview.adblockplus.org/29488555/diff/29542722/settings.ini File settings.ini (right): https://codereview.adblockplus.org/29488555/diff/29542722/settings.ini#newcode5 settings.ini:5: [langnames] On 2017/09/18 12:43:10, juliandoucette wrote: > NIT: Can you push this in a separate commit please (it can be Noissue) Done. https://codereview.adblockplus.org/29488555/diff/29542722/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29542722/static/scss/layout/... static/scss/layout/_grid.scss:17: @media(min-width: $phablet-breakpoint) On 2017/09/18 12:43:13, juliandoucette wrote: > NIT: This can be partially accomplished by setting $half-breakpoint to > $phablet-breakpoint. After that, I would suggest breaking one-fourth and > three-fourths in the footer specifically because it's unclear whether that will > be the desired functionality everywhere. And I suggest adding thirds when/if you > use thirds. Done. https://codereview.adblockplus.org/29488555/diff/29542722/static/scss/layout/... static/scss/layout/_grid.scss:29: @media(min-width: $tablet-breakpoint) On 2017/09/18 12:43:12, juliandoucette wrote: > NIT: This can be accomplished by setting $third-breakpoint and > $fourth-breakpoint to $tablet-breakpoint. Done. https://codereview.adblockplus.org/29488555/diff/29548675/static/scss/layout/... File static/scss/layout/_grid.scss (right): https://codereview.adblockplus.org/29488555/diff/29548675/static/scss/layout/... static/scss/layout/_grid.scss:17: $half-breakpoint: $phablet-breakpoint; I had to put these here because I couldn't use website-default variables ($phablet-breakpoint, $tablet-breakpoint) in this project's own variables file 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
