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

Issue 29519587: Issue 5534 - Create Contact Section for help.eyeo.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by ire
Modified:
1 year, 9 months ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Issue 5534 - Create Contact Section for help.eyeo.com

Patch Set 1 #

Total comments: 21

Patch Set 2 : Update alt text, rebase, refactor links in section-accent #

Total comments: 3

Patch Set 3 : Add footer block for contact section #

Total comments: 32

Patch Set 4 : #contact element, refactoring #

Total comments: 7

Patch Set 5 : Addressed final NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -57 lines) Patch
A includes/contact.html View 1 2 3 4 1 chunk +62 lines, -0 lines 0 comments Download
A static/img/png/accent-bg.png View Binary file 0 comments Download
A static/img/png/email-icon.png View Binary file 0 comments Download
A static/img/png/logo-facebook.png View Binary file 0 comments Download
A static/img/png/logo-twitter.png View Binary file 0 comments Download
A static/img/svg/email-icon.svg View 1 chunk +22 lines, -0 lines 0 comments Download
A static/img/svg/logo-facebook.svg View 1 chunk +12 lines, -0 lines 0 comments Download
A static/img/svg/logo-twitter.svg View 1 chunk +22 lines, -0 lines 0 comments Download
M static/scss/base/_utilities.scss View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
A static/scss/components/_contact.scss View 1 2 3 4 1 chunk +52 lines, -0 lines 0 comments Download
M static/scss/content/_typography.scss View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M static/scss/layout/_body.scss View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
M static/scss/main.scss View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M templates/default.tmpl View 1 2 1 chunk +7 lines, -57 lines 0 comments Download
A templates/minimal.tmpl View 1 2 1 chunk +62 lines, -0 lines 0 comments Download

Messages

Total messages: 10
ire
1 year, 11 months ago (2017-08-18 12:30:49 UTC) #1
ire
Ready for review. This is against the Product Help Template review (which itself is against ...
1 year, 11 months ago (2017-08-18 12:34:30 UTC) #2
juliandoucette
Thanks Ire :) Here are my first impressions. https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html File includes/contact.html (right): https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html#newcode5 includes/contact.html:5: {{contact[heading] ...
1 year, 10 months ago (2017-08-30 11:33:06 UTC) #3
ire
Updates ready for review :) https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html File includes/contact.html (right): https://codereview.adblockplus.org/29519587/diff/29519588/includes/contact.html#newcode5 includes/contact.html:5: {{contact[heading] Still looking for ...
1 year, 9 months ago (2017-09-25 08:47:36 UTC) #4
juliandoucette
I haven't reviewed this yet. Your question caught my eye. https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl#newcode6 ...
1 year, 9 months ago (2017-09-25 12:15:22 UTC) #5
ire
https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29519587/diff/29555634/templates/default.tmpl#newcode6 templates/default.tmpl:6: <? include contact ?> On 2017/09/25 12:15:22, juliandoucette wrote: ...
1 year, 9 months ago (2017-09-25 14:02:25 UTC) #6
juliandoucette
https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.html File includes/contact.html (right): https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.html#newcode1 includes/contact.html:1: <section class="section-accent"> NIT: I think that this section technically ...
1 year, 9 months ago (2017-09-26 15:37:56 UTC) #7
ire
Thanks Julian! I've made some changes. https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.html File includes/contact.html (right): https://codereview.adblockplus.org/29519587/diff/29555748/includes/contact.html#newcode1 includes/contact.html:1: <section class="section-accent"> On ...
1 year, 9 months ago (2017-09-28 09:43:46 UTC) #8
juliandoucette
LGTM - I think we can address the issues below separately as you see fit. ...
1 year, 9 months ago (2017-10-09 22:00:40 UTC) #9
ire
1 year, 9 months ago (2017-10-10 18:36:30 UTC) #10
Thanks. I've taken note of the comments that will be addressed in a separate
issue later

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:44: .section-accent
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:44, ire wrote:
> > 3. By your own naming scheme, this section should be an accent, not a
> secondary
> > (the way I understand it anyway. e.g in http://aa.com, the green sections
are
> "accent")
> 
> I disagree. It would be secondary. And the product colours would be accent.
> Secondary didn't necessarily mean "opposite".
> 
> > 4. I think the confusion comes from the naming of "secondary". Perhaps we
can
> > change it to be called "primary-contrast" or something like that, and keep
> > "accent" as "accent" (i.e. remove secondary)
> 
> Perhaps [primary, secondary, brand]?

Isn't "brand" "primary"? Haha. I think we should create an issue to discuss this
further, and maybe get input from others as well.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:48: background-image:
url("/img/png/accent-bg.png");
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:45, ire wrote:
> > Perhaps, but what would be the benefit in this case? (Considering we would
> lose
> > the glow/shadow)
> 
> [smaller file size, less blur]
> 
> (This is a NIT)

Fair points. I think I will consider this something that can be looked into post
launch.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:45, ire wrote:
> > Nice trick! But the margins between the list items don't collapse, and the
> > spacing between them gets too big.
> 
> I was counting on the margins not collapsing.
> 
> Assuming $md = 2 * $sm (or similar)
> ul (margin: 0 -$sm)
> li (margin: 0 $sm)
> 
> This will equal out to $md between each.
> 
> (Note: A note is less than a NIT :D - You don't have to address this if you
> don't want to.)

Ah I see, I think I was using $md when I tried and the spacing was much too big.

https://codereview.adblockplus.org/29519587/diff/29555748/static/scss/layout/...
static/scss/layout/_body.scss:61: [dir="rtl"]
On 2017/10/09 22:00:38, juliandoucette wrote:
> On 2017/09/28 09:43:45, ire wrote:
> > Nice trick! But the margins between the list items don't collapse, and the
> > spacing between them gets too big.
> 
> I was counting on the margins not collapsing.
> 
> Assuming $md = 2 * $sm (or similar)
> ul (margin: 0 -$sm)
> li (margin: 0 $sm)
> 
> This will equal out to $md between each.
> 
> (Note: A note is less than a NIT :D - You don't have to address this if you
> don't want to.)

Done.

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.html
File includes/contact.html (right):

https://codereview.adblockplus.org/29519587/diff/29558588/includes/contact.ht...
includes/contact.html:29: alt="{{ftwitter-logo[Image alt text] Blue Twitter
hieroglyph}}">
On 2017/10/09 22:00:39, juliandoucette wrote:
> NIT/Suggest: *just* "glyph"
> 
> Source: http://wikidiff.com/hieroglyph/glyph

Done.

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
static/scss/content/_typography.scss:35: font-size: $font-size-h2;
On 2017/10/09 22:00:39, juliandoucette wrote:
> Note: We should probably make this configurable via website-defaults

I agree

https://codereview.adblockplus.org/29519587/diff/29558588/static/scss/content...
static/scss/content/_typography.scss:47: color: $font-color-link;
On 2017/10/09 22:00:39, juliandoucette wrote:
> Note: This is configurable via website-defaults
> https://hg.adblockplus.org/website-defaults/file/tip/scss/_content.scss#l93 -
> perhaps it should be *more* configurable e.g. $link-color

Yes it probably should, as $accent could apply to other things
Sign in to reply to this message.

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