Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get Open Sans font from Google CDN instead of local hosting (and include extended character sets) #3556

Merged
merged 9 commits into from
Mar 25, 2014

Conversation

sathomas
Copy link
Contributor

get Open Sans font from Google CDN instead of local hosting (and include extended character sets)

@sathomas
Copy link
Contributor Author

Retargeting of #3369

@sathomas
Copy link
Contributor Author

assigned to @nicolaasmatthijs for review

@nicolaasmatthijs
Copy link
Contributor

@sathomas : Looks like there is a merge conflict here.

@sathomas
Copy link
Contributor Author

merge conflicts fixed, back to @nicolaasmatthijs for review

@nicolaasmatthijs
Copy link
Contributor

Moving this to bootstrap as it fixes a bug in there

@sathomas
Copy link
Contributor Author

Note to folks regarding this PR. I just checked and Vietnamese is listed as a requested language for OAE at crowdin.net. If/when we actually have a Vietnamese translation, we'll need to revisit this issue to add support for Vietnamese characters in the font. (Presently they're not included to minimize download time.)

@nicolaasmatthijs
Copy link
Contributor

It looks like the font is no longer included correctly when using this with a production build.

Now:

screen shot 2014-03-20 at 09 14 29

Before:

screen shot 2014-03-20 at 09 16 52

Assigning back to @sathomas for investigation. You might want to liaise with @stuartf and @mrvisser, 2 world-renowed experts in production builds.

@sathomas
Copy link
Contributor Author

@stuartf, @mrvisser I will definitely need some oversight (if not outright assistance). Digging through the grunt files, we're using r.js to optimize CSS files by inlining any @import statements. In this case, however, referencing the Google Fonts CDN requires an @import statement that should not be inlined, namely:

@import url('//fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,700italic,400,300,600,700&subset=latin,cyrillic-ext,latin-ext,greek-ext');

There is an option to r.js to avoid inlining specific CSS files.

If optimizeCss is in use, a list of files to ignore for the @import inlining. The value of this option should be a string of comma separated CSS file names to ignore (like 'a.css,b.css'. The file names should match whatever strings are used in the @import calls.

And now, perhaps, you can already see the problem; r.js is expecting a "comma separated list" but the URL itself includes commas. I'll try some experiments to figure out if there's any value that we can set to coerce r.js to ignore the Google CSS, but

  1. The r.js source code doesn't inspire a lot of confidence with comments such as "This is not done in the regexp, since my regexp fu is not that strong", and
  2. If it works at all it's probably going to result in some ugly bastardization of the filename such as

cssImportIgnore: 'css'

or

cssImportIgnore: 'css?family=Open+Sans:300italic'

Any suggestions would be most welcome.

@@ -93,7 +93,7 @@ module.exports = function(grunt) {
'optimize': 'uglify',
'preserveLicenseComments': false,
'optimizeCss': 'standard',
'cssImportIgnore': null,
'cssImportIgnore': '//fonts.googleapis.com/css?family=Open+Sans:300italic,400italic,600italic,700italic,400,300,600,700&subset=latin,cyrillic-ext,latin-ext,greek-ext',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to make a TODO here referencing your open PR r.js to change to a more robust regexp match.

@sathomas
Copy link
Contributor Author

Over to @nicolaasmatthijs for official review

@@ -40,3 +40,7 @@ grunt test
```

Note that Hilary and its dependencies should be installed and running on your system before the tests can be run successfully.

## Local Development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to move this to just below ## Installing OAE, as it seems better placed there

@nicolaasmatthijs
Copy link
Contributor

Still seeing the visual regression:

screen shot 2014-03-25 at 12 19 18

@nicolaasmatthijs
Copy link
Contributor

Assigning to @sathomas for investigation

@sathomas
Copy link
Contributor Author

Adjusted build and it looks to be working locally for me; time for another @nicolaasmatthijs review

@nicolaasmatthijs nicolaasmatthijs merged commit 9f6670c into oaeproject:bootstrap3 Mar 25, 2014
@nicolaasmatthijs
Copy link
Contributor

Looks good. Merged.

Verified that this fixes #3617 as well.

@sathomas sathomas deleted the bootstrap3-issue3369 branch March 25, 2014 15:59
@bertpareyn bertpareyn modified the milestone: Bootstrap 3 Apr 1, 2014
@bertpareyn bertpareyn added size 1 and removed size 1 labels May 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants