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

Issue 29516622: Issue 5511 - Create Product Help Home Template for help.eyeo.com (Closed)

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

Description

Issue 5511 - Create Product Help Home Template for help.eyeo.com

Patch Set 1 #

Total comments: 38

Patch Set 2 : Addressed first comments #

Patch Set 3 : Rebase #

Total comments: 39

Patch Set 4 : Rebase, Formatting template #

Total comments: 13

Patch Set 5 : Addressed final NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+433 lines, -2 lines) Patch
A globals/get_page_name.py View 1 2 3 4 1 chunk +19 lines, -0 lines 0 comments Download
A globals/products.py View 1 2 3 4 1 chunk +49 lines, -0 lines 0 comments Download
A pages/adblockplus/adblock-plus-breaks-the-websites-i-visit.md View 1 chunk +4 lines, -0 lines 0 comments Download
A pages/adblockplus/blockads.md View 1 chunk +5 lines, -0 lines 0 comments Download
A pages/adblockplus/download-and-install-adblock-plus.md View 1 chunk +5 lines, -0 lines 0 comments Download
A pages/adblockplus/index.md View 1 chunk +3 lines, -0 lines 0 comments Download
A pages/adblockplus/is-adblock-plus-the-same-thing-as-adblock.md View 1 chunk +5 lines, -0 lines 0 comments Download
A pages/adblockplus/removewhitelist.md View 1 chunk +5 lines, -0 lines 0 comments Download
A pages/adblockplus/what-are-acceptable-ads.md View 1 chunk +5 lines, -0 lines 0 comments Download
A static/img/png/help-bg.png View Binary file 0 comments Download
A static/img/png/install-icon.png View Binary file 0 comments Download
A static/img/png/logo-abp.png View Binary file 0 comments Download
A static/img/png/mobile-icon.png View Binary file 0 comments Download
A static/img/png/popular-icon.png View Binary file 0 comments Download
A static/img/png/reporting-icon.png View Binary file 0 comments Download
A static/img/png/settings-icon.png View Binary file 0 comments Download
A static/img/svg/install-icon.svg View 1 chunk +15 lines, -0 lines 0 comments Download
A static/img/svg/logo-abp.svg View 1 chunk +15 lines, -0 lines 0 comments Download
A static/img/svg/mobile-icon.svg View 1 chunk +12 lines, -0 lines 0 comments Download
A static/img/svg/popular-icon.svg View 1 chunk +12 lines, -0 lines 0 comments Download
A static/img/svg/reporting-icon.svg View 1 chunk +15 lines, -0 lines 0 comments Download
A static/img/svg/settings-icon.svg View 1 chunk +15 lines, -0 lines 0 comments Download
M static/scss/base/_utilities.scss View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M static/scss/base/_variables.scss View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
A static/scss/components/_card.scss View 1 2 3 1 chunk +38 lines, -0 lines 0 comments Download
M static/scss/components/_lists.scss View 1 2 3 4 1 chunk +38 lines, -0 lines 0 comments Download
A static/scss/content/_typography.scss View 1 2 3 1 chunk +57 lines, -0 lines 0 comments Download
A static/scss/layout/_body.scss View 1 2 3 1 chunk +42 lines, -0 lines 0 comments Download
M static/scss/main.scss View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M templates/default.tmpl View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A templates/product-home.tmpl View 1 2 3 1 chunk +60 lines, -0 lines 0 comments Download

Messages

