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

General: Bundle a local version of Merriweather instead of loading from Google Fonts. #2766

Merged
merged 2 commits into from
Feb 11, 2016

Conversation

mattmiklic
Copy link
Member

On January 22, the version of Merriweather hosted on Google Fonts was updated to version 1.582. This version includes significant visual departures from the original version of Merriweather. This PR bundles the original version of Merriweather into Calypso, and replaces the existing references to Google Fonts' version of Merriweather.

I've tested in the following browsers: IE 11, Edge, Chrome 48 (Mac/Win), Firefox 43 (Mac/Win), Safari 9, Safari for iOS 9, Chrome 47 Android, Firefox 43 Android.

The "before" in the following screenshots show what Calypso looks like today. The "after" is the result of this PR, and is identical to what Calypso looked like prior to January 22.

Chrome 48 Win:
chrome-48-win

Firefox 43 Win:
firefox-43-win

Edge:
edge-win

IE 11 (could not log into calypso.localhost in IE11, screenshot is from /devdocs/):
ie-11-win

Chrome 48 OS X:
chrome-48-osx

Firefox 43 OS X:
firefox-43-osx

Safari 9 OS X:
safari-9-osx

Safari 9 iOS:
safari-9-ios

Chrome 47 Android:
chrome-47-android

Firefox 43 Android:
firefox-43-android

TODO:

  • Test more.
  • Verify whether we need all of the font formats that I've included.
  • Check that I've put the font in the right place and code style looks right.
  • Probably something else -- what potential problems might this create?
  • Any potential impact on speed?

cc @apeatling

@mattmiklic mattmiklic added [Status] In Progress [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Jan 25, 2016
@mattmiklic
Copy link
Member Author

Everything seems to be working as expected here, but would love to get more eyes on it, particularly on non-Mac, non-iOS devices if possible.

@apeatling
Copy link
Member

I think we will want to serve this from a CDN on WordPress.com. I wonder if it's better to add it to our CDN and then include that in Calypso instead?

@mattmiklic
Copy link
Member Author

I think we will want to serve this from a CDN on WordPress.com. I wonder if it's better to add it to our CDN and then include that in Calypso instead?

I figured that would be the case for WordPress.com. I thought bundling it locally would be nice for local development (one fewer external dependency), but don't have a strong opinion on which way we include it.

@shaunandrews
Copy link
Contributor

Gave it a spin on Mac (safari, chrome, firefox), Windows 10 (Edge, Firefox, Chrome) and Android (chrome) and all works as expected.

Also, my eyes thank you.

@apeatling
Copy link
Member

@mattmiklic I think it's good to bundle it locally too. What if we first try the WordPress.com CDN, and then fall back to the local version that you have here?

@mattmiklic
Copy link
Member Author

What if we first try the WordPress.com CDN, and then fall back to the local version that you have here?

This makes sense to me.

Gave it a spin

Thank you @shaunandrews!

@apeatling
Copy link
Member

@mtias Do you have some thoughts here? We need to load the old version of Merriweather, so we're bundling it locally to load. I'd imagine that for WordPress.com we will want to use the CDN. What's the best way to load it from the CDN for WordPress.com only? Or should we just load it from the CDN for everyone, and fall back to a local copy with a bad connection?

@artpi
Copy link
Contributor

artpi commented Jan 26, 2016

@apeatling, @mattmiklic,

What if we first try the WordPress.com CDN, and then fall back to the local version that you have here?

I was playing around with these ideas in #2719

@mattmiklic, maybe we can combine our efforts? I want all resources to be loaded locally if cdn is not accessible.

We can detect CSS missing by (this is pages/index.jade) :

        script.
            (
                window.cssFallback = function() {
                    console.log('not loaded', this);
                }
            )
        link(rel='stylesheet', href='//s1.wp.com/potato.css', onerror='window.cssFallback()')

Apparently support for onerror is not that bad and it would work in desktop app, because chromium supports it.

@mattmiklic
Copy link
Member Author

@mattmiklic, maybe we can combine our efforts? I want all resources to be loaded locally if cdn is not accessible.

Makes sense to me. We'll want to use the font files I've added here, since they'll give us browser support that should be equal to what we got with Google Fonts. I'll defer on the code that actually calls them, since I think you guys know the best way to handle that.

@mattmiklic mattmiklic removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jan 29, 2016
@mattmiklic
Copy link
Member Author

@artpi @apeatling Is there anything else you guys need from me to make this change?

@mtias
Copy link
Member

mtias commented Feb 2, 2016

sorry @artpi, I will look at this tomorrow.

@apeatling
Copy link
Member

@mtias Would really like to get this one squared away so we don't have the new version of Merriweather sticking around. Could you take a look here?

@@ -124,7 +124,7 @@ const CONTENT_CSS = [
window.app.tinymceWpSkin,
'//s1.wp.com/wp-includes/css/dashicons.css',
window.app.tinymceEditorCss,
'//fonts.googleapis.com/css?family=Merriweather:700%2C400%2C700italic%2C400italic',
'/calypso/fonts/merriweather/merriweather.css',
Copy link
Member

Choose a reason for hiding this comment

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

@mattmiklic can we have this in WP.com cdn as well?

@mtias
Copy link
Member

mtias commented Feb 9, 2016

@apeatling @mattmiklic @artpi let's get this loaded from WP.com CDN to start as the fastest immediate fix for the visual design we want.

We should continue looking at loading local assets for development separately like Artur proposes but no need to conflate the two issues right now.

@mattmiklic
Copy link
Member Author

let's get this loaded from WP.com CDN to start as the fastest immediate fix for the visual design we want.

This sounds good to me -- I'm not sure how to go about doing this, though.

@mtias mtias force-pushed the try/merriweather-local branch from 053faee to 481fcb1 Compare February 11, 2016 15:07
mattmiklic and others added 2 commits February 11, 2016 16:10
Update reference to Merriweather in Tinymce.

Switch 404, 500, desktop templates to use local Merriweather.

Add Merriweather license.

Trim whitespace.

Arrange by weight.
@mtias mtias force-pushed the try/merriweather-local branch from 481fcb1 to 300a7f2 Compare February 11, 2016 15:10
@mtias
Copy link
Member

mtias commented Feb 11, 2016

@apeatling @mattmiklic mind giving this branch a go?

@mtias mtias added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 11, 2016
@mattmiklic
Copy link
Member Author

@mtias Tested the following:

  • Chrome OS X, Win, Android: loaded WOFF2
  • Firefox OS X, Win, Android: loaded WOFF2
  • Safari OS X, iOS: loaded WOFF
  • Microsoft Edge, IE 11: loaded WOFF

Everything looked good.

@mtias
Copy link
Member

mtias commented Feb 11, 2016

Excellent!

@apeatling
Copy link
Member

👍 Looks good

@apeatling apeatling added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 11, 2016
mattmiklic added a commit that referenced this pull request Feb 11, 2016
General: Bundle a local version of Merriweather instead of loading from Google Fonts.
@mattmiklic mattmiklic merged commit 52b6cbb into master Feb 11, 2016
@mattmiklic mattmiklic deleted the try/merriweather-local branch February 11, 2016 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants