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

fix(design-tokens): fixed font family #921

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

masoudmanson
Copy link
Contributor

@masoudmanson masoudmanson commented Jan 8, 2025

Summary

Design Tokens
Github issue: #920

https://czi-sci.slack.com/archives/C032S43KKFV/p1736185365071589

The SDS Tailwind configuration previously overrode the font-family to Inter, sans-serif, which does not fully meet the product requirements.

This PR refactors the default font-family to use the Inter font defined via Next.js Fonts by default, with a fallback to a list of system fonts for better compatibility and consistency.

@masoudmanson masoudmanson linked an issue Jan 8, 2025 that may be closed by this pull request
@masoudmanson masoudmanson requested a review from tihuan January 8, 2025 18:06
@masoudmanson masoudmanson self-assigned this Jan 8, 2025
@masoudmanson masoudmanson added the Bug Something isn't working label Jan 8, 2025
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks so much for the quick update, Masoud 👏 !!

Non blocking - It seems like the system fonts are specific to macos, wondering what the impact would be on Windows, Linux, etc. 🤔

@masoudmanson
Copy link
Contributor Author

LGTM! Thanks so much for the quick update, Masoud 👏 !!

Non blocking - It seems like the system fonts are specific to macos, wondering what the impact would be on Windows, Linux, etc. 🤔

Ahh, thanks for the quick review @tihuan 😍🙏🏻🥳
Actually, the font stack should work fine across Linux and Windows systems. Here’s a breakdown of the font-family list:

  • var(--font-inter): This sets the custom Inter font defined by Next.js.
  • Inter: Falls back to the standard Inter font for cases where the variable isn’t defined.
  • -apple-system, BlinkMacSystemFont: These cover macOS specific system fonts.
  • Segoe UI: The default system font for all Windows systems.
  • Roboto: The default system font for most Linux distributions.
  • The rest (Helvetica Neue, Helvetica, Arial, sans-serif) act as general fallbacks to ensure compatibility when none of the above fonts are available.

I also came across this article that mentions how platforms like GitHub and Medium use similar fallback fonts for their font stacks.

@tihuan
Copy link
Contributor

tihuan commented Jan 9, 2025

Ahh brilliant! Thanks so much for the rundown, Masoud 😁💡 That looks awesome!

Could you please add the article link to the code for future reference too please? Thanks a lot!

@masoudmanson
Copy link
Contributor Author

Ahh brilliant! Thanks so much for the rundown, Masoud 😁💡 That looks awesome!

Could you please add the article link to the code for future reference too please? Thanks a lot!

You’re welcome! 😍🥳🎉
Absolutely, I’ll add the article link to the code for reference. Thanks for pointing that out!

@masoudmanson masoudmanson merged commit 551de94 into main Jan 9, 2025
10 checks passed
@masoudmanson masoudmanson deleted the 920-fix-font-family-on-sds-design-tokens branch January 9, 2025 21:53
@@ -1,3 +1,16 @@
/* Font-family explanation:
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks perfect! Thanks for taking the time to do it 🤩 🏆 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix font-family on SDS design tokens
2 participants