-
Notifications
You must be signed in to change notification settings - Fork 36
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 webfont and script to subset a font. #35
base: master
Are you sure you want to change the base?
Conversation
Subsetted HanaMinA also included.
style.css
Outdated
.nrGrammardata { | ||
background: white; | ||
font-family: serif, HanaMinA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always pick serif
because it will always work as per the CSS spec, so you'll have to swap these around. The generic family class keyword always has to come last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was if the serif
font has CJK glyphs than the reader probably likes the default font and don't really need the HanaMinA substitute. But if the default serif
doesn't have CJK glyphs, which is I guess common for learners, they have their nice readable latin font and only CJK glyphs fall back to HanaMinA.
Alternative is to have a good looking latin font before HanaMinA and serif at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I have no intention of honoring the reader's locally installed fonts: the reason we're subsetting is to ensure that everyone sees the same thing, with a font that covers exactly what is necessary =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HanaMinA's latin is really ugly here. Added Times first.
style.css
Outdated
@@ -52,7 +58,7 @@ | |||
height: 500px; | |||
text-align: right; | |||
font-size: 13px; | |||
font-family: Arial; | |||
font-family: Arial, HanaMinA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably good to add a , sans-serif
since we're touching this rule anyway.
webfont/make_woff.py
Outdated
|
||
|
||
if sys.version_info[0:2] < (3, 6): | ||
error("This script requires Python version 3.6") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.6 or higher
?
webfont/make_woff.py
Outdated
import fontTools.merge | ||
import fontTools.misc.loggingTools | ||
except ImportError: | ||
error("Please install the fontTools python module.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels like we should be able to invoke pip here.
accepted = ask("FontTools dependency is missing, would you like to install them now? [Y/n]");
if accepted is True:
runPipInstalls()
else:
error("Dependencies not installed, exiting...")
exit(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are just so many possibilities. Do you want it to be installed with the distribution's package manager or pip? Install globally (which also needs root permission probably) or only for the current user or maybe in a virtual environment? But I can make something if it's really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offering the choice, only makes it more usable. We can add in the option to use pip
(which is the defacto manager these days) and if someone says "no" then they get exactly as much functionality as if we just immediately exit. So worst case: things stay the same. Best case: the script takes care of it and just continues.
Let's add in a pip install (and if it complains about permissions, we can make it say "could not install dependencies, please install the following packages manually:" and then a list of the dependencies necessary)
webfont/make_woff.py
Outdated
|
||
class BaseFilter(): | ||
def feed(self, codepoint): | ||
"""Let the filter handle the codepoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let a subclassing filter handle the codepoint
because without overriding this function literally breaks python.
webfont/make_woff.py
Outdated
uniq_filt = UniqueFilter() | ||
pipeline.add_filter(uniq_filt) | ||
|
||
excl_filt = ExcludeFilter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ASCII range takes up near-as-makes-no-difference nothing compared to the rest of the data that gets loaded, and having the Hanazono-specific latin characters in the font means that stretches that end up being marked as "use Japanese typesetting, even for the latin bits" will look better later.
Let's take out the exclusion option.
return False | ||
|
||
|
||
class ExcludeFilter(BaseFilter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
webfont/woff.conf
Outdated
"dataFiles": [ | ||
"../data/pages/en-GB/**/*.txt" | ||
], | ||
"excludeCodepoints": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some review comments as normal comments.
Having both PHP and Python as build tools might cause some problems when it comes to adding CI to this, but that can be a later concern. I've left a few comments but overall this looks fine to me. Thank you for helping get this implemented. |
Useful to ignore missing TAB and LFD, those don't have glyphs in font files.
style.css
Outdated
@@ -1,11 +1,12 @@ | |||
@font-face { | |||
font-family: HanaMinA; | |||
src: url(HanaMinA.woff); | |||
src: local("HanaMinA") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is still to offer exactly the font necessary so that everyone sees exactly the same, so let's remove local(), because there's no guarantee their local font is the same font version as the one used to build the woff subset font.
If the initial .woff is too big up front, let's use woff2 instead and slice it so that it loads incrementally based on what's necessary per chapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
323K as woff, 241K as woff2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brotli showing off its power. Very nice!
woff2 is used if the output filename ends in '.woff2' else woff used.
Unless slicing for sections are needed, I think I'm done. |
How about this for #34?
@font-face
rule and HanaMinA subsetted webfont.Hanazono font was chosen because for Han Nom:
Do you want anything different?