Issue 5848 - Fix vertical alignment on .heading-icon images in help.eyeo.com

Issue 5848 - Fix vertical alignment on .heading-icon images in help.eyeo.com

[ire, 2017-10-25 10:23:01]

Ready for review.

It appears that the spacing with text-decoration is also fixed by this

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
File static/scss/components/_aside-icon.scss (right):

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
static/scss/components/_aside-icon.scss:17: // Aside Icon
//////////////////////////////////////////////////////////////////
I changed the name to .aside-icon from .heading-icon since it's not always
applied to a heading

[juliandoucette, 2017-10-26 11:42:58]

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
File static/scss/components/_aside-icon.scss (right):

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
static/scss/components/_aside-icon.scss:17: // Aside Icon
//////////////////////////////////////////////////////////////////
On 2017/10/25 10:23:01, ire wrote:
> I changed the name to .aside-icon from .heading-icon since it's not always
> applied to a heading

suggest: `has-icon`, `has-pre-icon`, `pre-icon`, `iconify`

- "pre" is more specific than "aside"
- "has" is roughly "contains" / ~"container"
- "aside-icon-container" is quite long

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
static/scss/components/_aside-icon.scss:24: .aside-icon-container span
The icon still seems offcenter (too far up).

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

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/content...
static/scss/content/_typography.scss:65: width: 100%;
Why do we need this? (Headings are usually block.)

[ire, 2017-10-30 09:45:38]

I rewrote this to use background-images instead.

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
File static/scss/components/_aside-icon.scss (right):

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
static/scss/components/_aside-icon.scss:17: // Aside Icon
//////////////////////////////////////////////////////////////////
On 2017/10/26 11:42:57, juliandoucette wrote:
> On 2017/10/25 10:23:01, ire wrote:
> > I changed the name to .aside-icon from .heading-icon since it's not always
> > applied to a heading
> 
> suggest: `has-icon`, `has-pre-icon`, `pre-icon`, `iconify`
> 
> - "pre" is more specific than "aside"
> - "has" is roughly "contains" / ~"container"
> - "aside-icon-container" is quite long

I like `has-pre-icon`

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
static/scss/components/_aside-icon.scss:17: // Aside Icon
//////////////////////////////////////////////////////////////////
On 2017/10/26 11:42:57, juliandoucette wrote:
> On 2017/10/25 10:23:01, ire wrote:
> > I changed the name to .aside-icon from .heading-icon since it's not always
> > applied to a heading
> 
> suggest: `has-icon`, `has-pre-icon`, `pre-icon`, `iconify`
> 
> - "pre" is more specific than "aside"
> - "has" is roughly "contains" / ~"container"
> - "aside-icon-container" is quite long

Done.

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/compone...
static/scss/components/_aside-icon.scss:24: .aside-icon-container span
On 2017/10/26 11:42:57, juliandoucette wrote:
> The icon still seems offcenter (too far up).

You're right. I rewrote this to use background-images instead. That was the
cleanest way I could find to position it exactly how we would want.

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

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/content...
static/scss/content/_typography.scss:65: width: 100%;
On 2017/10/26 11:42:57, juliandoucette wrote:
> Why do we need this? (Headings are usually block.)

Because of the icon-container, it's display:table

[ire, 2017-10-30 09:46:41]

https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.t...
templates/article.tmpl:29: {{ title | translate( get_page_name(page) + "-title",
"Article title") }}
This looks odd when the article title flows on to multiple lines. I will handle
this in a separate issue. I would like to talk to Jeen about how we should
handle it too.

[juliandoucette, 2017-10-31 14:14:35]

LGTM + NIT below.

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

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/content...
static/scss/content/_typography.scss:65: width: 100%;
On 2017/10/30 09:45:37, ire wrote:
> On 2017/10/26 11:42:57, juliandoucette wrote:
> > Why do we need this? (Headings are usually block.)
> 
> Because of the icon-container, it's display:table

Ack. But it's not anymore?

https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.t...
templates/article.tmpl:29: {{ title | translate( get_page_name(page) + "-title",
"Article title") }}
On 2017/10/30 09:46:41, ire wrote:
> This looks odd when the article title flows on to multiple lines. I will
handle
> this in a separate issue. I would like to talk to Jeen about how we should
> handle it too.

Acknowledged.

[ire, 2017-11-01 07:57:25]

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

https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/content...
static/scss/content/_typography.scss:65: width: 100%;
On 2017/10/31 14:14:34, juliandoucette wrote:
> On 2017/10/30 09:45:37, ire wrote:
> > On 2017/10/26 11:42:57, juliandoucette wrote:
> > > Why do we need this? (Headings are usually block.)
> > 
> > Because of the icon-container, it's display:table
> 
> Ack. But it's not anymore?

Oops missed that. Done.

https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.t...
templates/article.tmpl:29: {{ title | translate( get_page_name(page) + "-title",
"Article title") }}
On 2017/10/31 14:14:35, juliandoucette wrote:
> On 2017/10/30 09:46:41, ire wrote:
> > This looks odd when the article title flows on to multiple lines. I will
> handle
> > this in a separate issue. I would like to talk to Jeen about how we should
> > handle it too.
> 
> Acknowledged.

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