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

Issue 29401619: Issue 4963 - Wrong size used for text logo on acceptableads.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by juliandoucette
Modified:
2 years, 4 months ago
Reviewers:
saroyanm
CC:
Thomas Greiner, ire
Base URL:
https://hg.adblockplus.org/web.acceptableads.com
Visibility:
Public.

Description

Issue 4963 - Wrong size used for text logo on acceptableads.com

Patch Set 1 #

Total comments: 9

Patch Set 2 : Addressed comments #

Total comments: 12

Patch Set 3 : Addressed comments #

Patch Set 4 : Changed non-space to word-spacing #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -54 lines) Patch
M includes/navbar.tmpl View 1 2 3 1 chunk +1 line, -4 lines 0 comments Download
M includes/sidebar.tmpl View 1 2 3 1 chunk +9 lines, -9 lines 0 comments Download
M static/css/main.css View 1 2 3 3 chunks +33 lines, -18 lines 1 comment Download
M static/scss/layout/_navbar.scss View 1 2 3 1 chunk +22 lines, -2 lines 0 comments Download
M static/scss/layout/_sidebar.scss View 1 5 chunks +17 lines, -21 lines 0 comments Download

Messages

Total messages: 14
juliandoucette
2 years, 7 months ago (2017-04-03 21:39:22 UTC) #1
juliandoucette
On 2017/04/03 21:39:22, juliandoucette wrote: Small change, but it's worth considering. I'm not 100% sure ...
2 years, 7 months ago (2017-04-03 21:41:26 UTC) #2
saroyanm
https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl#oldcode22 includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> We still have ...
2 years, 6 months ago (2017-04-28 10:19:48 UTC) #3
juliandoucette
(I'm going to skip this one today because it's not trivial. I'll get to it ...
2 years, 6 months ago (2017-04-28 18:16:29 UTC) #4
juliandoucette
Updated (finally). I tried to keep this change as small as possible because of it's ...
2 years, 5 months ago (2017-05-22 19:18:33 UTC) #5
saroyanm
https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl#newcode20 includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> What ...
2 years, 5 months ago (2017-06-01 13:09:54 UTC) #6
juliandoucette
Thanks Manvel :) A new patchset is ready. https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl#newcode20 includes/navbar.tmpl:20: <h1 ...
2 years, 5 months ago (2017-06-07 15:42:52 UTC) #7
saroyanm
I don't think we need the element in between. In case I don't understand the ...
2 years, 5 months ago (2017-06-13 10:31:03 UTC) #8
juliandoucette
Thanks Manvel! https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl#oldcode22 includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> > ...
2 years, 5 months ago (2017-06-13 11:17:09 UTC) #9
juliandoucette
See comment below. https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl#newcode20 includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span ...
2 years, 5 months ago (2017-06-13 11:59:31 UTC) #10
saroyanm
https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl#newcode20 includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span class="non-space"> </span><strong>Ads</strong></a></h1> On ...
2 years, 5 months ago (2017-06-13 12:28:20 UTC) #11
juliandoucette
Thanks Manvel :) https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl File includes/navbar.tmpl (right): https://codereview.adblockplus.org/29401619/diff/29445670/includes/navbar.tmpl#newcode20 includes/navbar.tmpl:20: <h1 class="site-title">{{ "index" | linkify(id="navbar-logo") }}Acceptable<span ...
2 years, 5 months ago (2017-06-14 12:05:34 UTC) #12
juliandoucette
https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl File includes/navbar.tmpl (left): https://codereview.adblockplus.org/29401619/diff/29401620/includes/navbar.tmpl#oldcode22 includes/navbar.tmpl:22: <img src="/img/png/logo-black.png" srcset="/img/png/logo-black.png, /img/svg/logo-black.svg 2x" aria-hidden="true"> On 2017/06/13 11:17:09, ...
2 years, 5 months ago (2017-06-14 12:15:21 UTC) #13
saroyanm
2 years, 5 months ago (2017-06-14 13:20:52 UTC) #14
LGTM - with 1 small comment.

https://codereview.adblockplus.org/29401619/diff/29465592/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29401619/diff/29465592/static/css/main.css...
static/css/main.css:1236: word-spacing: 1px; }
I assume you forgot to generate latest version, as I can see it's -2px.
Sign in to reply to this message.

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