feat: migrate typography to Urbanist + Manrope#10
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code Review — feat: migrate typography to Urbanist + ManropeOverall this is a clean, well-scoped migration. The approach is sound — offloading latin fonts to Issues1. Missing fallbacks on the explicit element rule ( /* Current */
p, li, label, button, input, textarea, select {
font-family: var(--font-urbanist); /* no fallback */
}
/* Should match the html/body rule */
p, li, label, button, input, textarea, select {
font-family: var(--font-urbanist), -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif;
}If 2. Redundant selectors —
/* Only form elements need the override */
button, input, textarea, select {
font-family: var(--font-urbanist), -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif;
}This also makes the locale overrides below less verbose (no need to list 3. Weight mismatch between font loaders
4. Tailwind config not updated (potential gap) If the project uses Tailwind's theme: {
extend: {
fontFamily: {
sans: ['var(--font-urbanist)', ...defaultTheme.fontFamily.sans],
heading: ['var(--font-manrope)', ...defaultTheme.fontFamily.sans],
},
},
},If no Tailwind font utilities are used and font is set entirely via CSS, this is a non-issue — just worth confirming. Minor / Nits
What's Good
Summary: Blocking on item #1 (missing fallbacks) and worth a quick check on #4 (Tailwind config). Items #2 and #3 are recommendations to improve consistency. The core approach is solid. |
There was a problem hiding this comment.
Pull request overview
Migrates the app’s typography system to use Urbanist for body/UI text and Manrope for headings via next/font/google, while keeping locale-specific non‑Latin fonts (Arabic/Chinese) loaded from Google Fonts and fixing the zh language selector mismatch (zh-Hans).
Changes:
- Update global CSS to use
--font-urbanist(body/UI) and--font-manrope(headings), with locale-specific overrides forarandzh-Hans. - Load Urbanist + Manrope via
next/font/googleand attach theirvariableclasses to the<html>element. - Only inject external Google Fonts
<link>tags for Arabic/Chinese locales (Noto Sans Arabic / Noto Sans SC).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| app/globals.css | Switches base and locale-specific font-family rules to Urbanist/Manrope variables and updates zh-Hans selector. |
| app/[locale]/layout.tsx | Adds next/font loaders for Urbanist/Manrope, applies variable classes on <html>, and conditionally loads Noto fonts for non‑Latin locales. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| html[lang="zh-Hans"] input, | ||
| html[lang="zh-Hans"] textarea, | ||
| html[lang="zh-Hans"] select { | ||
| font-family: "Noto Sans SC", var(--font-urbanist), sans-serif; |
There was a problem hiding this comment.
Same var() invalidation issue for the Simplified Chinese override: var(--font-urbanist) has no fallback, so if the variable isn't defined this whole font-family declaration is dropped and the zh-Hans override may not apply. Add a fallback argument to the var() (or remove it from this rule) to keep the declaration valid in all render paths.
| font-family: "Noto Sans SC", var(--font-urbanist), sans-serif; | |
| font-family: "Noto Sans SC", | |
| var(--font-urbanist, -apple-system, BlinkMacSystemFont, "Segoe UI"), | |
| sans-serif; |
| html[lang="zh-Hans"] h4, | ||
| html[lang="zh-Hans"] h5, | ||
| html[lang="zh-Hans"] h6 { | ||
| font-family: "Noto Sans SC", var(--font-manrope), sans-serif; |
There was a problem hiding this comment.
Same issue for zh-Hans headings: var(--font-manrope) without a fallback can invalidate the entire font-family declaration, preventing the locale-specific font from applying in contexts where the font variable class isn't present. Use var(--font-manrope, ...) (or remove the var() here) so the rule is always valid.
| font-family: "Noto Sans SC", var(--font-manrope), sans-serif; | |
| font-family: "Noto Sans SC", | |
| var(--font-manrope, -apple-system, BlinkMacSystemFont, "Segoe UI"), | |
| sans-serif; |
| html, | ||
| body { | ||
| font-family: var(--font-urbanist), -apple-system, BlinkMacSystemFont, | ||
| "Segoe UI", sans-serif; | ||
| } |
There was a problem hiding this comment.
font-family uses var(--font-urbanist) without a fallback value. If the urbanist.variable class is not present on <html> (e.g., global error / not-found pages that render their own <html>), the CSS variable is undefined and the whole font-family declaration becomes invalid, potentially falling back to the browser default font. Use a fallback in var() (or define default --font-urbanist/--font-manrope values) so typography degrades predictably outside the locale layout.
| h1, | ||
| h2, | ||
| h3, | ||
| h4, | ||
| h5, | ||
| h6 { | ||
| font-family: var(--font-manrope), -apple-system, BlinkMacSystemFont, | ||
| "Segoe UI", sans-serif; | ||
| } |
There was a problem hiding this comment.
Same issue as the body font: font-family: var(--font-manrope), ... becomes an invalid declaration if --font-manrope is undefined (e.g., pages not wrapped by the locale layout). Consider using var(--font-manrope, <system font stack>) or defining a default value for --font-manrope to avoid unexpected serif fallback on headings.
| input, | ||
| textarea, | ||
| select { | ||
| font-family: var(--font-urbanist); |
There was a problem hiding this comment.
font-family: var(--font-urbanist); will also be invalid anywhere --font-urbanist isn't set (same failure mode as the html, body rule). Adding a fallback to var() (or ensuring --font-urbanist is always defined globally) prevents these elements from dropping to browser-default fonts on pages that don't apply the font variable classes.
| font-family: var(--font-urbanist); | |
| font-family: var(--font-urbanist), -apple-system, BlinkMacSystemFont, | |
| "Segoe UI", sans-serif; |
| const urbanist = Urbanist({ | ||
| subsets: ["latin"], | ||
| weight: ["300", "400", "500", "600", "700", "800"], | ||
| variable: "--font-urbanist", | ||
| display: "swap", | ||
| }); |
There was a problem hiding this comment.
Both Urbanist/Manrope are configured to preload 6 weights (300–800). Each weight typically becomes a separate font file with next/font, so this can significantly increase download size. Consider limiting the weight arrays to only the weights actually used in the app (e.g., 400/500/600/700 if those are the only Tailwind weights in use) to reduce payload and improve LCP.
| html[lang="ar"] input, | ||
| html[lang="ar"] textarea, | ||
| html[lang="ar"] select { | ||
| font-family: "Noto Sans Arabic", var(--font-urbanist), sans-serif; |
There was a problem hiding this comment.
In this locale-specific font stack, var(--font-urbanist) has no fallback. If the CSS variable isn't defined, the entire font-family declaration becomes invalid (even though the first family is "Noto Sans Arabic"), so the locale override may not apply at all. Use var(--font-urbanist, <fallback stack>) here (or remove the var() from this rule) to keep the override working when the variable class isn't present.
| font-family: "Noto Sans Arabic", var(--font-urbanist), sans-serif; | |
| font-family: "Noto Sans Arabic", | |
| var(--font-urbanist, -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif); |
| html[lang="ar"] h4, | ||
| html[lang="ar"] h5, | ||
| html[lang="ar"] h6 { | ||
| font-family: "Noto Sans Arabic", var(--font-manrope), sans-serif; |
There was a problem hiding this comment.
Same var() invalidation issue in the Arabic heading override: if --font-manrope isn't defined, this font-family declaration becomes invalid and headings won't reliably use Noto Sans Arabic. Add a fallback argument to var(--font-manrope, ...) (or remove var() from this locale-specific rule) so the override always remains valid.
| font-family: "Noto Sans Arabic", var(--font-manrope), sans-serif; | |
| font-family: "Noto Sans Arabic", | |
| var(--font-manrope, -apple-system, BlinkMacSystemFont, "Segoe UI"), | |
| sans-serif; |
Move the system font stack INSIDE var(--font-*, ...) so declarations stay valid when the next/font variable class is absent from <html>. Without this, CSS invalidates the entire font-family property (IACVT) and text drops to browser default. Affected render path: app/global-error.tsx renders its own <html lang="en"> outside app/[locale]/layout.tsx, so --font-urbanist / --font-manrope are undefined there. Previously the error page silently rendered in Times New Roman instead of the system font stack. Addresses review feedback from Copilot (8 inline comments) and Claude.
Summary
next/font/google)h1–h6html[lang="zh"]override never matched — layout setslang="zh-Hans". New rules use[lang="zh-Hans"].CSS-only migration — no component (
.tsx) changes. Diff: 2 files.Test plan
pnpm devand load/en— DevTools → Computed → bodyfont-familyresolves toUrbanist(plusUrbanist Fallback)<h1>— resolves toManrope/ar— body renders in Noto Sans Arabic; RTL layout unaffected/zh— body renders in Noto Sans SC (verify this now actually applies, previously silently broken)/de— body renders in Urbanist (latin fallback for German)swapdisplay behavior visible