๐ fix : ํฐํธ ์ค๋ฅ ํด๊ฒฐ ๋ฐ ZEN-SERIF ์ ์ฉ#21
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMove typography utilities from a Tailwind plugin into Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Build as Tailwind/Vite
participant Config as tailwind.config.js
participant CSS as src/styles/globals.css
participant Page as src/pages/index.tsx
participant Browser as Browser
Dev->>Build: start dev server / run build
Build->>Config: load Tailwind config (fontFamily quoted, plugins: [])
Build->>CSS: read globals.css (Pretendard CDN, ZEN Serif @font-face, @layer utilities)
Note over Config,CSS: Typography utilities now live in globals.css\ninstead of the removed Tailwind plugin
Build->>Page: compile JSX with new typography classes
Build->>Dev: emit compiled CSS and JS
Dev->>Browser: open page
Browser->>CSS: request fonts & apply utilities
Browser->>Page: render updated index content with Pretendard/ZEN Serif styles
Estimated code review effort๐ฏ 3 (Moderate) | โฑ๏ธ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touchesโ Failed checks (1 warning)
โ Passed checks (2 passed)
โจ Finishing touches
๐งช Generate unit tests (beta)
Comment |
|
๐ท๏ธ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
|
๐ท๏ธ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
๐งน Nitpick comments (1)
tailwind.config.js (1)
103-103: Optional: Remove unnecessary quotes from Pretendard.While functional, Pretendard doesn't contain spaces and doesn't require quotes within the config string. For consistency with Tailwind best practices:
- sans: ['"Pretendard"', 'system-ui', 'sans-serif'], + sans: ['Pretendard', 'system-ui', 'sans-serif'],
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
โ Files ignored due to path filters (1)
public/fonts/ZEN-SERIF-TTF-Regular.woffis excluded by!**/*.woff
๐ Files selected for processing (2)
src/styles/globals.css(3 hunks)tailwind.config.js(1 hunks)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
๐ Additional comments (6)
tailwind.config.js (2)
104-104: LGTM!The quoted font name is correct for "JEN Serif" since it contains a space.
108-108: LGTM! Correctly removes the plugin.Removing the plugin and moving utilities to
@layer utilitiesin globals.css resolves the build-order issue that prevented fonts from being applied.src/styles/globals.css (4)
1-1: LGTM! CDN import for Pretendard.Using the CDN import is a good approach for Pretendard. The version (v1.3.9) is pinned, ensuring consistency.
67-70: LGTM! Consolidated base font styles.Combining html and body selectors for font-size and font-family is a clean approach for setting base styles.
100-100: Verify unusually large letter-spacing values.The letter-spacing values for some serif utilities are extremely large:
.text-display-serif: 0.6rem (6px at base font size).text-headline-sm-serif: 1.2rem (12px at base font size)These values will create very wide character spacing. Please confirm these are intentional design choices and not typos (e.g., should they be 0.06rem and 0.12rem?).
Also applies to: 103-103
81-105: LGTM! Moving utilities to @layer resolves the build-order issue.Using
@layer utilitiesto define typography classes is the correct solution to the build-order problem. This ensures Tailwind processes these utilities properly without the need for a plugin.
| @layer utilities { | ||
| /* Pretendard */ | ||
| .text-display-lg { font-size:5.7rem; font-weight:300; line-height:6.4rem; letter-spacing:-0.025rem; font-family: var(--font-sans); } | ||
| .text-display-md { font-size:5.2rem; font-weight:300; line-height:4.5rem; font-family: var(--font-sans); } |
There was a problem hiding this comment.
Fix line-height smaller than font-size.
The .text-display-md utility has a line-height (4.5rem) that is smaller than its font-size (5.2rem). This will cause text to overlap on multiple lines.
- .text-display-md { font-size:5.2rem; font-weight:300; line-height:4.5rem; font-family: var(--font-sans); }
+ .text-display-md { font-size:5.2rem; font-weight:300; line-height:5.6rem; font-family: var(--font-sans); }๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .text-display-md { font-size:5.2rem; font-weight:300; line-height:4.5rem; font-family: var(--font-sans); } | |
| .text-display-md { font-size:5.2rem; font-weight:300; line-height:5.6rem; font-family: var(--font-sans); } |
๐ค Prompt for AI Agents
In src/styles/globals.css around line 84, the .text-display-md rule sets
font-size: 5.2rem but line-height: 4.5rem (smaller than font-size) which can
cause overlapping; update the line-height to be equal to or greater than the
font-size (e.g., 5.2rem or a unitless value like 1.05โ1.2) or use a responsive
unitless multiplier to ensure adequate spacing across breakpoints.
| .text-headline-md { font-size:2.8rem; font-weight:200; line-height:3.2rem; font-family: var(--font-sans); } | ||
| .text-headline-sm { font-size:2.4rem; font-weight:200; line-height:3.2rem; font-family: var(--font-sans); } | ||
|
|
||
| .text-title-lg { font-size:2.2rem; font-weight:400; line-height:1.75rem; font-family: var(--font-sans); } |
There was a problem hiding this comment.
Fix line-height smaller than font-size.
The .text-title-lg utility has a line-height (1.75rem) significantly smaller than its font-size (2.2rem), causing severe text overlap on multiple lines.
- .text-title-lg { font-size:2.2rem; font-weight:400; line-height:1.75rem; font-family: var(--font-sans); }
+ .text-title-lg { font-size:2.2rem; font-weight:400; line-height:2.8rem; font-family: var(--font-sans); }๐ค Prompt for AI Agents
In src/styles/globals.css around line 91, the .text-title-lg rule sets
font-size: 2.2rem but line-height: 1.75rem which is smaller than the font size
and causes overlapping; update the rule so the line-height is at least equal to
or greater than the font-size (use a unitless value like 1.1โ1.3 or a rem value
>=2.2rem appropriate for your design, e.g., change line-height to a value such
as 2.5rem or 1.15) to prevent text overlap across multiple lines.
There was a problem hiding this comment.
Actionable comments posted: 1
โป๏ธ Duplicate comments (3)
src/styles/globals.css (3)
2-7: Verify font file and internal name match.Although a past comment indicated this was addressed, please confirm:
- The font file exists at
/fonts/ZEN-SERIF-TTF-Regular.woff2in your public directory- The internal font name in the WOFF2 file matches "ZEN Serif"
A mismatch could prevent the font from loading correctly.
Run this script to verify the font file:
#!/bin/bash # Verify the ZEN Serif font file exists if [ -f "public/fonts/ZEN-SERIF-TTF-Regular.woff2" ]; then echo "โ Font file exists" ls -lh public/fonts/ZEN-SERIF-TTF-Regular.woff2 else echo "โ Font file NOT found at public/fonts/ZEN-SERIF-TTF-Regular.woff2" echo "Searching for font files..." fd -e woff2 -e woff -e ttf . public/fonts/ 2>/dev/null || echo "fonts directory not found" fi
82-82: Fix line-height smaller than font-size.The
.text-display-mdutility hasline-height: 4.5remwhich is smaller thanfont-size: 5.2rem. This will cause text to overlap on multiple lines.Apply this diff to fix the issue:
- .text-display-md { font-size:5.2rem; font-weight:300; line-height:4.5rem; font-family: var(--font-sans); } + .text-display-md { font-size:5.2rem; font-weight:300; line-height:5.6rem; font-family: var(--font-sans); }
89-89: Fix line-height smaller than font-size.The
.text-title-lgutility hasline-height: 1.75remwhich is significantly smaller thanfont-size: 2.2rem. This will cause severe text overlap on multiple lines.Apply this diff to fix the issue:
- .text-title-lg { font-size:2.2rem; font-weight:400; line-height:1.75rem; font-family: var(--font-sans); } + .text-title-lg { font-size:2.2rem; font-weight:400; line-height:2.8rem; font-family: var(--font-sans); }
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
src/pages/index.tsx(1 hunks)src/styles/globals.css(3 hunks)tailwind.config.js(1 hunks)
๐ Additional comments (7)
tailwind.config.js (2)
103-104: LGTM! Font family declarations are correct.Quoting multi-word font names like "ZEN Serif" is necessary and proper. The consistent use of quotes for both families is clean.
108-108: Verify that typography utilities are available.The custom plugin has been removed and typography utilities have been moved to
src/styles/globals.cssunder@layer utilities. Ensure that all previously available utilities are now defined in the CSS file and that the build process correctly applies them.Based on the provided context, the utilities appear to be properly defined in
src/styles/globals.css, but you should confirm that the Tailwind build successfully processes the@layer utilitiesblock and that no utilities are missing in production builds.src/pages/index.tsx (1)
4-12: LGTM! Typography demo is clear.The example demonstrates both Pretendard and ZEN Serif fonts with appropriate utility classes. This serves as a good visual test for the font implementation.
src/styles/globals.css (4)
1-1: LGTM! CDN import is properly configured.Using the jsdelivr CDN with a pinned version (v1.3.9) ensures consistent font delivery and is a standard practice for Pretendard.
61-62: LGTM! CSS variable updated correctly.The
--font-serifvariable now references 'ZEN Serif', consistent with the @font-face declaration and the updated naming throughout the codebase.
65-68: LGTM! Base font configuration is correct.The
font-size: 62.5%on html,body is a standard practice that makes 1rem equal to 10px for easier calculation, and setting Pretendard as the default font-family is appropriate.
98-102: Verify large letter-spacing values are intentional.Some serif utilities use very large letter-spacing values (e.g.,
0.6remon line 98,1.2remon line 101). This is approximately 6-12px of spacing between characters, which creates a very loose, spaced-out appearance.If this is the intended design aesthetic for ZEN Serif, this is fine. However, if these values were meant to be smaller (e.g.,
0.06remor0.12rem), please adjust them.
| .text-label-md { font-size:1.2rem; font-weight:300; line-height:1.6rem; font-family: var(--font-sans); } | ||
| .text-label-sm { font-size:1.1rem; font-weight:300; line-height:1.6rem; font-family: var(--font-sans); } | ||
|
|
||
| /* JEN Serif */ |
There was a problem hiding this comment.
Update comment to match current font name.
The comment still says "JEN Serif" but the font has been renamed to "ZEN Serif" throughout the codebase. Please update for consistency.
Apply this diff:
- /* JEN Serif */
+ /* ZEN Serif */๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* JEN Serif */ | |
| /* ZEN Serif */ |
๐ค Prompt for AI Agents
In src/styles/globals.css around line 97, update the comment text which
currently reads "JEN Serif" to "ZEN Serif" so it matches the renamed font used
elsewhere in the codebase; simply replace the comment string only, preserving
whitespace and formatting.
2 participants
๐ฅ ์์ ๋ด์ฉ
๐ค ์ถํ ์์ ์ฌํญ
๐ย ์ด์
PR Point (To Reviewer)
ํฐํธ ์ค๋ฅ ํด๊ฒฐ
๋นํฉ์ค๋ฌ์ ์ต๋๋ค.. ๋น์ฅ ์ธํ ๊น์ง๋ง ํ์ ๋ ๋ฌธ์ ๊ฐ ์์๋๋ฐ ์ค๋๋ณด๋ ํ๋ฆฌํ ๋ค๋ ์กฐ์ฐจ๋ ์ ๋๋ก ์ ์ฉ์ด ์๋๋ ๋ชจ์ต์ ๋ณด๊ณ
์์ธ์ด ์ปค์คํ ํด๋ ํฐํธ ์ ํธ๋ฆฌํฐ๊ฐ tailwind.config.js ์ ๋ค์ด๊ฐ์ Tailwind Build ์์์ ๋ฐ๋ผ์ ๋ฌด์๊ฐ ๋ ๊ฑฐ ๊ฐ์ต๋๋ค. ์ด์ ์ ๋ง ์ ์ ์๋ํฉ๋๋ค ์ธํ ์ด ๋ฆ์ด์ ธ ๋ฏธ์ํฉ๋๋ค..
??: ์ด์ PR ์์ ๋๋ค๊ณ ํ ๊ฑด ๋ญ๊ฐ์??
ใด ์ด์ NotoSans์ NotoSerif๋ฅผ ์ ์ฉ๋๋ค๊ณ ์ฐฉ๊ฐํ ์ ๋์ ๋๋ค.
์ฌ์ฉ ๋ฐฉ๋ฒ(1์ฐจ ๋์์ธ์ธํ PR๊ณผ ๋์ผ)
๐ธย ํผ๊ทธ๋ง ์คํฌ๋ฆฐ์ท or ๊ธฐ๋ฅ GIF
์ ์ฉ์ฝ๋
Summary by CodeRabbit
New Features
UI Changes
Style