Total messages: 12
ire
2 years, 3 months ago (2017-08-15 15:02:23 UTC) #1
ire
First go. I left out the Contact Section that would go at the bottom of ...
2 years, 3 months ago (2017-08-15 15:09:22 UTC) #2
juliandoucette
Here are my first impressions. https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_slug.py File globals/get_page_slug.py (right): https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_slug.py#newcode17 globals/get_page_slug.py:17: def get_page_slug(url): On 2017/08/15 ...
2 years, 3 months ago (2017-08-24 11:14:01 UTC) #3
juliandoucette
(I forgot to mention that this Patchset does not apply in it's current form.)
2 years, 3 months ago (2017-08-24 11:15:09 UTC) #4
ire
> (I forgot to mention that this Patchset does not apply in it's current > ...
2 years, 3 months ago (2017-08-28 14:54:50 UTC) #5
ire
Rebased
2 years, 2 months ago (2017-09-11 14:47:29 UTC) #6
juliandoucette
Looking good. Mostly NITs left. Thanks Ire! https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py File globals/products.py (right): https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py#newcode21 globals/products.py:21: "help_categories": [ ...
2 years, 2 months ago (2017-09-12 13:22:00 UTC) #7
juliandoucette
https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/content/_typography.scss File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/content/_typography.scss#newcode19 static/scss/content/_typography.scss:19: font-size: $font-size-h3; On 2017/09/12 13:21:57, juliandoucette wrote: > Note: ...
2 years, 2 months ago (2017-09-12 22:53:56 UTC) #8
ire
> Looking good. Mostly NITs left. Thanks Ire! Yay :) https://codereview.adblockplus.org/29516622/diff/29541770/globals/get_page_slug.py File globals/get_page_slug.py (right): https://codereview.adblockplus.org/29516622/diff/29541770/globals/get_page_slug.py#newcode18 ...
2 years, 2 months ago (2017-09-19 10:27:24 UTC) #9
juliandoucette
Responses before review. https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py File globals/products.py (right): https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py#newcode21 globals/products.py:21: "help_categories": [ On 2017/09/19 10:27:18, ire ...
2 years, 2 months ago (2017-09-22 13:29:37 UTC) #10
juliandoucette
LGTM + NITs. I think that we can address the non-NITs below in separate issues. ...
2 years, 2 months ago (2017-09-22 13:49:15 UTC) #11
ire
2 years, 2 months ago (2017-09-25 07:25:30 UTC) #12
> I think that we can address the non-NITs below in separate issues.

Thanks! Done.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:39: .underlined-list .one-half:nth-child(1) a
On 2017/09/22 13:49:13, juliandoucette wrote:
> On 2017/08/28 14:54:48, ire wrote:
> > All of the list items typically have a border-bottom. With <li> items like
the
> > popular topics one that are displayed in two columns, the first item has a
> > border-top as well. The media queries are because, the columns collapse into
> one
> > on smaller screens, so only the first list item needs the border-top until a
> > certain viewport width
> 
> Can you add a comment about this?

Done.

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
File globals/get_page_name.py (right):

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
globals/get_page_name.py:17: def get_page_name(url):
On 2017/09/22 13:49:13, juliandoucette wrote:
> NIT: I don't think we newline under definition declarations

Done.

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
globals/get_page_name.py:20: return split_url[ len(split_url) - 1 ]
On 2017/09/22 13:49:13, juliandoucette wrote:
> NIT: I don't think we space inside brackets

Done.

https://codereview.adblockplus.org/29516622/diff/29549725/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29549725/globals/products.py...
globals/products.py:18: "abp": {
On 2017/09/22 13:49:14, juliandoucette wrote:
> NIT: I think we prefer single quotes in python

Done.

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

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/content...
static/scss/content/_typography.scss:56: @extend .ta-center;
On 2017/09/22 13:49:14, juliandoucette wrote:
> On 2017/09/19 10:27:23, ire wrote:
> > Do we have anything against using @extend rules?
> 
> Yes; unfortunately.
> 
> But I think we aught not to. Lets keep em for now.

Acknowledged.

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

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
static/scss/layout/_body.scss:19: background-image: url(/img/png/help-bg.png);
On 2017/09/22 13:49:14, juliandoucette wrote:
> This should be smaller/texturized/repeated and transparent.

I don't see the difference between the implementation and the design here.

- Why smaller? The image is the same size as in the mockup/spec
- The background image isn't supposed to be repeated. It's supposed to fade out
as it does here. 
- It is already transparent?

(Since I'm about to close this issue, we can continue this discuss in IRC or you
can create an issue with more info)

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
static/scss/layout/_body.scss:25: padding-top: $lg;
On 2017/09/22 13:49:15, juliandoucette wrote:
> Can we get rid of these custom spaces entirely in a separate issue? We already
> have space variables in website-defaults that we can set.

https://issues.adblockplus.org/ticket/5765
Sign in to reply to this message.

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