Update primary font to Roboto across all apps #4022
Conversation
Didn't add integration test packages to apps that don't have it yet, since we're getting enough coverage of AppBase through these tests anyway.
kofi-q
requested review from
jonahkagan
and removed request for
a team and
jonahkagan
|
Leaving Drew as primary reviewer, but probably also worth a sanity check from @jonahkagan |
|
I don't see Roboto de-emphasized in the preview |
Oh, good catch! Will take a look and fix that font weight - thanks |
| if (!disableFontsForTests) { | ||
| loadFonts(); | ||
| } | ||
| }, [disableFontsForTests]); |
There was a problem hiding this comment.
Does this mean there will be an initial render without fonts loaded? Does that cause any visual weirdness?
There was a problem hiding this comment.
Yeah, good question - I didn't see any noticeable flicker on initial render when I was testing, but I'm going to try it out with caching disabled to see what kind of impact it might have
There was a problem hiding this comment.
Couldn't spot it while watching the refresh, but managed to catch a frame on a screen recording where the text is in sans-serif just before switching to Roboto. Just to be safe, I'm going to delay the rendering until loadFonts has run
There was a problem hiding this comment.
Nice, thanks for looking into that
Ok, turned out to be an old screenshot from when I had a bug in one of the font definitions - fixed the screenshot now |
| React.useEffect(() => { | ||
| /* istanbul ignore next - tested via integration tests. */ | ||
| if (!disableFontsForTests) { | ||
| loadFonts(); |
There was a problem hiding this comment.
React runs useEffect multiple times in development, so we end up with multiple <style> elements being added. While this may not cause any visual issues, it's maybe not great for resource usage and keeping things close to production if we can.
There was a problem hiding this comment.
Oh, just seeing this! Thanks for the callout - didn't know about the multiple useEffect invocations in dev. Will add a cleanup then, to address the dev weirdness
Overview
Easier by commit - last 2 commits are just snapshot updates and dead code deletion.
Slack context here.
Replacing
Helvetica NeuewithRobotoas the primary font in apps, for consistency across the website, marketing materials and apps.Fonts downloaded from Google Fonts and converted to Base64 encoding with the Font Squirrel tool, to allow us to embed the fonts in a single shared source file.
<AppBase>component used across our apps, but disabling for unit tests, since it seemed to slow them down quite a bit.Robotoare fairly similar toHelvetica Neue.Noto Emojifonts, but we seem to be using them in a few places - we can replace those withIcons later and then deprecateNoto Emoji.Demo Video or Screenshot
Testing Plan
<AppBase>Checklist
[ ] I have added logging where appropriate to any new user actions, system updates such as file reads or storage writes, or errors introduced.