Skip to content
This repository has been archived by the owner on Oct 9, 2020. It is now read-only.

Change the import syntax to a more friend and customizable way #11

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

LukeXuan
Copy link

I've changed the syntax of import to

  [service@]font_or_id_name[@version][#timeout]

And should support all Typekit modules.

Also I added the registry key to the package.json, absence of which causes the original package not to work here.

Luke Lazurite added 3 commits January 25, 2016 21:26
Change import syntax.
Change to ES6 Module.
According to jspm package specification.
add some documentaion.
@@ -1,8 +1,24 @@
{
"main": "font",
"author": [
"LukeXuan"
Copy link
Contributor

Choose a reason for hiding this comment

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

This I assume is an oversight, and should either be removed or changed to something more generic, like systemjs-font contributors. If we include everyone here, which is another good alternative, I think it will become stall fairly quickly.

Copy link
Author

Choose a reason for hiding this comment

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

Well that's my fault, going to correct it in the next commit.

Copy link
Author

Choose a reason for hiding this comment

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

Modify to all contributors listed in github and generic authors appended. It was my fault to add only my name on it when I want to complete the package.json. Please forgive my offense.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I've done this myself.

@maxnordlund
Copy link
Contributor

Changing the format will break existing code, and should be avoided if possible. If however this doesn't do that I'm ok with this. But please add, not remove, tests for both formats to ensure this.

@LukeXuan
Copy link
Author

Changing the format was my original propose, since It's hard to understand the original format from the test.html for me. And for the same reason I cannot support both format:(

@LukeXuan
Copy link
Author

Maybe I'll contribute more time on the original repository and this pr I will close.

@LukeXuan LukeXuan closed this Jan 25, 2016
@maxnordlund
Copy link
Contributor

That not so good, let's pull in @guybedford to hear what he has to say. (Maybe I'm just being paranoid and we should bump a major version)

@maxnordlund maxnordlund reopened this Jan 25, 2016
@guybedford
Copy link
Member

Thanks for the PR. Plugin contributors are very much in need. How about [service:]font_or_id_name[@version][#timeout] for the syntax? Ideally I would actually like this plugin to be entirely a third-party project. @LukeXuan if you'd like to keep it at your own repo, I'd gladly deprecate this one for that.

@LukeXuan
Copy link
Author

LukeXuan commented Feb 3, 2016

@guybedford I originally intended to use that one. However I find that the address I receive by System.import("google:Open Sans") in an HTML file differs from that received by import "google:Open Sans". So I changed it to @

@guybedford
Copy link
Member

@LukeXuan I've updated the URL polyfill implementation in SystemJS to properly support google:x style URLs so they don't turn into google://x. This will be coming in the 0.19.21 version of SystemJS later today. Let me know if that might work for you here then?

@HongJheLi
Copy link

@LukeXuan Thanks, it works.

So, does this PR become official system-font?

@HongJheLi
Copy link

@LukeXuan jspm bundle-sfx error...

err  ReferenceError: window is not defined
            at eval (/jspm_packages/github/components/[email protected]/webfontloader.js:19:1701)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants