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

Add improvements from mrdrogdrog/master #61

Merged
merged 7 commits into from
Jul 20, 2021

Conversation

mrdrogdrog
Copy link
Contributor

@mrdrogdrog mrdrogdrog commented Jul 17, 2021

I really like this project (despite the fact that it's quite a mess. 😅 ) so I want to give back some of my personal improvements.

  • Rename font file to just "twemoji" to get rid of the space.
    The white space in the file name is rather annoying
  • Generate "woff2" instead of "ttf".
    It's 2021. Nobody should use ttf files
  • Reformat "layerize.js"
    Corrected some white spaces, indentations and added triple equal signs
  • Get rid of twe-svg.zip repacking
    I altered layerize.js so you don't have to repackage twe-svg.zip anymore
  • Update libs
    Node lib updates
  • Use python3

Hope you like it!

@jfkthame
Copy link
Collaborator

Thanks for your suggestions/contribution! Could I ask you to split this into individual commits for each item, which can be considered/merged separately? Regarding each of the items mentioned:

Rename font file to just "twemoji" to get rid of the space.

Please make it "TwemojiMozilla.ttf" (i.e. just eliminate the space, but keep the Mozilla suffix because we want to be clear that this is not part of the upstream Twemoji project itself, it's a Mozilla derivative). This will match how it is actually shipped in Firefox (see https://hg.mozilla.org/mozilla-central/file/tip/browser/fonts).

Generate "woff2" instead of "ttf".

I don't necessarily agree with this. TTF is the canonical font format for an OpenType font with TrueType outlines. WOFF2 is a compression/packaging of this targeted at webfont use, but is not necessarily optimal for all use cases. (E.g. a TTF file can be directly mapped into memory and used, whereas the WOFF2 has to first go through a decompression to a separate memory buffer. So depending on the usage scenario, the runtime RAM footprint of WOFF2 may actually be larger.)

Reformat "layerize.js"
Get rid of twe-svg.zip repacking
Update libs
Use python3

These changes sound fine, although I haven't looked at the details yet.

@stone-zeng
Copy link

  • Generate "woff2" instead of "ttf".
    It's 2021. Nobody should use ttf files

Need to mention that a lot of applications other than web browsers cannot use WOFF2 format, so TTF is still necessary.

@mrdrogdrog mrdrogdrog force-pushed the upstream-master branch 3 times, most recently from cd94a36 to 64b7697 Compare July 17, 2021 19:48
@mrdrogdrog
Copy link
Contributor Author

mrdrogdrog commented Jul 17, 2021

Okay, I changed some things:

  • I split the single commit into multiple commits
  • I renamed the font to "TwemojiMozilla" (like before but without the space)
  • I didn't replace the zip file because it doesn't matter right if it's the full zip or the repacked one.
  • I reverted the woff2 change. I still think that a woff2 file should be created as well but I get your concerns.

@mrdrogdrog
Copy link
Contributor Author

What do you think @jfkthame ?

@jfkthame
Copy link
Collaborator

Thanks for the update. One question (as I haven't pulled this and tried a build myself yet):

* I renamed the font to "TwemojiMozilla" (like before but without the space)

Does this rename the font itself (internally) so that its family name no longer has a space? What I'd really prefer to see is that the font family name remains "Twemoji Mozilla" (with a space), while the filename becomes TwemojiMozilla.ttf. Can we get that result?

@mrdrogdrog
Copy link
Contributor Author

Hm as far as I can see no because grunt-webfonts uses the internal name as file name. Well. The name thing isn't that important. I dropped it.

Copy link
Collaborator

@jfkthame jfkthame left a comment

Choose a reason for hiding this comment

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

While you're tidying things up, would you mind also fixing the whitespace in Gruntfile.js? It seems to have an ugly mix of tabs and spaces, which can make it look confusing depending on the tab settings in the editor/viewer.

Unless "real" tabs are strictly required for some reason, I'd prefer to just use spaces so that the indentation is stable regardless of editor settings.

Thanks!

@mrdrogdrog
Copy link
Contributor Author

Fixed it

@jfkthame jfkthame merged commit 93dffc7 into mozilla:master Jul 20, 2021
@jfkthame
Copy link
Collaborator

LGTM - thanks for doing this.

@mrdrogdrog
Copy link
Contributor Author

No problem. I have many more improvements waiting for a PR 😏 (as soon as they're finished)

@mrdrogdrog mrdrogdrog deleted the upstream-master branch July 20, 2021 18:25
@Ypnose
Copy link

Ypnose commented Jul 24, 2021

@mrdrogdrog As it was mentioned in previous comments, replacing TTF by WOFF2 will create issues. Even if Fontconfig seems to support WOFF2 thanks to FreeType, it seems it still has problems. Some people (including me) directly use Twemoji as a font in terminal emulators or other GUI programs.

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

Successfully merging this pull request may close these issues.

4 participants