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

Issue 29588588: Issue 5848 - Fix vertical alignment on .heading-icon images in help.eyeo.com (Closed)

Created:
Oct. 25, 2017, 10:21 a.m. by ire
Modified:
Nov. 1, 2017, 7:57 a.m.
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 10

Patch Set 2 : Rebased #

Patch Set 3 : Rewrite .has-pre-icon #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+163 lines, -101 lines) Patch
A globals/get_inline_bg.py View 1 2 1 chunk +20 lines, -0 lines 0 comments Download
M includes/contact.html View 1 2 1 chunk +0 lines, -62 lines 0 comments Download
A includes/contact.tmpl View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
M static/scss/components/_breadcrumb.scss View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
A static/scss/components/_pre-icon.scss View 1 2 1 chunk +49 lines, -0 lines 0 comments Download
M static/scss/content/_typography.scss View 1 2 1 chunk +6 lines, -12 lines 0 comments Download
M static/scss/main.scss View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M templates/article.tmpl View 1 2 1 chunk +15 lines, -14 lines 3 comments Download
M templates/product-home.tmpl View 1 2 2 chunks +10 lines, -11 lines 0 comments Download

Messages

Total messages: 7
ire
Oct. 25, 2017, 10:21 a.m. (2017-10-25 10:21:53 UTC) #1
ire
Ready for review. It appears that the spacing with text-decoration is also fixed by this ...
Oct. 25, 2017, 10:23 a.m. (2017-10-25 10:23:01 UTC) #2
juliandoucette
https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/components/_aside-icon.scss File static/scss/components/_aside-icon.scss (right): https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/components/_aside-icon.scss#newcode17 static/scss/components/_aside-icon.scss:17: // Aside Icon ////////////////////////////////////////////////////////////////// On 2017/10/25 10:23:01, ire wrote: ...
Oct. 26, 2017, 11:42 a.m. (2017-10-26 11:42:58 UTC) #3
ire
I rewrote this to use background-images instead. https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/components/_aside-icon.scss File static/scss/components/_aside-icon.scss (right): https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/components/_aside-icon.scss#newcode17 static/scss/components/_aside-icon.scss:17: // Aside ...
Oct. 30, 2017, 9:45 a.m. (2017-10-30 09:45:38 UTC) #4
ire
https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29588588/diff/29592559/templates/article.tmpl#newcode29 templates/article.tmpl:29: {{ title | translate( get_page_name(page) + "-title", "Article title") ...
Oct. 30, 2017, 9:46 a.m. (2017-10-30 09:46:41 UTC) #5
juliandoucette
LGTM + NIT below. https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/content/_typography.scss File static/scss/content/_typography.scss (right): https://codereview.adblockplus.org/29588588/diff/29588589/static/scss/content/_typography.scss#newcode65 static/scss/content/_typography.scss:65: width: 100%; On 2017/10/30 09:45:37, ...
Oct. 31, 2017, 2:14 p.m. (2017-10-31 14:14:35 UTC) #6
ire
Nov. 1, 2017, 7:57 a.m. (2017-11-01 07:57:25 UTC) #7 
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

Powered by Google App Engine
This is Rietveld