-
Notifications
You must be signed in to change notification settings - Fork 750
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
Update font tooling to support semi-bold font weight #11976
Update font tooling to support semi-bold font weight #11976
Conversation
Enforce Python 3.8 for font tooling.
Add flexibility to use variable fonts to fill in missing font weights.
Build Artifacts
|
2c2cdf8
to
ae19b87
Compare
Tested the This newest ![]() @pcenov can you please test the |
Hi @radinamatic no fonts related issues observed while testing with the .deb and .exe builds as well. |
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.
Manual QA passes, should be good to go after the code review! 🙂
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.
I think I got a good grasp of what was going on in the code and the changes made sense and made for a clean grokkable read.
Just left a non-blocking question
except ImportError: | ||
utils.install_requirement(f"fonttools=={font_tools_version}") | ||
except AssertionError: | ||
utils.install_requirement(f"fonttools=={font_tools_version}") | ||
logging.error( | ||
"Wrong fontTools version was installed. This has been updated. Please re-run the command." | ||
) | ||
sys.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.
Why do we notify that we've updated fontTools on one type of error but not the other?
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 assertion error only happens in the case that fontTools was already installed. Unfortunately, because we already imported it, we have to exit Python before we can import the updated version (although, now I think about it, it might be possible to clear the module cache - but generally safer to quit). When the import didn't even succeed, we can reimport again.
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.
Blocking merge into 0.16.x until we are ready for all branding to be merged
e7edd02
into
learningequality:release-v0.16.x
Summary
Reviewer guidance
In the future we may want to be explicit about which typefaces we want to include in the font subsetting code - although, with the dropping of IE11 support in the future, it's possible we can stop doing the subsetting altogether.
Opening this as a draft for now, to make sure that everything builds, check the size of the built WHL file, and also open it up for QA testing to make sure fonts are still behaving as expected.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)