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

Issue 29556593: Issue 5690 - Create Article Template for help.eyeo.com (Closed)

Created:
Sept. 26, 2017, 9:12 a.m. by ire
Modified:
Oct. 9, 2017, 7:49 p.m.
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Issue 5690 - Create Article Template for help.eyeo.com

Patch Set 1 #

Total comments: 16

Patch Set 2 : Addressed NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -0 lines) Patch
A static/scss/components/_article.scss View 1 chunk +35 lines, -0 lines 0 comments Download
M static/scss/content/_typography.scss View 2 chunks +15 lines, -0 lines 0 comments Download
M static/scss/main.scss View 1 chunk +1 line, -0 lines 0 comments Download
A templates/article.tmpl View 1 1 chunk +30 lines, -0 lines 0 comments Download

Messages

Total messages: 4
ire
Sept. 26, 2017, 9:12 a.m. (2017-09-26 09:12:55 UTC) #1
ire
Ready for review. https://codereview.adblockplus.org/29556593/diff/29556594/pages/adblockplus/uninstall-adblock-plus.md File pages/adblockplus/uninstall-adblock-plus.md (right): https://codereview.adblockplus.org/29556593/diff/29556594/pages/adblockplus/uninstall-adblock-plus.md#newcode1 pages/adblockplus/uninstall-adblock-plus.md:1: title=Uninstall Adblock Plus I added this ...
Sept. 26, 2017, 9:16 a.m. (2017-09-26 09:16:43 UTC) #2
juliandoucette
LGTM + NITs (Please address the NITs or create separate issues) https://codereview.adblockplus.org/29556593/diff/29556594/static/scss/components/_article.scss File static/scss/components/_article.scss (right): ...
Oct. 9, 2017, 12:54 p.m. (2017-10-09 12:54:00 UTC) #3
ire
Oct. 9, 2017, 7:48 p.m. (2017-10-09 19:48:07 UTC) #4 
Thanks Julian. I've addressed some of the NITs here and created issues for the
others.

https://codereview.adblockplus.org/29556593/diff/29556594/static/scss/compone...
File static/scss/components/_article.scss (right):

https://codereview.adblockplus.org/29556593/diff/29556594/static/scss/compone...
static/scss/components/_article.scss:25: padding: $lg;
On 2017/10/09 12:53:59, juliandoucette wrote:
> NIT: p contains margin-bottom. As a result, the space at the bottom of most
> cards will be .article padding-bottom + p margin-bottom.  I'm not sure what,
if
> anything, we should do about this. [we could make this padding-bottom less, we
> could make p margin-bottom less, we could allow more space at the bottom] 
> 
> This seems like a question that I should already have a good answer to based
on
> experience :/ :D

Hmm, I'm unsure about if I think this needs to be addressed. My thoughts:

1. I don't think we should change the margin/padding on the p or this .article,
because we can't be certain the the last bit of content will even be a paragraph
(e.g. it could be an image, blockquote, etc).

2. I don't think the extra space makes the design look off

3. Because of 1 & 2, I would be fine doing a `.article p:last-of-type` selector
to remove the margin-bottom, and have this only apply to browsers that do
support that selector

Nonetheless, I will create an issue and we can discuss this further there.

https://codereview.adblockplus.org/29556593/diff/29556594/static/scss/compone...
static/scss/components/_article.scss:25: padding: $lg;
On 2017/10/09 12:53:59, juliandoucette wrote:
> NIT: p contains margin-bottom. As a result, the space at the bottom of most
> cards will be .article padding-bottom + p margin-bottom.  I'm not sure what,
if
> anything, we should do about this. [we could make this padding-bottom less, we
> could make p margin-bottom less, we could allow more space at the bottom] 
> 
> This seems like a question that I should already have a good answer to based
on
> experience :/ :D

https://issues.adblockplus.org/ticket/5849#ticket

https://codereview.adblockplus.org/29556593/diff/29556594/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29556593/diff/29556594/templates/article.t...
templates/article.tmpl:9: <div class="row">
On 2017/10/09 12:54:00, juliandoucette wrote:
> On 2017/09/26 09:16:43, ire wrote:
> > This row class is removing the margins added by the container.
> 
> This is expected.
> 
> > I don't completely understand why the negative margins exist on the rows
> actually.
> 
> The negative margin exists to negate the first and last column padding.
> 
> Components should handle horizontal padding and columns should handle
horizontal
> margin.
> 
> Imagine the following:
> 
> div.container (padding: 0 1em)
>     p (padding: 1em 0)
>     .row (margin: 0 -1em)
>         .column.one-half (padding: 0 1em)
>             p
>         .column.one-half
>             p
> 
> If .container and .column both have 1em padding then the .one-half p will have
> 2em padding at the beginning and end of the row (provided by the .container).
> .row negates the padding on the .container in-order-to align the .container p
> with the .column p(s).
> 
> It works the same way with components. Text within components will align like
> this unless they are supposed to have more padding - which can/should be
> provided by the specific component.
> 
> > class in website-defaults on the row that doesn't do this?
> 
> We could do that. But .row and .column are not married (.column is styled, not
> .row > .column). You could just omit the .row if you don't want the negative
> margin.

Ah okay. This makes sense (obviously :p). I will omit it and add a .clearfix
class to the #main instead, as that's all I really need for the .columns to
function properly

https://codereview.adblockplus.org/29556593/diff/29556594/templates/article.t...
templates/article.tmpl:10: <article class="article card {{ product_id }}-card
section column two-thirds">
On 2017/10/09 12:53:59, juliandoucette wrote:
> NIT/Thinking-out-loud: We could add a product specific class to the root
element
> and then modify children instead. e.g. .abp a {color: red} .abp .card
> {border-top: red}

This would work on some cases, but when we support other products, there will be
pages (e.g. the homepage) in which not all .cards will belong to one product. So
I don't think we could always guarantee that a page will *only* have content for
one product

https://codereview.adblockplus.org/29556593/diff/29556594/templates/article.t...
templates/article.tmpl:12: <img class="heading-icon" src="/img/png/logo-{{
product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{
product.full_name+" Logo" | translate( product_id+"-logo-alt", "Image alt text")
}}">
On 2017/10/09 12:53:59, juliandoucette wrote:
> NIT: This doesn't seem to be vertically centred

I will create a separate issue for this as it could apply to all .heading-icon
images

https://codereview.adblockplus.org/29556593/diff/29556594/templates/article.t...
templates/article.tmpl:12: <img class="heading-icon" src="/img/png/logo-{{
product_id }}.png" srcset="/img/svg/logo-{{ product_id }}.svg 2x" alt="{{
product.full_name+" Logo" | translate( product_id+"-logo-alt", "Image alt text")
}}">
On 2017/10/09 12:53:59, juliandoucette wrote:
> NIT: This doesn't seem to be vertically centred

https://issues.adblockplus.org/ticket/5848

https://codereview.adblockplus.org/29556593/diff/29556594/templates/article.t...
templates/article.tmpl:17: {% if hide_browser_selector is not defined %}
On 2017/10/09 12:53:59, juliandoucette wrote:
> Neat

Thanks :)

Powered by Google App Engine
This is Rietveld