feat(avatar): avatar 컴포넌트 구현 #45
Conversation
kimdaeyeobbb
requested review from
hy57in,
developerjhp,
bae-sh,
noahluftyang,
froggy1014,
heeji289 and
synuns
as code owners
|
packages/avatar/package.json
Outdated
| "name": "@sipe-team/component", | ||
| "description": "component for Sipe Design System", |
|
|
||
| type Story = StoryObj<typeof meta>; | ||
|
|
||
| const testImage = faker.image.avatar(); |
packages/avatar/src/Avatar.test.tsx
Outdated
| test('기본 크기는 40px이다.', () => { | ||
| render(<Avatar src={testImage} />); | ||
| expect(screen.getByRole('img')).toHaveStyle({ width: '40px', height: '40px' }); | ||
| }); | ||
|
|
||
| test('원형 형태로 표시한다.', () => { | ||
| render(<Avatar src={testImage} shape="circle" />); | ||
| expect(screen.getByRole('img')).toHaveStyle({ borderRadius: '50%' }); | ||
| }); |
There was a problem hiding this comment.
다른 크기나 shape도 잘 되는지 확인하는 코드가 있으면 좋겠네요
packages/avatar/src/Avatar.test.tsx
Outdated
| const testImage = faker.image.avatar(); | ||
|
|
||
|
|
||
| test('이미지를 정상적으로 표시한다.', () => { |
There was a problem hiding this comment.
정상적이라는 단어가 조금 주관적인 표현이라고 생각해~ 이렇게 다듬어보면 어떨까?
=> 주입받은 이미지 주소를 src로 설정한다.
packages/avatar/src/Avatar.test.tsx
Outdated
|
|
||
| const img = screen.getByRole('img'); | ||
| expect(img).toHaveAttribute('src', testImage); | ||
| expect(img).toHaveAttribute('alt', '대체 텍스트'); |
There was a problem hiding this comment.
alt attribute를 확인하는건 이 테스트의 역할이 아니기도 하고, 바로 아래 테스트에서 검증하고 있기 때문에 이 구문은 지워도 될 것 같아!
packages/avatar/src/Avatar.tsx
Outdated
| const style = { | ||
| '--avatar-size': getAvatarSize(size), | ||
| '--avatar-shape': getAvatarShape(shape), | ||
| } as CSSProperties; |
There was a problem hiding this comment.
prop으로 주입 받은 style도 그대로 넘겨주도록 작성하면 어떨까?
|
토끼야 리뷰해줘 |
|
@developerjhp: 변경 사항을 검토하겠습니다. ✅ Actions performedReview triggered.
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/avatar/.storybook/main.tsOops! Something went wrong! :( ESLint: 9.17.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by Walkthrough이 변경 사항은 Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (9)
packages/avatar/vitest.setup.ts (1)
1-4: 테스트 설정이 적절히 구성되었습니다.DOM 테스트를 위한 필수적인 설정이 잘 되어있습니다. 다만, 타입스크립트 환경에서의 더 나은 개발 경험을 위해 다음과 같은 개선사항을 제안드립니다.
타입 정의를 위해 다음과 같이 수정해보세요:
+/// <reference types="@testing-library/jest-dom" /> import '@testing-library/jest-dom';이렇게 하면 IDE에서 matcher들의 타입 힌트를 더 정확하게 제공받을 수 있습니다.
packages/avatar/vitest.config.ts (2)
4-14: 테스트 환경 설정이 적절합니다만, 몇 가지 고려사항이 있습니다.환경 설정이 잘 되어 있으나, 다음 사항들을 고려해보시기 바랍니다:
happy-dom과jsdom중 선택한 이유를 문서화하면 좋을 것 같습니다- 글로벌 API 사용 시 타입 지원을 위한 추가 설정이 필요할 수 있습니다
다음과 같이 주석을 보강하는 것을 제안드립니다:
test: { - // 테스트를 실행할 환경 - // default: 'node' - // 브라우저 환경에서 테스트를 희망시 - 'jsdom' 또는 'happy-dom'으로 설정 + // 테스트를 실행할 환경 + // default: 'node' + // happy-dom 선택 이유: jsdom보다 가벼우며 컴포넌트 테스트에 충분한 기능 제공 environment: 'happy-dom',
23-26: 플러그인 관련 추가 고려사항이 있습니다.현재 플러그인이 비어있는데, 다음 플러그인들의 추가를 고려해보시면 좋을 것 같습니다:
vite-tsconfig-paths: TypeScript 경로 별칭 지원@vitejs/plugin-react: React 관련 기능 지원다음과 같은 변경을 제안드립니다:
// 환경별로 설정해주어야하는 추가 기능을 플러그인으로 주입 가능 // ex) vite-tsconfig-paths - plugins: [], + plugins: [ + // TypeScript 경로 별칭을 지원하기 위한 플러그인 + tsconfigPaths(), + // React 지원을 위한 플러그인 + react(), + ],packages/avatar/src/Avatar.test.tsx (1)
8-14: 테스트 설명을 더 명확하게 개선해 주세요."정상적으로"라는 표현이 다소 모호합니다. 테스트의 목적을 더 구체적으로 설명하는 것이 좋겠습니다.
다음과 같이 변경을 제안합니다:
-test('이미지를 정상적으로 표시한다.', () => { +test('주입받은 이미지 주소를 src 속성에 설정한다.', () => {packages/avatar/src/Avatar.stories.tsx (2)
19-24: Basic 스토리의 이미지 소스를 일관되게 변경해 주세요.다른 스토리들은
faker를 사용하는데 Basic 스토리만 하드코딩된 URL을 사용하고 있습니다.다음과 같이 변경을 제안합니다:
export const Basic: Story = { args: { - src: 'https://randomuser.me/api/portraits/men/1.jpg', + src: testImage, alt: '대체 텍스트', }, };
29-34: alt 텍스트 포맷을 일관되게 맞춰주세요.크기를 나타내는 alt 텍스트가 "XSmall"과 "small" 등 일관성이 없습니다.
다음과 같이 변경을 제안합니다:
- <Avatar size="xs" src={testImage} alt="XSmall" /> - <Avatar size="sm" src={testImage} alt="small" /> - <Avatar size="md" src={testImage} alt="medium" /> - <Avatar size="lg" src={testImage} alt="large" /> - <Avatar size="xl" src={testImage} alt="XLarge" /> + <Avatar size="xs" src={testImage} alt="아주 작은 크기" /> + <Avatar size="sm" src={testImage} alt="작은 크기" /> + <Avatar size="md" src={testImage} alt="중간 크기" /> + <Avatar size="lg" src={testImage} alt="큰 크기" /> + <Avatar size="xl" src={testImage} alt="아주 큰 크기" />packages/avatar/package.json (1)
24-25: 워크스페이스 의존성의 버전 지정자가 일관적이지 않습니다@sipe-team/typography는 "*"를, @sipe-team/tokens는 "^"를 사용하고 있습니다. 워크스페이스 패키지들은 동일한 버전 지정자를 사용하는 것이 좋습니다.
- "@sipe-team/typography": "workspace:*", - "@sipe-team/tokens": "workspace:^", + "@sipe-team/typography": "workspace:^", + "@sipe-team/tokens": "workspace:^",packages/avatar/src/Avatar.tsx (2)
44-46: 에러 처리 로직을 개선할 수 있습니다현재 에러 처리가 fallback이 있을 때만 동작합니다. fallback이 없는 경우에도 적절한 처리가 필요할 수 있습니다.
onError={(e) => { - if (fallback) e.currentTarget.src = fallback; + if (fallback) { + e.currentTarget.src = fallback; + } else { + e.currentTarget.style.display = 'none'; + } }}
9-16: props에 대한 JSDoc 문서화가 필요합니다각 prop의 용도와 가능한 값들에 대한 설명을 JSDoc으로 추가하면 좋겠습니다.
+/** + * Avatar 컴포넌트의 props 인터페이스 + */ export interface AvatarProps extends ComponentProps<'div'> { + /** 다른 컴포넌트로 렌더링할지 여부 */ asChild?: boolean; + /** 아바타 이미지 소스 URL */ src?: string; + /** 이미지 대체 텍스트 */ alt?: string; + /** 아바타 크기 ('xs' | 'sm' | 'md' | 'lg' | 'xl') */ size?: AvatarSize; + /** 아바타 모양 ('circle' | 'rounded' | 'square') */ shape?: AvatarShape; + /** 이미지 로드 실패시 표시할 대체 이미지 URL */ fallback?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/avatar/.storybook/main.ts(1 hunks)packages/avatar/.storybook/preview.ts(1 hunks)packages/avatar/package.json(1 hunks)packages/avatar/src/Avatar.module.css(1 hunks)packages/avatar/src/Avatar.stories.tsx(1 hunks)packages/avatar/src/Avatar.test.tsx(1 hunks)packages/avatar/src/Avatar.tsx(1 hunks)packages/avatar/src/index.ts(1 hunks)packages/avatar/tsconfig.json(1 hunks)packages/avatar/tsup.config.ts(1 hunks)packages/avatar/vitest.config.ts(1 hunks)packages/avatar/vitest.setup.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- packages/avatar/tsconfig.json
- packages/avatar/src/index.ts
- packages/avatar/tsup.config.ts
- packages/avatar/src/Avatar.module.css
- packages/avatar/.storybook/preview.ts
🔇 Additional comments (5)
packages/avatar/vitest.config.ts (2)
1-3: LGTM! 기본 구성이 잘 되어있습니다.
vitest/config에서 defineConfig를 가져와서 적절하게 사용하고 있습니다.
15-21: 설정 파일의 존재 여부를 확인해주세요.
setupFiles에 지정된 vitest.setup.ts 파일이 올바른 위치에 존재하는지 확인이 필요합니다.
✅ Verification successful
설정 파일이 정상적으로 존재합니다.
packages/avatar/vitest.setup.ts 파일이 지정된 위치에서 확인되었습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of vitest.setup.ts file
# Expected: File should exist in the same directory as vitest.config.ts
# Find the setup file
fd -p "packages/avatar/vitest.setup.ts"
Length of output: 71
packages/avatar/.storybook/main.ts (1)
1-16: 설정이 잘 구성되어 있습니다! ✨
필수적인 Storybook 애드온들이 모두 포함되어 있고, TypeScript 타입 체크도 적절히 적용되어 있습니다.
packages/avatar/package.json (1)
2-2:
패키지 이름이 컴포넌트의 특성을 반영하지 않습니다
현재 패키지 이름이 "@sipe-team/component"로 되어있는데, 이는 너무 일반적입니다. Avatar 컴포넌트를 위한 패키지이므로 "@sipe-team/avatar"로 변경하는 것이 좋겠습니다.
packages/avatar/src/Avatar.tsx (1)
33-36: 🛠️ Refactor suggestion
스타일 props 처리 개선이 필요합니다
사용자가 전달한 style prop과 내부 스타일을 병합하도록 수정하면 좋겠습니다.
const style = {
+ ...props.style,
'--avatar-size': getAvatarSize(size),
'--avatar-shape': getAvatarShape(shape),
} as CSSProperties;
packages/avatar/src/Avatar.test.tsx
Outdated
| test('기본 크기는 40px이다.', () => { | ||
| render(<Avatar src={testImage} />); | ||
| expect(screen.getByRole('img')).toHaveStyle({ width: '40px', height: '40px' }); | ||
| }); | ||
|
|
||
| test('원형 형태로 표시한다.', () => { | ||
| render(<Avatar src={testImage} shape="circle" />); | ||
| expect(screen.getByRole('img')).toHaveStyle({ borderRadius: '50%' }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 케이스를 보완해 주세요.
크기와 모양에 대한 테스트가 기본값만 확인하고 있습니다. 다양한 크기와 모양 조합에 대한 테스트 케이스를 추가하면 좋겠습니다.
다음과 같은 테스트 케이스 추가를 제안합니다:
test.each([
['xs', '24px'],
['sm', '32px'],
['md', '40px'],
['lg', '48px'],
['xl', '56px'],
])('size가 %s일 때 %s 크기로 렌더링된다', (size, expected) => {
render(<Avatar src={testImage} size={size} />);
expect(screen.getByRole('img')).toHaveStyle({
width: expected,
height: expected
});
});
test.each(['circle', 'rounded', 'square'])
('shape가 %s일 때 올바른 스타일이 적용된다', (shape) => {
render(<Avatar src={testImage} shape={shape} />);
const img = screen.getByRole('img');
if (shape === 'circle') {
expect(img).toHaveStyle({ borderRadius: '50%' });
} else if (shape === 'rounded') {
expect(img).toHaveStyle({ borderRadius: '8px' });
} else {
expect(img).toHaveStyle({ borderRadius: '0px' });
}
});
packages/avatar/package.json
Outdated
| "@storybook/test": "catalog:", | ||
| "@types/react": "^18.3.12", | ||
| "@testing-library/jest-dom": "^6.6.3", | ||
| "@testing-library/react": "^16.0.1", |
There was a problem hiding this comment.
@testing-library/react 버전이 최신이 아닙니다
현재 "@testing-library/react"의 버전이 ^16.0.1로 설정되어 있습니다. 최신 안정 버전을 사용하는 것이 좋습니다.
- "@testing-library/react": "^16.0.1",
+ "@testing-library/react": "^14.1.2",📝 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.
| "@testing-library/react": "^16.0.1", | |
| "@testing-library/react": "^14.1.2", |
packages/avatar/src/Avatar.tsx
Outdated
| fallback, | ||
| ...props | ||
| }: AvatarProps, | ||
| ref: ForwardedRef<any> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ref 타입이 너무 광범위합니다
ForwardedRef<any> 대신 더 구체적인 타입을 사용하는 것이 좋습니다. HTMLDivElement 또는 HTMLElement를 사용하여 타입 안전성을 높일 수 있습니다.
- ref: ForwardedRef<any>
+ ref: ForwardedRef<HTMLDivElement>📝 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.
| ref: ForwardedRef<any> | |
| ref: ForwardedRef<HTMLDivElement> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/avatar/src/Avatar.tsx (4)
11-12: 타입 정의에 JSDoc 문서를 추가하면 좋겠습니다타입 정의에 각 값이 어떤 의미를 가지는지 설명하는 JSDoc 문서를 추가하면 컴포넌트 사용자가 더 쉽게 이해할 수 있습니다.
+/** Avatar 컴포넌트의 크기를 정의합니다. */ type AvatarSize = "xs" | "sm" | "md" | "lg" | "xl"; +/** Avatar 컴포넌트의 모서리 형태를 정의합니다. */ type AvatarShape = "circle" | "rounded" | "square";
14-21: 프로퍼티 설명을 추가해주세요각 프로퍼티의 용도와 기능을 설명하는 JSDoc 문서를 추가하면 좋겠습니다.
export interface AvatarProps extends ComponentProps<"div"> { + /** Radix UI의 Slot을 사용하여 다른 엘리먼트로 렌더링할지 여부 */ asChild?: boolean; + /** 아바타 이미지의 URL */ src?: string; + /** 이미지의 대체 텍스트 */ alt?: string; + /** 아바타의 크기 */ size?: AvatarSize; + /** 아바타의 모서리 형태 */ shape?: AvatarShape; + /** 이미지 로드 실패시 표시할 대체 이미지 URL */ fallback?: string; }
54-56: 이미지 에러 처리 로직을 개선해주세요현재 구현은 fallback이 있을 때만 에러를 처리합니다. 에러 발생 시 로깅이나 사용자 피드백을 추가하는 것이 좋겠습니다.
onError={(e) => { + console.warn('Avatar 이미지 로드 실패:', src); if (fallback) e.currentTarget.src = fallback; + else { + // fallback이 없을 경우 alt 텍스트를 보여주기 위해 이미지를 제거 + e.currentTarget.style.display = 'none'; + } }}
66-92: 유틸리티 함수를 별도 파일로 분리하는 것이 좋겠습니다
getAvatarSize와getAvatarShape함수를 별도의 유틸리티 파일로 분리하면 코드 구조가 더 깔끔해지고 재사용성이 향상됩니다.새로운 파일
utils.ts를 생성하여 다음과 같이 분리하는 것을 제안합니다:// utils.ts export function getAvatarSize(size: AvatarSize) { // ... 현재 구현 ... } export function getAvatarShape(shape: AvatarShape) { // ... 현재 구현 ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/avatar/src/Avatar.test.tsx(1 hunks)packages/avatar/src/Avatar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/avatar/src/Avatar.test.tsx
🔇 Additional comments (1)
packages/avatar/src/Avatar.tsx (1)
34-34: ref 타입이 너무 광범위합니다
ForwardedRef<any>대신 더 구체적인 타입을 사용하는 것이 좋습니다.- ref: ForwardedRef<any> + ref: ForwardedRef<HTMLDivElement>
packages/avatar/src/Avatar.tsx
Outdated
| const style = { | ||
| "--avatar-size": size, | ||
| "--avatar-shape": shape, | ||
| } as CSSProperties; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
style 프로퍼티 전달 방식 개선이 필요합니다
현재 구현은 커스텀 CSS 변수만 설정하고 있습니다. 사용자가 전달한 style 프로퍼티와 병합하여 처리해야 합니다.
const style = {
+ ...props.style,
"--avatar-size": size,
"--avatar-shape": shape,
} as CSSProperties;📝 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 style = { | |
| "--avatar-size": size, | |
| "--avatar-shape": shape, | |
| } as CSSProperties; | |
| const style = { | |
| ...props.style, | |
| "--avatar-size": size, | |
| "--avatar-shape": shape, | |
| } as CSSProperties; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/avatar/src/Avatar.tsx (2)
11-12: 타입 정의에 JSDoc 문서화 추가 필요
AvatarSize와AvatarShape타입에 대한 JSDoc 문서화를 추가하여 각 옵션값의 의미와 사용 사례를 명확히 설명하는 것이 좋습니다.+/** + * Avatar 컴포넌트의 크기 옵션 + * @type {AvatarSize} + * - xs: 24px + * - sm: 32px + * - md: 40px (기본값) + * - lg: 70px + * - xl: 96px + */ type AvatarSize = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; + +/** + * Avatar 컴포넌트의 모양 옵션 + * @type {AvatarShape} + * - circle: 원형 (50% border-radius) + * - rounded: 둥근 모서리 (4px border-radius) + * - square: 정사각형 (0px border-radius) + */ type AvatarShape = 'circle' | 'rounded' | 'square';
66-92: 상수값 분리 및 오류 처리 개선 필요크기와 모양에 대한 상수값을 별도의 상수/테마 파일로 분리하고, 잘못된 값이 전달되었을 때의 오류 처리를 추가하는 것이 좋습니다.
다음과 같이 개선할 수 있습니다:
- 상수 파일 생성 (
constants.ts):export const AVATAR_SIZES = { xs: '24px', sm: '32px', md: '40px', lg: '70px', xl: '96px', } as const; export const AVATAR_SHAPES = { rounded: '4px', square: '0px', circle: '50%', } as const;
- 함수 개선:
import { AVATAR_SIZES, AVATAR_SHAPES } from './constants'; function getAvatarSize(size: AvatarSize): string { if (!(size in AVATAR_SIZES)) { console.warn(`Invalid avatar size: ${size}. Falling back to 'md'`); return AVATAR_SIZES.md; } return AVATAR_SIZES[size]; } function getAvatarShape(shape: AvatarShape): string { if (!(shape in AVATAR_SHAPES)) { console.warn(`Invalid avatar shape: ${shape}. Falling back to 'circle'`); return AVATAR_SHAPES.circle; } return AVATAR_SHAPES[shape]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/avatar/package.json(1 hunks)packages/avatar/src/Avatar.test.tsx(1 hunks)packages/avatar/src/Avatar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/avatar/src/Avatar.test.tsx
- packages/avatar/package.json
🔇 Additional comments (2)
packages/avatar/src/Avatar.tsx (2)
34-34: ref 타입이 너무 광범위합니다
ForwardedRef<any>대신 더 구체적인 타입을 사용하는 것이 좋습니다. HTMLDivElement 또는 HTMLElement를 사용하여 타입 안전성을 높일 수 있습니다.
38-41: style 프로퍼티 전달 방식 개선이 필요합니다현재 구현은 커스텀 CSS 변수만 설정하고 있습니다. 사용자가 전달한 style 프로퍼티와 병합하여 처리해야 합니다.
| {src ? ( | ||
| <img | ||
| src={src} | ||
| alt={alt} | ||
| onError={(e) => { | ||
| if (fallback) e.currentTarget.src = fallback; | ||
| }} | ||
| className={styles.image} | ||
| /> | ||
| ) : ( | ||
| <span className={styles.fallback}>{alt || fallback}</span> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 개선이 필요합니다
이미지 로딩 실패 시 스크린 리더 사용자에게 적절한 피드백을 제공해야 합니다.
{src ? (
<img
src={src}
alt={alt}
+ aria-invalid={false}
onError={(e) => {
- if (fallback) e.currentTarget.src = fallback;
+ if (fallback) {
+ e.currentTarget.src = fallback;
+ e.currentTarget.setAttribute('aria-invalid', 'true');
+ }
}}
className={styles.image}
/>
) : (
- <span className={styles.fallback}>{alt || fallback}</span>
+ <span className={styles.fallback} role="img" aria-label={alt || fallback}>
+ {alt || fallback}
+ </span>
)}📝 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.
| {src ? ( | |
| <img | |
| src={src} | |
| alt={alt} | |
| onError={(e) => { | |
| if (fallback) e.currentTarget.src = fallback; | |
| }} | |
| className={styles.image} | |
| /> | |
| ) : ( | |
| <span className={styles.fallback}>{alt || fallback}</span> | |
| )} | |
| {src ? ( | |
| <img | |
| src={src} | |
| alt={alt} | |
| aria-invalid={false} | |
| onError={(e) => { | |
| if (fallback) { | |
| e.currentTarget.src = fallback; | |
| e.currentTarget.setAttribute('aria-invalid', 'true'); | |
| } | |
| }} | |
| className={styles.image} | |
| /> | |
| ) : ( | |
| <span className={styles.fallback} role="img" aria-label={alt || fallback}> | |
| {alt || fallback} | |
| </span> | |
| )} |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/avatar/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: chromatic
🔇 Additional comments (6)
packages/avatar/package.json (6)
13-21: 스크립트 구성이 잘 되어있습니다!빌드, 테스트, 타입 체크 등 필요한 스크립트들이 모두 잘 구성되어 있습니다.
22-27: 의존성 설정이 적절합니다!내부 패키지는 workspace 의존성을 사용하고, 외부 패키지는 적절한 버전으로 고정되어 있습니다.
40-40: @testing-library/react 버전이 최신이 아닙니다현재 "@testing-library/react"의 버전이 ^16.0.1로 설정되어 있습니다. 최신 안정 버전을 사용하는 것이 좋습니다.
- "@testing-library/react": "^16.0.1", + "@testing-library/react": "^14.1.2",
48-50: Peer 의존성이 적절하게 설정되었습니다!React 18 이상을 요구하는 것이 적절합니다.
51-66: 배포 설정이 잘 되어있습니다!ESM과 CommonJS 모듈 형식을 모두 지원하도록 잘 구성되어 있습니다.
67-67: 사이드 이펙트 설정이 적절합니다!UI 컴포넌트 패키지에 적합한 설정입니다.
packages/avatar/package.json
Outdated
| "name": "@sipe-team/component", | ||
| "description": "Avatar component for Sipe Design System", | ||
| "version": "0.0.1", | ||
| "license": "MIT", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/sipe-team/3-1_sds" | ||
| }, |
There was a problem hiding this comment.
패키지 이름과 저장소 URL을 수정해주세요
- 패키지 이름이 너무 일반적입니다. Avatar 컴포넌트를 위한 패키지이므로 더 구체적인 이름을 사용하는 것이 좋습니다.
- 저장소 URL이 현재 PR의 저장소와 일치하지 않습니다.
- "name": "@sipe-team/component",
+ "name": "@sipe-team/avatar",
...
- "url": "https://github.com/sipe-team/3-1_sds"
+ "url": "https://github.com/sipe-team/3-2_side"📝 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.
| "name": "@sipe-team/component", | |
| "description": "Avatar component for Sipe Design System", | |
| "version": "0.0.1", | |
| "license": "MIT", | |
| "repository": { | |
| "type": "git", | |
| "url": "https://github.com/sipe-team/3-1_sds" | |
| }, | |
| "name": "@sipe-team/avatar", | |
| "description": "Avatar component for Sipe Design System", | |
| "version": "0.0.1", | |
| "license": "MIT", | |
| "repository": { | |
| "type": "git", | |
| "url": "https://github.com/sipe-team/3-2_side" | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/avatar/package.json (2)
2-2:⚠️ Potential issue패키지 이름과 저장소 URL이 올바르지 않습니다
- 패키지 이름이 Avatar 컴포넌트의 목적과 맞지 않습니다
- 저장소 URL이 현재 PR의 저장소와 다릅니다
- "name": "@sipe-team/component", + "name": "@sipe-team/avatar", ... - "url": "https://github.com/sipe-team/3-1_sds" + "url": "https://github.com/sipe-team/3-2_side"Also applies to: 8-8
🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
42-42:⚠️ Potential issue@testing-library/react 버전을 업데이트해주세요
현재 16.0.1 버전을 사용 중이나, 최신 안정 버전은 14.1.2입니다.
- "@testing-library/react": "^16.0.1", + "@testing-library/react": "^14.1.2",🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🧹 Nitpick comments (1)
packages/avatar/package.json (1)
15-23: 코드 포맷팅 스크립트가 누락되었습니다Biome을 사용하고 있지만 format 스크립트가 정의되어 있지 않습니다. 일관된 코드 스타일을 위해 format 스크립트를 추가해주세요.
"scripts": { "build": "tsup", "build:storybook": "storybook build", "dev:storybook": "storybook dev -p 6006", "lint": "biome lint .", + "format": "biome format .", "test": "vitest", "typecheck": "tsc", "prepack": "pnpm run build" }🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/avatar/package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Chromatic
packages/avatar/package.json
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
packages/avatar/package.json
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
packages/avatar/package.json
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🔇 Additional comments (2)
packages/avatar/package.json (2)
53-68: 배포 설정이 잘 구성되어 있습니다ESM과 CJS 모듈 형식을 모두 지원하도록 올바르게 설정되어 있습니다.
🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
69-69: 트리 쉐이킹 최적화가 올바르게 설정되어 있습니다
sideEffects: false설정으로 번들 크기 최적화가 가능합니다.🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
packages/avatar/package.json
Outdated
| "dependencies": { | ||
| "@radix-ui/react-slot": "^1.1.0", | ||
| "@sipe-team/typography": "workspace:^", | ||
| "@sipe-team/tokens": "workspace:*", | ||
| "clsx": "^2.1.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@biomejs/biome": "catalog:", | ||
| "@faker-js/faker": "^9.2.0", | ||
| "@storybook/addon-essentials": "catalog:", | ||
| "@storybook/addon-interactions": "catalog:", | ||
| "@storybook/addon-links": "catalog:", | ||
| "@storybook/blocks": "catalog:", | ||
| "@storybook/react": "catalog:", | ||
| "@storybook/react-vite": "catalog:", | ||
| "@storybook/test": "catalog:", | ||
| "@types/react": "^18.3.12", | ||
| "@testing-library/jest-dom": "^6.6.3", | ||
| "@testing-library/react": "^16.0.1", | ||
| "happy-dom": "catalog:", | ||
| "react": "^18.3.1", | ||
| "storybook": "catalog:", | ||
| "tsup": "catalog:", | ||
| "typescript": "catalog:", | ||
| "vitest": "catalog:" | ||
| }, | ||
| "peerDependencies": { | ||
| "react": ">= 18" | ||
| }, |
There was a problem hiding this comment.
pnpm-lock.yaml 파일을 업데이트해주세요
CI 파이프라인에서 package.json과 lockfile이 일치하지 않는다는 오류가 발생했습니다. 다음 명령어를 실행하여 lockfile을 업데이트해주세요:
pnpm install🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
| "url": "https://github.com/sipe-team/3-1_sds" | ||
| }, | ||
| "type": "module", | ||
| "exports": "./src/index.ts", |
There was a problem hiding this comment.
exports 필드가 소스 파일을 직접 가리키고 있습니다
배포된 패키지는 컴파일된 파일을 가리켜야 합니다. src 대신 dist 디렉토리를 참조하도록 수정해주세요.
- "exports": "./src/index.ts",
+ "exports": {
+ ".": {
+ "import": "./dist/index.js",
+ "require": "./dist/index.cjs",
+ "types": "./dist/index.d.ts"
+ }
+ }📝 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.
| "exports": "./src/index.ts", | |
| "exports": { | |
| ".": { | |
| "import": "./dist/index.js", | |
| "require": "./dist/index.cjs", | |
| "types": "./dist/index.d.ts" | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Chromatic
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: CI
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
🪛 GitHub Actions: Run tests and upload coverage
[error] Package dependency specifications in package.json do not match the lockfile (pnpm-lock.yaml). Lock file needs to be updated
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/avatar/package.json (1)
26-27: 워크스페이스 의존성 버전 지정자를 통일해주세요워크스페이스 의존성의 버전 지정자가 일관되지 않습니다 (
^vs*).다음과 같이 통일해주세요:
- "@sipe-team/typography": "workspace:^", - "@sipe-team/tokens": "workspace:*", + "@sipe-team/typography": "workspace:*", + "@sipe-team/tokens": "workspace:*",packages/avatar/src/Avatar.tsx (1)
85-111: 크기와 모양 관련 상수를 분리하여 관리하면 좋을 것 같습니다크기와 모양 값을 상수로 분리하면 유지보수성이 향상되고, 재사용성이 높아집니다.
다음과 같이 수정해주세요:
+const AVATAR_SIZES = { + xs: "24px", + sm: "32px", + md: "40px", + lg: "70px", + xl: "96px", +} as const; + +const AVATAR_SHAPES = { + rounded: "4px", + square: "0px", + circle: "50%", +} as const; + function getAvatarSize(size: AvatarSize) { - switch (size) { - case "xs": - return "24px"; - case "sm": - return "32px"; - case "md": - return "40px"; - case "lg": - return "70px"; - case "xl": - return "96px"; - default: - return "40px"; - } + return AVATAR_SIZES[size] ?? AVATAR_SIZES.md; } function getAvatarShape(shape: AvatarShape) { - switch (shape) { - case "rounded": - return "4px"; - case "square": - return "0px"; - default: - return "50%"; - } + return AVATAR_SHAPES[shape] ?? AVATAR_SHAPES.circle; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/avatar/global.d.ts(1 hunks)packages/avatar/package.json(1 hunks)packages/avatar/src/Avatar.test.tsx(1 hunks)packages/avatar/src/Avatar.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/avatar/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/avatar/src/Avatar.test.tsx
🔇 Additional comments (4)
packages/avatar/package.json (2)
42-42: 🛠️ Refactor suggestion@testing-library/react 버전 업데이트가 필요합니다
현재 사용 중인 버전(^16.0.1)이 최신 버전이 아닙니다.
다음과 같이 수정해주세요:
- "@testing-library/react": "^16.0.1", + "@testing-library/react": "^14.1.2",Likely invalid or redundant comment.
11-11:⚠️ Potential issue소스 파일이 아닌 배포된 파일을 내보내도록 수정이 필요합니다
exports필드가 소스 파일을 직접 가리키고 있습니다. 배포된 패키지는 컴파일된 파일을 참조해야 합니다.다음과 같이 수정해주세요:
- "exports": "./src/index.ts", + "exports": { + ".": { + "import": "./dist/index.js", + "require": "./dist/index.cjs", + "types": "./dist/index.d.ts" + } + }Likely invalid or redundant comment.
packages/avatar/src/Avatar.tsx (2)
51-51: 🛠️ Refactor suggestionref 타입을 더 구체적으로 지정해주세요
ForwardedRef<any>대신 더 구체적인 타입을 사용하면 타입 안전성이 향상됩니다.다음과 같이 수정해주세요:
- ref: ForwardedRef<any> + ref: ForwardedRef<HTMLDivElement>Likely invalid or redundant comment.
69-80: 🛠️ Refactor suggestion접근성 개선이 필요합니다
스크린 리더 사용자를 위한 적절한 ARIA 속성이 누락되어 있습니다.
다음과 같이 수정해주세요:
{src ? ( <img src={src} alt={alt} + aria-invalid={false} onError={(e) => { - if (fallback) e.currentTarget.src = fallback; + if (fallback) { + e.currentTarget.src = fallback; + e.currentTarget.setAttribute('aria-invalid', 'true'); + } }} className={styles.image} /> ) : ( - <span className={styles.fallback}>{alt || fallback}</span> + <span className={styles.fallback} role="img" aria-label={alt || fallback}> + {alt || fallback} + </span> )}Likely invalid or redundant comment.
kimdaeyeobbb
force-pushed
the
feat/avatar
branch
from
d41c9a6 to
cce1e51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/avatar/src/Avatar.tsx (1)
85-111: 헬퍼 함수의 성능을 개선할 수 있습니다
switch문 대신 객체 매핑을 사용하면 더 효율적일 수 있습니다.-function getAvatarSize(size: AvatarSize) { - switch (size) { - case "xs": - return "24px"; - case "sm": - return "32px"; - case "md": - return "40px"; - case "lg": - return "70px"; - case "xl": - return "96px"; - default: - return "40px"; - } -} +const AVATAR_SIZES: Record<AvatarSize, string> = { + xs: "24px", + sm: "32px", + md: "40px", + lg: "70px", + xl: "96px", +}; + +function getAvatarSize(size: AvatarSize) { + return AVATAR_SIZES[size] ?? "40px"; +} -function getAvatarShape(shape: AvatarShape) { - switch (shape) { - case "rounded": - return "4px"; - case "square": - return "0px"; - default: - return "50%"; - } -} +const AVATAR_SHAPES: Record<AvatarShape, string> = { + rounded: "4px", + square: "0px", + circle: "50%", +}; + +function getAvatarShape(shape: AvatarShape) { + return AVATAR_SHAPES[shape] ?? "50%"; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/avatar/.storybook/main.ts(1 hunks)packages/avatar/.storybook/preview.ts(1 hunks)packages/avatar/global.d.ts(1 hunks)packages/avatar/package.json(1 hunks)packages/avatar/src/Avatar.module.css(1 hunks)packages/avatar/src/Avatar.stories.tsx(1 hunks)packages/avatar/src/Avatar.test.tsx(1 hunks)packages/avatar/src/Avatar.tsx(1 hunks)packages/avatar/src/index.ts(1 hunks)packages/avatar/tsconfig.json(1 hunks)packages/avatar/tsup.config.ts(1 hunks)packages/avatar/vitest.config.ts(1 hunks)packages/avatar/vitest.setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/avatar/global.d.ts
- packages/avatar/vitest.setup.ts
- packages/avatar/tsup.config.ts
- packages/avatar/.storybook/preview.ts
- packages/avatar/src/index.ts
- packages/avatar/tsconfig.json
- packages/avatar/src/Avatar.module.css
- packages/avatar/.storybook/main.ts
- packages/avatar/vitest.config.ts
- packages/avatar/src/Avatar.stories.tsx
- packages/avatar/src/Avatar.test.tsx
- packages/avatar/package.json
🔇 Additional comments (3)
packages/avatar/src/Avatar.tsx (3)
11-29: 타입 정의가 명확하고 잘 문서화되어 있습니다!크기와 모양에 대한 타입 정의가 한글 주석과 함께 잘 문서화되어 있습니다.
51-51: ref 타입을 더 구체적으로 지정해주세요
ForwardedRef<any>대신 더 구체적인 타입을 사용하는 것이 좋습니다.- ref: ForwardedRef<any> + ref: ForwardedRef<HTMLDivElement>
69-80: 접근성 개선이 필요합니다스크린 리더 사용자를 위한 접근성 개선이 필요합니다:
- 이미지 로딩 실패 시 적절한 피드백
- 대체 텍스트에 대한 적절한 role 속성
{src ? ( <img src={src} alt={alt} + aria-invalid={false} onError={(e) => { - if (fallback) e.currentTarget.src = fallback; + if (fallback) { + e.currentTarget.src = fallback; + e.currentTarget.setAttribute('aria-invalid', 'true'); + } }} className={styles.image} /> ) : ( - <span className={styles.fallback}>{alt || fallback}</span> + <span className={styles.fallback} role="img" aria-label={alt || fallback}> + {alt || fallback} + </span> )}
Codecov ReportAttention: Patch coverage is
|
변경사항
Avatar #21
기능 명세를 만족하는 테스트 시나리오 작성 a16e7e1
테스트 시나리오를 만족하도록 컴포넌트의 기능 구현 0608c10
스토리 작성 c66f5b6
시각자료
체크리스트
추가 논의사항
Summary by CodeRabbit
릴리스 노트
신규 기능
@sipe-team/avatar추가.버그 수정
문서화