Skip to content
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

feat: add fonts to the system #100

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

huyenltnguyen
Copy link
Member

Checklist:

I noticed that Storybook doesn't display text in Lato font, even though we do have the font-family rule specified.

font-family: "Lato", Helvetica, Arial, sans-serif;

This is simply because we don't have the font files and font definitions in the repo.

This PR adds the following fonts to the library:

  • Lato
  • Noto Sans Arabic
  • Hack-ZeroSlash

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +90 to +93
fontFamily: {
sans: ["Lato", "sans-serif"],
mono: ["Hack-ZeroSlash", "monospace"],
},
Copy link
Member Author

Choose a reason for hiding this comment

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

These are currently unused, but will be needed when we enable Tailwind preflight (#61).

@huyenltnguyen huyenltnguyen marked this pull request as ready for review April 26, 2024 06:55
@huyenltnguyen huyenltnguyen requested a review from a team as a code owner April 26, 2024 06:55
@ahmaxed
Copy link
Member

ahmaxed commented Apr 27, 2024

Thanks for adding the fonts.

@ahmaxed ahmaxed merged commit 6328b69 into freeCodeCamp:main Apr 27, 2024
5 checks passed
@ojeytonwilliams
Copy link
Contributor

How does this work with the client's fonts? If we're telling tailwind the fonts live in assets/fonts, doesn't that conflict with the fact the client doesn't look in assets?

That said, the client already imports the fonts in two ways via

import latoBoldURL from '../../../static/fonts/lato/Lato-Light.woff';

and via

@font-face {
  font-family: 'Lato';
  src: url('../../../static/fonts/lato/Lato-Light.woff') format('woff');
  font-weight: 300;
  font-style: normal;
  font-display: fallback;
}

so it seems like we already have extra imports. @ahmaxed should one of these go?

@huyenltnguyen huyenltnguyen deleted the feat/add-fonts branch May 2, 2024 05:58
@huyenltnguyen
Copy link
Member Author

huyenltnguyen commented May 2, 2024

How does this work with the client's fonts? If we're telling tailwind the fonts live in assets/fonts, doesn't that conflict with the fact the client doesn't look in assets?

What I have in mind right now:

  • Add the CSS rules we need to base.css
  • Export base.css out for consumption
  • Include the fonts in the bundle
  • Replace bootstrap.min.css in /learn with base.css (which includes the font definitions)
  • Remove the font files and font definitions from /learn and consume the fonts exported from the component library

That is just the high level approach though. I still need to sort out the base.css content, build its final version, and try adding it to /learn to test.


so it seems like we already have extra imports.

I noticed this, too, but I assumed/thought the imports in client/src/components/layouts/default.tsx have something to do with Gatsby.

I'll defer this to Ahmad. (Though I could remove those imports as part of the CSS cleanup if they are redundant.)

@ojeytonwilliams
Copy link
Contributor

That is just the high level approach though

Sounds good! I was just a bit confused about how these fonts related to the /learn fonts.

have something to do with Gatsby.

Yeah, I think Gatsby bundles them (via webpack). I'd have to dig into it to understand exactly what's used and why, though.

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.

3 participants