Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🏷️ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
WalkthroughAdds an SVG sprite system: new Icon React component, generated icon name list and index, a generation script, SVGO config, package scripts/deps, and a Next.js webpack hook to process icons directory with svg-sprite-loader; wires icons into the app and uses them on the homepage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant NPM as NPM Scripts
participant Gen as generate-icon-list.ts
participant FS as File System
participant Repo as Icons Repo
Dev->>NPM: run icons:gen / icons
NPM->>Gen: execute script
Gen->>FS: read src/shared/icons/source/*.svg
Gen->>Repo: write iconNames.ts (const tuple + type)
Gen->>Repo: write index.ts (side-effect SVG imports + export { Icon })
Note over Repo: Generated files used by app build
sequenceDiagram
autonumber
participant Next as Next.js Build
participant WP as Webpack
participant Ldr as svg-sprite-loader
participant App as _app.tsx (side-effect imports)
participant Icon as Icon Component
participant DOM as Sprite Sheet
App->>Next: import "@/shared/icons"
Next->>WP: compile
WP->>Ldr: process ICON_DIR SVGs -> generate sprite symbols
Ldr-->>DOM: emit sprite with <symbol id="icon-...">
Page->>Icon: render <Icon name="User" />
Icon->>DOM: <svg><use href="#icon-User"/></svg>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
🏷️ 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: 5
🧹 Nitpick comments (6)
package.json (1)
11-11: Consider adding tsx as a devDependency for consistency.The
icons:genscript usespnpm dlx tsx, which downloads and executes tsx on-the-fly. While this works, it can lead to inconsistent behavior across environments and slower execution. Consider addingtsxas a devDependency and updating the script topnpm tsx.Apply this diff:
"devDependencies": { "@eslint/eslintrc": "^3", "@tailwindcss/postcss": "^4", "@types/node": "^20", "@types/react": "^19", "@types/react-dom": "^19", "eslint": "^9", "eslint-config-next": "15.5.4", "svg-sprite-loader": "^6.0.11", "tailwindcss": "4.1.14", + "tsx": "^4.0.0", "typescript": "^5" }Then update the script:
- "icons:gen": "pnpm dlx tsx src/shared/icons/scripts/generate-icon-list.ts ", + "icons:gen": "tsx src/shared/icons/scripts/generate-icon-list.ts",next.config.ts (1)
10-11: Consider a more type-safe approach to finding the SVG rule.The current approach uses
@ts-ignoreand relies on runtime checks. Consider using a type guard or asserting the rule structure more safely.You could refactor to:
const svgRule = config.module.rules.find((rule): rule is { test: RegExp; exclude?: any } => { return ( typeof rule === 'object' && rule !== null && 'test' in rule && rule.test instanceof RegExp && rule.test.test('.svg') ); });src/pages/index.tsx (1)
6-15: Consider moving icon showcase to a dedicated demo page.While the Icon usage demonstrates the functionality, the production home page might not be the best place for this showcase. Consider creating a separate
/icons-demopage or a Storybook setup for icon previews.src/shared/icons/components/icon.tsx (3)
43-53: Remove redundant className from IconProps.The
classNameproperty is already included inReact.SVGProps<SVGSVGElement>viaReact.HTMLAttributes, making this declaration redundant.Apply this diff:
interface IconProps extends React.SVGProps<SVGSVGElement> { name: IconName; size?: number | string; width?: number | string; height?: number | string; color?: IconColor; - className?: string; rotate?: IconRotate; hasRotateAnimation?: boolean; ariaHidden?: boolean; }
79-84: Consider the inline-block default display.The
inline-blockdisplay is always applied, which works well for most use cases but may require overrides in flex or grid contexts whereinline-flexorblockmight be more appropriate.This is generally acceptable, but document that consumers can override via
classNameif needed, or consider making display configurable if you encounter frequent overrides in practice.
86-99: Consider aria-hidden default for semantic icons.The
ariaHiddendefault oftrueis appropriate for decorative icons but may not suit icons that convey semantic meaning without accompanying text. Consumers must remember to setariaHidden={false}and addaria-labelfor semantic icons.Consider either:
- Documenting this behavior clearly (when to override
ariaHidden)- Detecting when
aria-labeloraria-labelledbyis provided and automatically settingariaHidden={false}:const shouldHideFromAria = ariaHidden ?? !(rest['aria-label'] || rest['aria-labelledby']); // Then use: aria-hidden={shouldHideFromAria}Additionally, consider adding a development-mode warning if an icon is missing from the sprite (the
<use>element would silently fail):if (process.env.NODE_ENV === 'development') { // Validate icon exists after initial render React.useEffect(() => { const symbolExists = document.getElementById(`icon-${name}`); if (!symbolExists) { console.warn(`Icon "${name}" not found in sprite. Did you forget to run "pnpm icons"?`); } }, [name]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/shared/icons/source/CalendarBlank.svgis excluded by!**/*.svgsrc/shared/icons/source/Caret.svgis excluded by!**/*.svgsrc/shared/icons/source/ChatCircle.svgis excluded by!**/*.svgsrc/shared/icons/source/Check.svgis excluded by!**/*.svgsrc/shared/icons/source/CopySimple.svgis excluded by!**/*.svgsrc/shared/icons/source/Export.svgis excluded by!**/*.svgsrc/shared/icons/source/FadersHorizontal.svgis excluded by!**/*.svgsrc/shared/icons/source/HeartStraight.svgis excluded by!**/*.svgsrc/shared/icons/source/HouseSimple.svgis excluded by!**/*.svgsrc/shared/icons/source/MapPin.svgis excluded by!**/*.svgsrc/shared/icons/source/MapPin_.svgis excluded by!**/*.svgsrc/shared/icons/source/Save.svgis excluded by!**/*.svgsrc/shared/icons/source/User.svgis excluded by!**/*.svgsrc/shared/icons/source/backto.svgis excluded by!**/*.svgsrc/shared/icons/source/x.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
next.config.ts(1 hunks)package.json(2 hunks)src/pages/_app.tsx(1 hunks)src/pages/index.tsx(1 hunks)src/shared/icons/components/icon.tsx(1 hunks)src/shared/icons/iconNames.ts(1 hunks)src/shared/icons/index.ts(1 hunks)src/shared/icons/scripts/generate-icon-list.ts(1 hunks)svgo.config.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/shared/icons/scripts/generate-icon-list.ts (2)
eslint.config.mjs (2)
__filename(5-5)__dirname(6-6)src/shared/icons/iconNames.ts (1)
iconNames(2-18)
src/shared/icons/components/icon.tsx (2)
src/shared/icons/iconNames.ts (1)
IconName(19-19)src/shared/icons/index.ts (1)
Icon(18-18)
src/pages/index.tsx (2)
src/shared/icons/index.ts (1)
Icon(18-18)src/shared/icons/components/icon.tsx (1)
Icon(55-100)
next.config.ts (1)
eslint.config.mjs (1)
__dirname(6-6)
🔇 Additional comments (6)
src/pages/_app.tsx (1)
1-3: LGTM!The side-effect import of
'@/shared/icons'is necessary to ensure that all SVG sprites are loaded and registered with svg-sprite-loader before any components attempt to use them.src/shared/icons/index.ts (1)
1-18: LGTM!This auto-generated file correctly imports all SVG sources as side effects (required for svg-sprite-loader) and re-exports the Icon component. The structure aligns with the generator script's output.
src/shared/icons/iconNames.ts (1)
1-19: LGTM!This auto-generated file correctly exports a readonly tuple of icon names and derives a union type from it, providing type safety for the Icon component's
nameprop.svgo.config.mjs (1)
6-10: All SVG icons are stroke-only
Nofillattributes found in src/shared/icons/source; the SVGO config change is safe.src/shared/icons/components/icon.tsx (2)
1-2: LGTM!The imports are appropriate:
clsxfor composing class names andIconNamefor type safety.
67-77: LGTM!The size computation correctly prioritizes
width/heightoversizewith a sensible default of 20px. The rotation class mapping properly handles Tailwind's standard rotation classes and arbitrary value syntax for 270 degrees.
| const svgRule = config.module.rules.find( | ||
| // @ts-ignore | ||
| (rule) => rule.test && rule.test.test && rule.test.test('.svg'), | ||
| ); | ||
| if (svgRule) { | ||
| // @ts-ignore | ||
| svgRule.exclude = [...(svgRule.exclude || []), ICON_DIR]; | ||
| } |
There was a problem hiding this comment.
Safely handle the svgRule.exclude mutation.
The code assumes svgRule.exclude is either undefined or an array, but it could be a single value (string/RegExp) or have other configurations. Spreading a non-array will cause a runtime error.
Apply this diff to safely handle the exclude property:
if (svgRule) {
// @ts-ignore
- svgRule.exclude = [...(svgRule.exclude || []), ICON_DIR];
+ const currentExclude = Array.isArray(svgRule.exclude)
+ ? svgRule.exclude
+ : svgRule.exclude
+ ? [svgRule.exclude]
+ : [];
+ svgRule.exclude = [...currentExclude, ICON_DIR];
}📝 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.
| const svgRule = config.module.rules.find( | |
| // @ts-ignore | |
| (rule) => rule.test && rule.test.test && rule.test.test('.svg'), | |
| ); | |
| if (svgRule) { | |
| // @ts-ignore | |
| svgRule.exclude = [...(svgRule.exclude || []), ICON_DIR]; | |
| } | |
| const svgRule = config.module.rules.find( | |
| // @ts-ignore | |
| (rule) => rule.test && rule.test.test && rule.test.test('.svg'), | |
| ); | |
| if (svgRule) { | |
| // @ts-ignore | |
| const currentExclude = Array.isArray(svgRule.exclude) | |
| ? svgRule.exclude | |
| : svgRule.exclude | |
| ? [svgRule.exclude] | |
| : []; | |
| svgRule.exclude = [...currentExclude, ICON_DIR]; | |
| } |
🤖 Prompt for AI Agents
In next.config.ts around lines 9 to 16, the code unsafely spreads
svgRule.exclude assuming it's an array which can throw if exclude is a string,
RegExp, or other value; change the mutation to normalize exclude into an array
first (if Array.isArray(use it), else if exclude is defined wrap it in
[exclude], otherwise use []), then set svgRule.exclude = [...normalizedExclude,
ICON_DIR]; this preserves existing values and avoids spreading non-array types.
src/pages/index.tsx
Outdated
| <main className='flex flex-col items-center justify-center w-full flex-1 px-4 sm:px-20 text-center'> | ||
| <h1 className='text-4xl sm:text-6xl font-extrabold text-gray-900 mb-4'> | ||
| 초기 세팅 완료 | ||
| <Icon name='User' width={24} hanging={24} /> |
There was a problem hiding this comment.
Fix the typo: hanging should be height.
The prop name hanging is not valid for the Icon component. Based on the Icon component definition, it should be height.
Apply this diff:
- <Icon name='User' width={24} hanging={24} />
+ <Icon name='User' width={24} height={24} />📝 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.
| <Icon name='User' width={24} hanging={24} /> | |
| <Icon name='User' width={24} height={24} /> |
🤖 Prompt for AI Agents
In src/pages/index.tsx around line 10 the Icon component is given an invalid
prop name "hanging"; change that prop to "height" so the component receives the
correct prop (i.e., replace hanging={24} with height={24}) leaving other props
unchanged.
| const files = readdirSync(ICON_DIR, { withFileTypes: true }) | ||
| .filter((d) => d.isFile() && d.name.toLowerCase().endsWith('.svg')) | ||
| .map((d) => d.name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for missing or empty icon directory.
If the ICON_DIR doesn't exist or contains no SVG files, the script will proceed to generate empty or incorrect output files without warning.
Apply this diff to add validation:
+import { existsSync } from 'fs';
import { readdirSync, writeFileSync } from 'fs';
import { basename, dirname, join } from 'path';
import { fileURLToPath } from 'url';
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
const ICON_DIR = join(__dirname, '../source');
const NAMES_OUT = join(__dirname, '../iconNames.ts');
const INDEX_OUT = join(__dirname, '../index.ts');
+if (!existsSync(ICON_DIR)) {
+ console.error(`Error: Icon directory not found at ${ICON_DIR}`);
+ process.exit(1);
+}
+
const files = readdirSync(ICON_DIR, { withFileTypes: true })
.filter((d) => d.isFile() && d.name.toLowerCase().endsWith('.svg'))
.map((d) => d.name);
+
+if (files.length === 0) {
+ console.warn(`Warning: No SVG files found in ${ICON_DIR}`);
+}📝 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.
| const files = readdirSync(ICON_DIR, { withFileTypes: true }) | |
| .filter((d) => d.isFile() && d.name.toLowerCase().endsWith('.svg')) | |
| .map((d) => d.name); | |
| import { existsSync } from 'fs'; | |
| import { readdirSync, writeFileSync } from 'fs'; | |
| import { basename, dirname, join } from 'path'; | |
| import { fileURLToPath } from 'url'; | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = dirname(__filename); | |
| const ICON_DIR = join(__dirname, '../source'); | |
| const NAMES_OUT = join(__dirname, '../iconNames.ts'); | |
| const INDEX_OUT = join(__dirname, '../index.ts'); | |
| if (!existsSync(ICON_DIR)) { | |
| console.error(`Error: Icon directory not found at ${ICON_DIR}`); | |
| process.exit(1); | |
| } | |
| const files = readdirSync(ICON_DIR, { withFileTypes: true }) | |
| .filter((d) => d.isFile() && d.name.toLowerCase().endsWith('.svg')) | |
| .map((d) => d.name); | |
| if (files.length === 0) { | |
| console.warn(`Warning: No SVG files found in ${ICON_DIR}`); | |
| } |
🤖 Prompt for AI Agents
In src/shared/icons/scripts/generate-icon-list.ts around lines 12 to 14, the
code blindly reads ICON_DIR and proceeds even if the directory is missing or
contains no SVGs; update it to validate the directory and file list by wrapping
the readdirSync in a try/catch (or using existsSync/statSync) to produce a clear
error and exit if the directory cannot be read, then check that the filtered SVG
files array has length > 0 and if empty log/throw an informative message and
exit non-zero so downstream file generation is not run with empty input.
| ? 'rotate-180' | ||
| : rotate === 270 | ||
| ? 'rotate-[270deg]' | ||
| : ''; |
There was a problem hiding this comment.
이 부분 일관성을 위해 rotate-[270deg] → rotate-270 으로 통일 하는게 좋을거 같습니다!
There was a problem hiding this comment.
진짜 거짓말같은데 Tailwind에는 rotate-90, rotate-180는 있지만 rotate-270는 없다고해요... 그래서 저렇게 되었습니다
| hasRotateAnimation && 'transform transition-transform duration-200', | ||
| className, | ||
| ); | ||
|
|
There was a problem hiding this comment.
지금 코드에서는 hasRotateAnimation이 false 일때
회전(rotate)이 적용안될 수 도 있을 거 같아요
'inline-block', ' -> 'inline-block transform',
이렇게 항상 transform 클래스 포함시키도록 하면 어떨까요?
참고문헌
https://tailwindcss.com/docs/transform?utm_source=chatgpt.com
https://tailwindcss.com/docs/transition-property?utm_source=chatgpt.com
There was a problem hiding this comment.
전체적으로 아이콘 시스템을 자동화하고, 유지보수성 높은 구조로 정리한 점이 인상 깊네요
특히 svg-sprite-loader를 활용해 SVG를 스프라이트 형태로 관리하는 구조는, 추후 확장성 측면에서 좋은 선택이라고 생각합니다. generate-icon-list.ts로 자동화한 것도 실무에서 자주 쓰이는 접근인 것 같습니다!
🍎Good 포인트
- 아이콘 로테이션 처리
transform 클래스 항상 포함되게 하신 것도 좋네요!
회전값만 분기하고 나머지는 일관되게 유지돼서 깔끔한 것 같습니다😊 - 빌드 자동화 확장
pnpm icons:clean && pnpm icons:gen으로 자동 생성 파이프라인을 구성한 점이 훌륭합니다. 추가로, 이 작업을 CI/CD에 포함시켜서 커밋 전 자동 실행되게 해놔도 좋을 것 같네요:) - 참고 자료
혹시 이 구조 설계 시 참고하신 블로그나 오픈소스가 있다면 알려주세요🙇♂️
…/svg-sprite-setting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/shared/icons/components/icon.tsx (1)
95-95: Consider accessibility when icons are sole interactive content.Defaulting
ariaHiddentotrueworks well for decorative icons but may require explicit override when an icon is the only content inside buttons or links. Consider documenting this guideline for consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(2 hunks)src/pages/index.tsx(1 hunks)src/shared/icons/components/icon.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/pages/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/shared/icons/components/icon.tsx (2)
src/shared/icons/iconNames.ts (1)
IconName(19-19)src/shared/icons/index.ts (1)
Icon(18-18)
🔇 Additional comments (2)
src/shared/icons/components/icon.tsx (2)
67-68: LGTM! Clean implementation with good fallback patterns.The dimension fallback logic (width/height → size → 20), className composition with clsx (including the
transformclass as suggested in past reviews), and dimension-to-px conversion are all well implemented.Also applies to: 79-86, 91-92
94-94: Verify CSS variable format when resolving IconColor mismatch.When addressing the IconColor/CSS variable alignment flagged in the previous review, confirm whether the format should be
var(--${color})(current) orvar(--color-${color})depending on the final naming convention chosen for the CSS custom properties.Based on past review comments.
|
수고하셨습니다!! 비로 공동 컴포넌트 만들도록 하겠습니다! |
jjangminii
changed the title
3 participants
🔥 작업 내용
🤔 추후 작업 사항
🔗 이슈
PR Point (To Reviewer)
Icons svg sprite 방식 적용
디자인 시스템에서 icon을 추가할 때 svg icon을 어떻게 사용할지 고민이 필요했어요. 회의를 통해 기존에 svg 파일을 그대로 쓰지 않고 tsx와 같은 컴포넌트 파일로 변경해서 스타일 확장이나, 변경을 용이하게 하자고 결정했지만 svgr/cli 방법에는 다음과 같은 문제가 존재했어요.
많은 svg 파일 존재: 아이콘을 추가할 때마다 svg 파일 + 연관된 tsx 파일 다수 존재
네트워크 오버헤드: 다수의 아이콘이 각 HTTP 요청을 통해 성능 감소
이러한 이유를 통해 svgr/cli로 다수의 tsx를 생성하는 것이 아닌 하나의 svg 파일 안에서 다수의 svg를 정의하고 사용할(use) 아이콘만 선택하여 사용하는 sprite 기법을 선택하게 되었어요.
따라서 generater함수로 source에 있는 원본 svg 리스트(name)를 뽑아와 Icon 컴포넌트에 단 하나의 svg안에 를 통해 svg를 선택하도록 설계하였습니다.
다만 width,height 값이 svg파일에 들어가 있으면 ui가 깨지거나 원하는대로 조정하기가 힘들어 일일이 삭제해줘야하는 번거로움이 있어 svgr로 바꿀까하는 고민이있었습니다. 하지만 다음글에 보면 svgr도 일일이 삭제해줘야하는 번거로움은 동일해보여 svg sprite 방식으로 하기로 결정했습니다.
width,height 자동 삭제
width,height는 개발자가 삭제하거나 피그마에서 export할 때 방식 수정하는 방법이 있었어요. 피그마에서 플러그인 사용해서 설정하는 방식이 익숙하지않아 svgo를 사용해서 명령어로 이미지들의 width,height 가 자동으로 삭제되게하였습니다.
current color 자동 변경
밖에서 컬러 주입하여 사용하려 stroke의 컬러를 기본컬러-> current color로 자동 변환해주는게 1번과 동일하게 svgo 최적화를 적용해두었습니다. 컬러를 따로 입력하지않으면 디폴트로 검정색으로 세팅되고 컬러를 따로 주입하면 해당 컬러로 변합니다 여기서 컬러는 저희 토큰화 되어있는 이름으로만 주입 가능합니다-!
번외로 저희 아이콘에 fill은없고 stroke만 있길래 svgo 자동화로 fill은 none stroke만 currentColor로 변경되게 해뒀습니다 이부분에 대해 다른 생각 있으시면 알려주세요-!!
사용방법
현재까지 디자인 시스템에 있는 icon들은 다 추가해뒀습니다
(Icon의 name을 타입화 해둬서 name에 정의 되어 있는 이름이 아닐경우 오류가 발생해요)
명령어 입력하면 자동으로 수정되는 형식이라 source 파일에 원본 파일만 넣고 icons의 파일은 건들면..곤란해져요
📸 피그마 스크린샷 or 기능 GIF
Summary by CodeRabbit
New Features
Chores
Style