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

Subsetting Zilla Slab #5024

Merged
merged 5 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@tkadlec
Contributor

tkadlec commented Oct 4, 2018

This PR addresses mdn/sprints#254.

It introduces a subsetted version of the Zilla Slab font to be used instead of the full file, cutting the font file sizes in half.

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Oct 4, 2018

@schalkneethling and @jwhitlock: This one is pretty lightweight, me thinks.

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Oct 4, 2018

Ooo....almost forgot about the file I used to subset. I mentioned it in mdn/sprints#254:

I created a file that contains the unicode ranges to include. There are other shortcuts to go to certain subsets, but I like the clarity this gives us in case we edit the subset in the future

I wasn't sure exactly where to put it. Right now it's in /kuma/static/.

U+2000-206F
# Currency Symbols
U+20A0-20CF

This comment has been minimized.

@schalkneethling

schalkneethling Oct 4, 2018

Collaborator

Nit: Missing empty line at end of file. This seems to be a new file? I am genuinely curious as to the purpose of this file? @tkadlec ?

This comment has been minimized.

@tkadlec

tkadlec Oct 5, 2018

Contributor

Added the new line. Also added some info to the top of the file for clarity. The file is used to generate the subset. You can see more information here: mdn/sprints#254 (comment)

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 4, 2018

@tkadlec Thanks for this. How would you suggest we go about testing this? Which pages etc. did you test? Thanks!

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Oct 5, 2018

@schalkneethling Good question! I tested a handful of random pages in each locale. Here are the pages I tested:

/
/docs/Web/JavaScript
/docs/Mozilla/Add-ons/WebExtensions
/docs/Web/HTML
/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

And here are the locales:

Svenska (sv-SE)
Portugues (pt-PT) and (pt-BR)
Polski (pl)
Nederlands (nl)
Melayu (ms)
Taqbaylit (kab)
Italiano (it)
Bahasa Indonesia (id)
magyar (hu)
Francais (fr)
suomi (fi)
Espanol (es)
Deutsche (de)
Catala (ca)
English (en-US)

It's tedious, unfortunately. :/

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 5, 2018

Thanks for the additional information @tkadlec - I will take this for a test run.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 5, 2018

@tkadlec I notice that the issue mentions subsetting ZillaSlab and OpenSans but this only address ZillaSlab. Is that intentional, perhaps to keep the work separate?

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Oct 5, 2018

Ah, nope. We removed Open Sans so I didn't do any subsetting there.

That being said, I suppose I haven't looked at all locales to see if we still have the full version of Open Sans lingering somewhere.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 5, 2018

@tkadlec I see there are still opensans that looks like it is being used for https://github.com/mozilla/kuma/tree/master/kuma/static/fonts/locales/ro

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Oct 5, 2018

@schalkneethling

I have tested a bunch of pages in almost all possible locales and did not find a missing character anywhere 👍 r+ @jwhitlock you want to take this for a test drive as well or, happy to merge?

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 10, 2018

No, that review works for me.

@jwhitlock

I'll need a bug number to merge, however. It looks like these commits should be squashed on merge.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 10, 2018

Thanks @jwhitlock. @tkadlec Can you attach a bug number to this one and rebase against master so we have one commit? Thanks!

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 10, 2018

I can't find a relevant existing bug, I'll write a new one. I can add a bug # and squash on merge.

@jwhitlock jwhitlock merged commit 7c814ec into mozilla:master Oct 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No manifest changes detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment