Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(235)

Issue 29488555: Issue 5406 - Create Site Footer Component for Help Center

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by ire
Modified:
1 hour, 40 minutes ago
Reviewers:
juliandoucette
Visibility:
Public.

Description

Issue 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: 24
Unified diffs Side-by-side diffs Delta from patch set Stats (+445 lines, -9 lines) Patch
A includes/layout/footer.tmpl View 1 1 chunk +29 lines, -0 lines 4 comments Download
M settings.ini View 1 chunk +19 lines, -0 lines 0 comments Download
A static/js/main.js View 1 chunk +27 lines, -0 lines 2 comments Download
M static/scss/base/_reset.scss View 1 chunk +6 lines, -6 lines 0 comments Download
M static/scss/base/_variables.scss View 1 2 1 chunk +23 lines, -1 line 1 comment Download
A static/scss/components/_select.scss View 1 chunk +103 lines, -0 lines 7 comments Download
A static/scss/content/_typography.scss View 1 1 chunk +47 lines, -0 lines 2 comments Download
A static/scss/layout/_footer.scss View 1 chunk +90 lines, -0 lines 5 comments Download
A static/scss/layout/_grid.scss View 1 chunk +55 lines, -0 lines 1 comment Download
A static/scss/layout/_navbar.scss View 1 chunk +36 lines, -0 lines 1 comment Download
M static/scss/main.scss View 1 2 1 chunk +6 lines, -1 line 1 comment Download
M templates/default.tmpl View 1 2 2 chunks +4 lines, -1 line 0 comments Download

Messages

Total messages: 9
ire
1 month, 1 week ago (2017-07-13 07:57:34 UTC) #1
ire
First patch :) There are some files here that are basically the same as the ...
1 month, 1 week ago (2017-07-13 08:04:39 UTC) #2
ire
Minor updates based on review of Site Header Component https://codereview.adblockplus.org/29488555/diff/29490585/static/scss/content/_typography.scss File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29488555/diff/29490585/static/scss/content/_typography.scss#newcode37 static/scss/content/_typography.scss:37: ...
1 month ago (2017-07-17 09:37:18 UTC) #3
juliandoucette
On what base does this latest patchset apply?
4 weeks ago (2017-07-25 15:34:06 UTC) #4
ire
On 2017/07/25 15:34:06, juliandoucette wrote: > On what base does this latest patchset apply? The ...
4 weeks ago (2017-07-25 15:40:53 UTC) #5
juliandoucette
Can you rebase this onto the tip of help.eyeo.com?
1 week, 6 days ago (2017-08-09 15:49:43 UTC) #6
juliandoucette
(Or the header component)
1 week, 6 days ago (2017-08-09 15:50:23 UTC) #7
ire
On 2017/08/09 15:49:43, juliandoucette wrote: > Can you rebase this onto the tip of help.eyeo.com? ...
1 week, 4 days ago (2017-08-11 16:08:33 UTC) #8
juliandoucette
4 hours, 13 minutes ago (2017-08-22 14:33:44 UTC) #9
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5