-
Notifications
You must be signed in to change notification settings - Fork 102
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
Is there a plan to support ligatures? #182
Comments
Hello, I've created a PR to add a proposal to add ligatures. Inspired by the comment of @thepian , I've digged into webfonts-generator code and looked to where it adds the ligature here. I've done the changes locally and I've run the tests, I'm aware that they're failing, but so far I think that the returned value is "correct" considering the changes on the asset generation. Summary of all failing tests
FAIL generators/asset-types/__tests__/svg.ts
● `SVG` font generator › resolves with the result of the completed `SVGIcons2SVGFontStream`
expect(received).toMatchSnapshot()
Snapshot name: `\`SVG\` font generator resolves with the result of the completed \`SVGIcons2SVGFontStream\` 1`
Snapshot: "processed->content->/root/foo.svg|{\"name\":\"foo\",\"unicode\":[\"\\u0001\"]}$processed->content->/root/bar.svg|{\"name\":\"bar\",\"unicode\":[\"\\u0001\"]}$"
Received: "processed->content->/root/foo.svg|{\"name\":\"foo\",\"unicode\":[\"\\u0001\",\"foo\"]}$processed->content->/root/bar.svg|{\"name\":\"bar\",\"unicode\":[\"\\u0001\",\"bar\"]}$"
77 | });
78 |
> 79 | expect(result).toMatchSnapshot();
| ^
80 | });
81 |
82 | test('passes correctly format options to `SVGIcons2SVGFontStream`', async () => {
at generators/asset-types/__tests__/svg.ts:79:20
at fulfilled (generators/asset-types/__tests__/svg.ts:24:58)
● `SVG` font generator › passes correctly format options to `SVGIcons2SVGFontStream`
expect(received).toMatchSnapshot()
Snapshot name: `\`SVG\` font generator passes correctly format options to \`SVGIcons2SVGFontStream\` 1`
Snapshot: "processed->content->/root/foo.svg|{\"name\":\"foo\",\"unicode\":[\"\\u0001\"]}$processed->content->/root/bar.svg|{\"name\":\"bar\",\"unicode\":[\"\\u0001\"]}$"
Received: "processed->content->/root/foo.svg|{\"name\":\"foo\",\"unicode\":[\"\\u0001\",\"foo\"]}$processed->content->/root/bar.svg|{\"name\":\"bar\",\"unicode\":[\"\\u0001\",\"bar\"]}$"
85 | const result = await svgGen.generate(mockOptions(formatOptions), null);
86 |
> 87 | expect(result).toMatchSnapshot();
| ^
88 |
89 | expect(SVGIcons2SVGFontStream).toHaveBeenCalledTimes(1);
90 | expect(SVGIcons2SVGFontStream).toHaveBeenCalledWith({
at generators/asset-types/__tests__/svg.ts:87:20
at fulfilled (generators/asset-types/__tests__/svg.ts:24:58) Let me know if I miss something or there is something else to take into account. It'd be really nice to have ligatures creation added into the library. |
I need ligatures for my case as well, so I've tried the @oscargm PR and it seem to work fine here. Tested here: https://fontdrop.info/#/?darkmode=true on the Ligatures Tab and on my project and everything is going fine! One thing to keep in mind is that it increases the final font file size, in my case the TTF file had around 240kb, and it increased to 320kb. So its probably better to keep it behind a config flag like @oscargm have mention as next step. |
If @tancredi don't see anything wrong, PR should be mergeable. I've added:
And fixed all the tests, SVG generator is tested with and without ligatures. |
As described in Workshape/icon-font-generator#12 (comment)
The text was updated successfully, but these errors were encountered: