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

Issue 29693561: Noissue - Update font-family to Source Sans Pro on abp.org (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by ire
Modified:
2 months ago
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

Noissue - Update font-family to Source Sans Pro on abp.org Branch: index_page https://hg.adblockplus.org/web.adblockplus.org/shortlog/index_page

Patch Set 1 #

Total comments: 10

Patch Set 2 : Remove unused font formats #

Total comments: 8

Patch Set 3 : Separate font variants #

Total comments: 4

Patch Set 4 : Update licence file #

Total comments: 14

Patch Set 5 : Fix typo #

Patch Set 6 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -6 lines) Patch
M includes/styles.tmpl View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A static/css/fonts.css View 1 2 3 4 1 chunk +255 lines, -0 lines 0 comments Download
M static/css/main.css View 1 2 3 1 chunk +1 line, -6 lines 0 comments Download
A static/fonts/Source-Sans-Pro/300/cyrillic.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/300/cyrillic-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/300/greek.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/300/greek-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/300/latin.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/300/latin-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/300/vietnamese.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/cyrillic.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/cyrillic-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/greek.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/greek-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/latin.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/latin-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/400/vietnamese.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/cyrillic.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/cyrillic-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/greek.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/greek-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/latin.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/latin-ext.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/700/vietnamese.woff2 View 1 2 Binary file 0 comments Download
A static/fonts/Source-Sans-Pro/LICENSE.txt View 1 2 3 1 chunk +93 lines, -0 lines 0 comments Download
R static/fonts/SourceSansPro-Light.woff View Binary file 0 comments Download
R static/fonts/SourceSansPro-Regular.woff View Binary file 0 comments Download
R static/fonts/SourceSansPro-Semibold.woff View Binary file 0 comments Download

Messages

Total messages: 21
ire
3 months, 1 week ago (2018-02-09 12:32:58 UTC) #1
ire
@Manvel please take a look at this when you can
3 months, 1 week ago (2018-02-09 12:34:08 UTC) #2
juliandoucette
https://codereview.adblockplus.org/29693561/diff/29693562/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29693561/diff/29693562/static/css/main.css#newcode6 static/css/main.css:6: src: url("../fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.eot"); I don't think this isn't necessary unless ...
3 months ago (2018-02-13 18:32:06 UTC) #3
ire
https://codereview.adblockplus.org/29693561/diff/29693562/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29693561/diff/29693562/static/css/main.css#newcode6 static/css/main.css:6: src: url("../fonts/Source-Sans-Pro-400/Source-Sans-Pro-400.eot"); On 2018/02/13 18:32:06, juliandoucette wrote: > I ...
3 months ago (2018-02-14 08:43:42 UTC) #4
saroyanm
https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css#newcode1 static/css/main.css:1: @font-face Question: How much effort it would be to ...
3 months ago (2018-02-14 16:49:30 UTC) #5
ire
https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css#newcode1 static/css/main.css:1: @font-face On 2018/02/14 16:49:29, saroyanm wrote: > Question: How ...
3 months ago (2018-02-15 17:21:47 UTC) #6
juliandoucette
Thanks Ire! https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css#newcode1 static/css/main.css:1: @font-face On 2018/02/15 17:21:47, ire wrote: > ...
3 months ago (2018-02-19 11:32:49 UTC) #7
ire
@Manvel : Any update on your findings from your other review? I'm waiting on that ...
2 months, 3 weeks ago (2018-02-26 19:02:54 UTC) #8
saroyanm
On 2018/02/26 19:02:54, ire wrote: > @Manvel : Any update on your findings from your ...
2 months, 3 weeks ago (2018-02-27 10:57:26 UTC) #9
ire
On 2018/02/27 10:57:26, saroyanm wrote: > On 2018/02/26 19:02:54, ire wrote: > > @Manvel : ...
2 months, 3 weeks ago (2018-02-28 09:14:19 UTC) #10
saroyanm
On 2018/02/28 09:14:19, ire wrote: > On 2018/02/27 10:57:26, saroyanm wrote: > > On 2018/02/26 ...
2 months, 3 weeks ago (2018-02-28 09:52:05 UTC) #11
ire
Thanks Manvel! New patch set uploaded. A few comments: 1. Jeen suggested we use the ...
2 months, 2 weeks ago (2018-03-01 14:00:16 UTC) #12
saroyanm
Sorry, for late reply. LGTM https://codereview.adblockplus.org/29693561/diff/29712605/static/css/fonts.css File static/css/fonts.css (right): https://codereview.adblockplus.org/29693561/diff/29712605/static/css/fonts.css#newcode1 static/css/fonts.css:1: /****************************************************************************** Note: This file ...
2 months, 1 week ago (2018-03-13 12:58:19 UTC) #13
ire
Thanks Manvel! I've updated the licence. Julian, any comments on this before I commit&push? https://codereview.adblockplus.org/29693561/diff/29712605/static/css/fonts.css ...
2 months, 1 week ago (2018-03-13 13:50:51 UTC) #14
juliandoucette
On 2018/03/13 13:50:51, ire wrote: > Thanks Manvel! I've updated the licence. > > Julian, ...
2 months, 1 week ago (2018-03-13 14:41:13 UTC) #15
juliandoucette
https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl File includes/styles.tmpl (right): https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl#newcode2 includes/styles.tmpl:2: <link rel="stylesheet" href="/css/fonts.css"> Why a separate file? https://codereview.adblockplus.org/29693561/diff/29721646/static/css/fonts.css File ...
2 months, 1 week ago (2018-03-14 17:08:39 UTC) #16
saroyanm
https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl File includes/styles.tmpl (right): https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl#newcode2 includes/styles.tmpl:2: <link rel="stylesheet" href="/css/fonts.css"> On 2018/03/14 17:08:38, juliandoucette wrote: > ...
2 months, 1 week ago (2018-03-14 17:56:30 UTC) #17
saroyanm
2 months, 1 week ago (2018-03-14 17:56:35 UTC) #18
ire
https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl File includes/styles.tmpl (right): https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl#newcode2 includes/styles.tmpl:2: <link rel="stylesheet" href="/css/fonts.css"> On 2018/03/14 17:08:38, juliandoucette wrote: > ...
2 months, 1 week ago (2018-03-14 18:10:34 UTC) #19
juliandoucette
LGTM + superNIT https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl File includes/styles.tmpl (right): https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl#newcode2 includes/styles.tmpl:2: <link rel="stylesheet" href="/css/fonts.css"> On 2018/03/14 18:10:31, ...
2 months, 1 week ago (2018-03-14 18:18:03 UTC) #20
ire
2 months ago (2018-03-15 08:30:49 UTC) #21
https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmpl
File includes/styles.tmpl (right):

https://codereview.adblockplus.org/29693561/diff/29721646/includes/styles.tmp...
includes/styles.tmpl:2: <link rel="stylesheet" href="/css/fonts.css">
On 2018/03/14 18:18:00, juliandoucette wrote: 
> I'd prefer to update website-defaults and then update defaults.css in
> http://web.adblockplus.org in order to include this CSS in defaults.css. But
that's a
> super-NIT preference. It only matters in the rare case that a user who has a
bad
> connection is having a hard time downloading this small file (which will
rarely
> if ever happen).

I agree, but adding it to website-defaults may not be so straightforward since
we will probably have to make the path to the font files configurable. 

I will push this as it is for now and open a separate review for that
Sign in to reply to this message.

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