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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by ire
Modified:
3 days, 4 hours 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: 4

Messages

Total messages: 6
ire
1 week, 2 days ago (2018-02-09 12:32:58 UTC) #1
ire
@Manvel please take a look at this when you can
1 week, 2 days 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 ...
5 days, 3 hours 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 ...
4 days, 13 hours 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 ...
4 days, 5 hours ago (2018-02-14 16:49:30 UTC) #5
ire
3 days, 4 hours ago (2018-02-15 17:21:47 UTC) #6
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...
static/css/main.css:1: @font-face
On 2018/02/14 16:49:29, saroyanm wrote:
> Question: How much effort it would be to include Fonts implementation in the
> Websites default ?
> Is there a such plan ?
> 
> (I assume we use same fonts across all websites)

I think we would ideally want to have this font in website-defaults and share it
across websites, but it loading a file directly from website-defaults would
require that we add a build step (since the website-defaults repository would be
managed via npm). 

We don't currently have website-defaults in this project (currently Julian has
been manually adding the CSS to defaults.css), so I don't think doing this would
work until we decide to add it.

https://codereview.adblockplus.org/29693561/diff/29697558/static/css/main.css...
static/css/main.css:3: font-family: "Source Sans Pro";
On 2018/02/14 16:49:30, saroyanm wrote:
> Suggestion/Note: Doesn't necessarily need to be included in the current
> review/release.
> 
> I assume current fonts doesn't cover all supported languages ?
> For example I assume they do not contain ex. cyrillic, greek characters, but
> rather only Latin.
> 
> With: https://codereview.adblockplus.org/29654555/ we are trying to fix that
for
> the UI, but I think it's also relevant to the websites.

From my tests (viewing the homepage in all the different languages) there didn't
seem to be any issue with any of the languages. But from looking though the
review you mentioned, it seems like you did come across issues in certain
languages. If that is the case, we could follow your methodology here of loading
different fonts for the different subsets.

I would prefer to start with just this and have a tester see if there is an
issue to begin with though, just to make sure it's actually necessary in our
case. What do you think?
Sign in to reply to this message.

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