Adding suppport for "Optimizing your font requests (Beta)" #75

Merged
merged 6 commits into from Jan 21, 2013

Conversation

Projects
None yet
4 participants
Contributor

vitalyvolkov commented Dec 25, 2012

No description provided.

This is for #49

@bramstein bramstein and 1 other commented on an outdated diff Dec 29, 2012

src/google/fontapiurlbuilder.js
@@ -63,5 +64,9 @@ webfont.FontApiUrlBuilder.prototype.build = function() {
url += '&subset=' + this.subsets_.join(',');
}
+ if (this.text_.length > 0) {
+ url += '&text=' + escape(this.webSafe(this.text_));
@bramstein

bramstein Dec 29, 2012

Owner

Could you change escape to encodeURIComponent?

Owner

bramstein commented Dec 29, 2012

The changes look good. I've made a small comment, and I would also like to see one or two tests for this new feature in the test suite.

You don't need the call to webSafe. encodeURIComponent will take care of encoding spaces (and I assume Google's API supports percent encoded spaces as well.)

Owner

bramstein commented Dec 29, 2012

Minor nitpicking, but aren't the last two tests redundant? They seem to test parts of the code that you didn't modify. Perhaps you could add a test that explicitly tests that &text=... isn't added when an empty string is given?

Owner

bramstein commented Jan 9, 2013

Looks good to me. How about you @seanami?

Contributor

seanami commented Jan 11, 2013

Yeah, it looks good to me too. Maybe someone from Google take a look since it's their module?

@bramstein bramstein added a commit that referenced this pull request Jan 21, 2013

@bramstein bramstein Merge pull request #75 from hash3g/master
Adding suppport for "Optimizing your font requests (Beta)"
8fc28e1

@bramstein bramstein merged commit 8fc28e1 into typekit:master Jan 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment