Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

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: 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+349 lines, -1 line) Patch
A includes/layout/footer.tmpl View 1 2 3 4 5 6 1 chunk +29 lines, -0 lines 0 comments Download
M static/js/main.js View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
M static/scss/base/_utilities.scss View 1 2 3 1 chunk +14 lines, -0 lines 0 comments Download
A static/scss/components/_lists.scss View 1 2 3 4 1 chunk +34 lines, -0 lines 0 comments Download
A static/scss/components/_select.scss View 1 2 3 4 5 1 chunk +104 lines, -0 lines 0 comments Download
A static/scss/content/_typography.scss View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A static/scss/layout/_footer.scss View 1 2 3 4 5 6 7 8 1 chunk +93 lines, -0 lines 0 comments Download
A static/scss/layout/_grid.scss View 1 2 3 4 5 6 7 8 1 chunk +19 lines, -0 lines 1 comment Download
M static/scss/main.scss View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M templates/default.tmpl View 1 2 3 4 5 6 7 2 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 21
ire
July 13, 2017, 7:57 a.m. (2017-07-13 07:57:34 UTC) #1
ire
First patch :) There are some files here that are basically the same as the ...
July 13, 2017, 8:04 a.m. (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: ...
July 17, 2017, 9:37 a.m. (2017-07-17 09:37:18 UTC) #3
juliandoucette
On what base does this latest patchset apply?
July 25, 2017, 3:34 p.m. (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 ...
July 25, 2017, 3:40 p.m. (2017-07-25 15:40:53 UTC) #5
juliandoucette
Can you rebase this onto the tip of help.eyeo.com?
Aug. 9, 2017, 3:49 p.m. (2017-08-09 15:49:43 UTC) #6
juliandoucette
(Or the header component)
Aug. 9, 2017, 3:50 p.m. (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? ...
Aug. 11, 2017, 4:08 p.m. (2017-08-11 16:08:33 UTC) #8
juliandoucette
Looks like many of these styles may update with the header component. I hope that ...
Aug. 22, 2017, 2:33 p.m. (2017-08-22 14:33:44 UTC) #9
ire
On 2017/08/22 14:33:44, juliandoucette wrote: > Looks like many of these styles may update with ...
Aug. 25, 2017, 10:12 a.m. (2017-08-25 10:12:45 UTC) #10
juliandoucette
> Thanks Julian! I think you're right. If you don't mind I would like to ...
Aug. 30, 2017, 11:36 a.m. (2017-08-30 11:36:29 UTC) #11
ire
I've rebased this on top of the site header commit, and made changes based on ...
Sept. 4, 2017, 8:28 p.m. (2017-09-04 20:28:09 UTC) #12
juliandoucette
Thanks Ire! https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/footer.tmpl File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/footer.tmpl#newcode5 includes/layout/footer.tmpl:5: <button class="custom-select-selected" aria-expanded="false" aria-controls="language-options"> On 2017/09/04 20:28:06, ...
Sept. 6, 2017, 5:48 p.m. (2017-09-06 17:48:23 UTC) #13
ire
> Thanks Ire! You're welcome :) . Updates ready. https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/footer.tmpl File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29512700/includes/layout/footer.tmpl#newcode5 includes/layout/footer.tmpl:5: ...
Sept. 8, 2017, 9:53 a.m. (2017-09-08 09:53:37 UTC) #14
juliandoucette
https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/footer.tmpl File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/footer.tmpl#newcode1 includes/layout/footer.tmpl:1: <footer id="site-footer" class="navbar"> On 2017/09/08 09:53:33, ire wrote: > ...
Sept. 8, 2017, 5:02 p.m. (2017-09-08 17:02:55 UTC) #15
ire
Thanks! Updates uploaded. https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/footer.tmpl File includes/layout/footer.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29535608/includes/layout/footer.tmpl#newcode9 includes/layout/footer.tmpl:9: {% for lang in available_locales %} ...
Sept. 12, 2017, 8:57 a.m. (2017-09-12 08:57:56 UTC) #16
juliandoucette
FWIW it looks good if I add the no-js class :D https://codereview.adblockplus.org/29488555/diff/29539636/includes/layout/footer.tmpl File includes/layout/footer.tmpl (right): ...
Sept. 12, 2017, 11:41 a.m. (2017-09-12 11:41:13 UTC) #17
ire
> FWIW it looks good if I add the no-js class :D That's good :P ...
Sept. 12, 2017, 3:04 p.m. (2017-09-12 15:04:03 UTC) #18
ire
https://codereview.adblockplus.org/29488555/diff/29542722/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29488555/diff/29542722/templates/default.tmpl#newcode22 templates/default.tmpl:22: class="no-js"> This was missing for some reason :/
Sept. 12, 2017, 3:06 p.m. (2017-09-12 15:06:20 UTC) #19
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/components/_select.scss File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29488555/diff/29542559/static/scss/components/_select.scss#newcode74 static/scss/components/_select.scss:74: .no-js .custom-select-options On 2017/09/12 15:04:02, ire ...
Sept. 18, 2017, 12:43 p.m. (2017-09-18 12:43:13 UTC) #20
ire
Sept. 18, 2017, 3:13 p.m. (2017-09-18 15:13:13 UTC) #21
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

Powered by Google App Engine
This is Rietveld