Skip to content

Fontdeck module - small change #37

Merged
merged 5 commits into from Nov 29, 2011

4 participants

@ahume
ahume commented Nov 9, 2011

Hi,

Quick summary of this:

Instead of dealing with the mechanics of loading fonts within the WFL module itself (via adding a CSS link), we now call out to a JS API on our side that handles this, and then simply passes the loaded fonts and a support status back to WFL.

There's some changes in the tests to reflect this, plus a few extra tests for a bit more coverage.

A code review would be great - thanks as always.

Cheers,
Andy.

@seanami seanami commented on an outdated diff Nov 17, 2011
src/fontdeck/fontdeck_script.js
var api = this.configuration_['api'] || webfont.FontdeckScript.API;
- return api + 'project=' + projectId + '&domain=' + document.location.hostname + '&callback=window.__webfontfontdeckmodule__[' + projectId + ']';
+ return protocol + api + document.location.hostname + '/' + projectId + '.js';
@seanami
seanami added a note Nov 17, 2011

Making document an argument to your webfont.FontdeckScript constructor and then saving a reference to it as this.document_ for use within the class would make your module easier to test. For testing, you could pass in a fake document object with a hostname that you specify. At the bottom of this file, you could just instantiate your class with a reference to the actual document (just like DomHelper and UserAgentParser).

That way your test would pass even when the tests aren't run on localhost. Not a requirement, but just a suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanami
seanami commented Nov 17, 2011

I took a look through this and didn't see anything glaring. Looks like a fairly straightforward change. It's nice that it's protocol relative now.

I did have one suggestion in order to make your test more generic (and allow it to pass when hostname isn't localhost). Let me know what you think.

@ahume
ahume commented Nov 18, 2011

I've got the tests working in more or less the way you described - except rather than passing in a ref to document, I'm using the global that is already being passed in as the first argument. This gives access to window and document for use in tests.

@seanami
seanami commented Nov 22, 2011

Nice, I think that's a good change. This looks like it's ready to go.

There's another pull request from Jeremie that's currently being debated. Perhaps once that's ready (hopefully in the next few days?) then we can put both into one release. Otherwise, if that takes to long, we can do two versions to not delay yours.

Do you have any sort of time constraints on this going out?

@ahume
ahume commented Nov 22, 2011

No urgency, so happy to wait for Jeremie's change. Thanks Sean.

@seanami seanami merged commit d0711de into typekit:master Nov 29, 2011
@seanami
seanami commented Nov 29, 2011

Alright, this is merged and released in version 1.0.23. I'll get Jeremie to get that version up as the latest on the Google CDN as soon as possible.

@wolthers
wolthers commented Dec 2, 2011

Fontactive is still firing too early in 1.023. I updated the jsfiddle from #26 -> http://jsfiddle.net/QY2Ts/

@seanami
seanami commented Dec 2, 2011

Hey @wolthers. This is the wrong pull request for that issue, but that's OK. (The right one is over here: #39 ).

Which module are you using when loading fonts in the test? Currently, only the Google module is using the new code that should prevent early font active events. We chose not to make it the default because it can cause severe delays in active events when loading fonts that happen to have the same width as a set of last resort fallback fonts, so the downsides are pretty severe for the average use case.

If you'd like to use the new code, you'd need to pull down a copy of webfontloader and rig up your own custom module that uses it. But unless you're loading Google fonts, you won't get the alternate behavior with the default package.

@ahume
ahume commented Dec 9, 2011

I don't suppose you know how we can get the documentation for the Fontdeck module on to this page:

http://code.google.com/apis/webfonts/docs/webfont_loader.html

Is Jeremie the person to talk to about that?

@seanami
seanami commented Dec 9, 2011

Jeremie might be the best bet, although that particular page only mentions the Google module, and I think it's because it's a page dedicated to the Google web font API specifically (and not the web font loader open source project generally). So they may not be open to adding it, but he'd be the one to ask.

@ahume
ahume commented Dec 9, 2011

Yeah, I always thought it only mentioned G and Typekit, but it was just pointed out to me that it has examples of ascender and monotype as well. Will ask Jeremie. Thanks.

@seanami
seanami commented Dec 9, 2011

Oh, sure enough! I didn't see that section because the section header in the outline didn't mention the other providers. I'm sure if you talk to Jeremie, he'd know how you can get another example in there.

@ahume
ahume commented Jan 23, 2012

Hi Shaun, do you have an email address for Jeremie that you can message me privately? He hasn't provided one on Github.

@ahume
ahume commented Jan 23, 2012

Shaun -> Sean :/

@seanami
seanami commented Jan 24, 2012

Hmmm, it seems that you haven't provided an address either, so I can't privately message you from your profile page. Maybe if we mention Jeremie here he'll see it? @jeremiele, Andy is wondering about adding a reference to their module to the Google documentation about webfontloader.

@jeremiele
@ahume
ahume commented Jan 24, 2012

That's great, thanks very much. This should do the job:

WebFontConfig = {
    fontdeck: {
        id: 'xxxxx'
    }
};
@jeremiele
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.