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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by ire
Modified:
1 month 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: 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
3 months, 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 ...
3 months, 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: ...
3 months ago (2017-07-17 09:37:18 UTC) #3
juliandoucette
On what base does this latest patchset apply?
2 months, 3 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 ...
2 months, 3 weeks ago (2017-07-25 15:40:53 UTC) #5
juliandoucette
Can you rebase this onto the tip of help.eyeo.com?
2 months, 1 week ago (2017-08-09 15:49:43 UTC) #6
juliandoucette
(Or the header component)
2 months, 1 week 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? ...
2 months, 1 week ago (2017-08-11 16:08:33 UTC) #8
juliandoucette
Looks like many of these styles may update with the header component. I hope that ...
1 month, 4 weeks ago (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 ...
1 month, 3 weeks ago (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 ...
1 month, 3 weeks ago (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 ...
1 month, 2 weeks ago (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, ...
1 month, 2 weeks ago (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: ...
1 month, 1 week ago (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: > ...
1 month, 1 week ago (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 %} ...
1 month, 1 week ago (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): ...
1 month, 1 week ago (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 ...
1 month, 1 week ago (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 :/
1 month, 1 week ago (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 ...
1 month ago (2017-09-18 12:43:13 UTC) #20
ire
1 month ago (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
Sign in to reply to this message.

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