Skip to content

Fix: Font weights #116

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

Merged
merged 9 commits into from
Aug 29, 2023
Merged

Fix: Font weights #116

merged 9 commits into from
Aug 29, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Aug 23, 2023

fixes #101
closes #37
depends on #107 to be merged first

What this PR does

  • Use preloaded externally-hosted Google web font instead, with specific font weights
  • Changes font weights

Font weights

This is a little chaotic 😰 but the font-weights in Figma don't always match up with what we want on the browser for CSS.

  css figma
a 500 700
h1 - -
h2 700 700
h3 700 700
h4 600 800
button 600 700
pill 400 600

I just had to visually test different weights and get it approved with Segacy. Generally, the smaller font sizes don't need to be as heavy, font-weight-wise. Whereas, H2 and H3 are fine as 700, H4, A or buttons are too heavy, and have to have decreased font-weights. This makes it even more important that all type use an already-defined type style (a, h1, h2, h3, h4, button, body/p) - so we know it's correct. I haven't added H1 yet simply b/c none of our pages have an H1 yet - that will come with the About page creation.

In case you're wondering why this discrepancy exists:

Screenshots

H2 class

image image image

H3 class

image image

H4 class

image

Pill

image

Links

image

Buttons

image image

@machikoyasuda machikoyasuda added this to the Home Page milestone Aug 23, 2023
@machikoyasuda machikoyasuda self-assigned this Aug 23, 2023
@machikoyasuda machikoyasuda requested a review from a team as a code owner August 23, 2023 21:30
@netlify
Copy link

netlify bot commented Aug 23, 2023

Deploy Preview for compilerla ready!

Name Link
🔨 Latest commit eb0d844
🔍 Latest deploy log https://app.netlify.com/sites/compilerla/deploys/64ee64948794a50008119e8a
😎 Deploy Preview https://deploy-preview-116--compilerla.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@angela-tran
Copy link
Member

Hi @machikoyasuda - here is the reasoning for having our own copy of the fonts: #5 (comment). The TLDR is at the bottom of the linked article:

Google Fonts resources will be redownloaded for every website, regardless it being cached on the CDN. Self-host your fonts for better performance. The old performance argument is not valid anymore.

@machikoyasuda
Copy link
Member Author

@angela-tran Yea I can move it back. I just did it this way for now so I can quickly get all the weights up and make a Netlify Preview, so Segacy can confirm what weights we need.

The font weight values from Figma aren't 1:1 with what we actually need for the browser.

@machikoyasuda
Copy link
Member Author

@angela-tran Google argues otherwise 😆 https://web.dev/font-best-practices/#using-self-hosted-fonts - I want to get the font-weights down first before doing all the font performance stuff.

@machikoyasuda machikoyasuda marked this pull request as draft August 23, 2023 21:51
@machikoyasuda machikoyasuda requested a review from segacy1 August 23, 2023 21:51
@angela-tran
Copy link
Member

angela-tran commented Aug 23, 2023

@machikoyasuda Oh, interesting find. If there's all this additional nuance to it, then I'm fine with just loading from Google Fonts.

@segacy1
Copy link

segacy1 commented Aug 23, 2023

Weights look good to me! One q - are the weights for the text on the two buttons on home the same? Hard for me to tell visually because of the different color contrasts

@machikoyasuda
Copy link
Member Author

@segacy1 Yea the buttons are the same font-weight. Isn't it trippy that they look so different b/c of the colors?!?!

@segacy1
Copy link

segacy1 commented Aug 24, 2023

@machikoyasuda truly! In that case, this looks great to me. Thank you!

@machikoyasuda machikoyasuda mentioned this pull request Aug 24, 2023
Comment on lines 26 to 28
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&family=Source+Code+Pro:ital,wght@0,500;0,600;0,800;1,700&display=swap" rel="stylesheet">
Copy link
Member Author

@machikoyasuda machikoyasuda Aug 24, 2023

Choose a reason for hiding this comment

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

styles/base.css Outdated
Comment on lines 1 to 3
@import url("./fonts.css");

:root {
Copy link
Member Author

Choose a reason for hiding this comment

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

Preliminary performance comparison between externally-hosted and self-hosted fonts. Keep in mind, the externally-hosted fonts are request more font-weights now - 2 for Roboto and 4 for Source Code Pro (vs. 1 for Roboto and 2 for Source Code Pro).

This PR vs. Production, ran tests 2 times in a row:

image image

It's hard to get consistent Performance results (like look at how Bootstrap differs across tests) - but it seems like loading 4 weights of Source Code Pro via externally-hosted is still faster than loading 2 weights of self-hosted.

Technically, we're not using weight 800 for Source Code Pro yet, but I kept it in b/c it'll be used for the H1, which is used in the About pages coming up.

@machikoyasuda machikoyasuda force-pushed the feat/101-font-type-fix branch from b45666b to 8260e9e Compare August 24, 2023 20:15
@machikoyasuda machikoyasuda marked this pull request as ready for review August 28, 2023 16:53
@machikoyasuda
Copy link
Member Author

@angela-tran @thekaveman This is now ready for eng review. @segacy1 has approved it design-wise.

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Does this need to be rebased? I feel like some of these commits were from the Homepage PR?

@machikoyasuda machikoyasuda force-pushed the feat/101-font-type-fix branch from 8260e9e to 8134300 Compare August 28, 2023 17:50
@machikoyasuda
Copy link
Member Author

@thekaveman Rebased

@machikoyasuda machikoyasuda force-pushed the feat/101-font-type-fix branch from 8134300 to cddbbe1 Compare August 29, 2023 21:03
@machikoyasuda
Copy link
Member Author

Rebased main and took care of comments. Ready for re-review @thekaveman @angela-tran

@machikoyasuda machikoyasuda merged commit f704119 into main Aug 29, 2023
@machikoyasuda machikoyasuda deleted the feat/101-font-type-fix branch August 29, 2023 23:22
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.

Investigate font-weight discrepancy (and fix if needed) Update fonts and colors
4 participants