feat/#87 slot 컴포넌트 개발 #100
Conversation
* feat: 반영되지 않은 패키지 변경사항 반영 * chore: 불필요한 gitkeep 파일 삭제 * feat: 컴포넌트 스토리 템플릿 추가 * feat: 컴포넌트 테스트 템플릿 추가 * feat: 컴포넌트 템플릿 추가 * feat: package.json 템플릿 추가 * feat: 리드미 템플릿 추가 * feat: tsconfig.json 템플릿 추가 * feat: tsup.config.ts 템플릿 추가 * rename: 테스트, 컴포넌트, 스토리 템플릿 폴더 구조 세팅 * feat: root devDependency에 tsup 설치 * docs: tsup config 파일 템플릿 추가 * docs: tsconfig 파일 템플릿 추가 * docs: 리드미 파일 템플릿 추가 * docs: 스토리 파일 템플릿 추가 * feat: root devDependency에 @testing-library/react 설치 * docs: 테스트 파일 템플릿 추가 * docs: 컴포넌트 파일 템플릿 추가 * docs: 컴포넌트 배럴 파일 템플릿 추가 * docs: 컴포넌트 package.json 템플릿 추가 * feat: root devDependency에 @turbo/gen 패키지 설치 * chore: 불필요한 파일 삭제 * feat: primitive 컴포넌트 base file generator 작동하도록 turbo gen 설정 * feat: primitive, themed 패키지 중 생성할 패키지 고를 수 있도록 기능 추가 * fix: tsconfig.json 템플릿 루트 tsconfig.json 경로 잘못 지정한 부분 수정 * fix: tsup.config.ts 템플릿 target es2020으로 수정 * chore: 테스트 파일 템플릿 수정 * chore: tsconfig.json 파일 템플릿 수정 * chore: 컴포넌트 파일 템플릿 수정 * chore: 컴포넌트 배럴 파일 템플릿 수정 * chore: 컴포넌트 스토리 파일 템플릿 수정 * chore: root tsconfig 설정 변경 * chore: 자잘한 오타 반영 * chore: package.json build 스크립트 수정 * chore: package.json peerDependency로 next 포함하도록 수정 * chore: 스토리북 예시 파일 복원 * chore: turbo gen prompts message 한글로 변경 * chore: component 템플릿 타입 추론되도록 수정 * chore: component package.json 템플릿 peerDependency에서 next 삭제 * chore: turbo gen generator name, description message 한글로 변경 * chore: package.json 템플릿 name primitive, themed 구분하도록 수정 * chore: package.json 템플릿 name 포맷 변경 * feat: 테스트 코드 템플릿 테스트 케이스 추가 * chore: props 타입 컨벤션 변경에 따른 변경사항 적용 * chore: prettierignore 목록에 turbo 폴더 추가 * feat: pnpm lock 파일 변경사항 반영 * feat: root format 스크립트 추가 * fix: 사라진 prettier 설정 수정 * feat: plop format 기능 추가 * feat: plop kebabCase helper 추가 * chore: plop 생성 파일명 kebabCase로 변경 * feat: root에 turbo gen 스크립트 추가 * feat: prettierignore에 pnpm-lock 파일 추가 * chore: eol 이슈 해결
* chore: playwright 설치 * feat: playwright e2e 테스트 (시각적 회귀 테스트, 웹접근성 테스트) * chore: playwright ci 테스트 * chore: ci 환경에서 의존성 설치 방식 변경 * chore: 워크플로우 ci환경에 playwright 브라우저 설치 * chore: 워크플로우 playwright ci 옵션 추가 * chore: playwright npx로 실행 명령어 변경 * chore: playwright --project 옵션 제거 * chore: 명령어 복구 * chore: playwright 환경 수정 * chore: playwright worker 옵션 변경 * chore: log 추가 * chore: 스토리북 빌드 방식으로 실행 * chore: button 스냅샷 업로드 * chore: playwright 설정 복원 * chore: pr comment 추가 * chore: fetch-depth 추가 * chore: 스냅샷 파일 수정 * chore: testDir path 설정 * chore: playwright 옵션 변경 * chore: 불필요한 테스트 삭제 * chore: checkout version 업데이트 * chore: 테스트 코드 변경 * chore: log 삭제 * chore: error 로그 추가 * chore: 로그 추가 * chore: --update-snapshots 옵션 추가 * chore: 폴더 확인 로직 수정 * chore: yml indent 수정 * chore: 디렉토리 확인 수정 * chore: update snapshot 옵션 삭제 * chore: 시각적 변경 테스트 * chore: update snapshot * chore: 디렉토리 구조 확인 * chore: 아티팩트 업로드 * chore: bold 처리 삭제 * chore: button font arial로 변경 * chore: font 업데이트 이후 actual 변경 * chore: font-weight 700 to bold * chore: web font로 변경 * chore: 폰트 로딩 상태 확인 * chore: roboto로 변경 * chore: font-weight 삭제 * chore: ci 환경에 폰트 설치 * chore: 변경되는 스냅샷 확인 * chore: plarywright 스크립트 변경 * chore: storybook 실행 명령어 수정 * chore: process 변경 * chore: local vrt 삭제 * chore: label flow 수정 * chore: .playwright의 하위에 있는 폴더만 pr에 반영되도록 수정 * Update VRT snapshots in .playwright folder * chore: button에 불필요한 텍스트 삭제 * Update VRT snapshots in .playwright folder * chore: Button 라인 분리 * chore: 실패시 아티팩트 업로드하도록 수정 * chore: 성공 comment 메시지 수정 * chore: chromatic action version 수정 * chore: pnpm action version 변경 * chore: vrt test에 permission 추가 * chore: 토큰 체커 --------- Co-authored-by: GitHub Action <action@github.com>
* chore: component 이슈 템플릿 생성 * chore: component 이슈 템플릿 생성 * chore: init @testing-library/jest-dom * chore: generate divider component * chore: prettier 적용 * feat: Divider 컴포넌트 구현 * feat: Divider 컴포넌트 스토리북 생성 * docs: Divider storybook componentSubtitle 수정 * docs: component issue 템플릿 labels 추가 * docs: Divider storybook componentSubtitle 수정 * docs: component issue 템플릿 내용 수정 * feat: orientation에 따른 태그 변경 및 displayName 수정 * docs: default에 있던 style 삭제 * test: orientation prop에 따른 테스트 코드 추가 * docs: Update Divider README.md * fix: 오타 수정 * chore: Divider Default 이름 변경 및 args 제거 * fix: JSX 형식으로 변경 및 ref 타입 수정 * docs: Divider description 추가 * refactor: 변수명 변경 및 unknown 제거 * chore: pnpm install
minai621
requested review from
gs0428,
ghdtjgus76 and
ShinYoung-Kim
as code owners
Walkthrough이 변경 사항은 Changes
Assessment against linked issues
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: 1
Outside diff range, codebase verification and nitpick comments (2)
packages/primitive/components/slot/src/components/slottable.tsx (1)
3-3: 빈 인터페이스 대신 타입 별칭을 사용하는 것이 좋습니다.정적 분석 도구에서 제안한 대로, 빈 인터페이스 대신 타입 별칭을 사용하는 것이 가독성과 일관성 측면에서 더 좋습니다.
다음과 같이 변경하는 것을 제안합니다:
-export interface SlottableProps {} +export type SlottableProps = {};Tools
Biome
[error] 3-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/primitive/components/slot/src/utils/merge-props.ts (1)
22-42: 승인되었지만 console.log 문을 제거해 주세요.
mergeProps함수의 구현이 올바릅니다.하지만 34번 줄의 console.log 문은 프로덕션 환경에서는 제거되어야 합니다.
아래 diff를 적용하여 console.log 문을 제거해 주세요:
- } else if (propName === "className") { - console.log(mergeClassNames(parentValue, childValue)); - result.className = mergeClassNames(parentValue, childValue); + } else if (propName === "className") { + result.className = mergeClassNames(parentValue, childValue);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- packages/primitive/components/divider/README.md (1 hunks)
- packages/primitive/components/slot/README.md (1 hunks)
- packages/primitive/components/slot/tests/slot.test.tsx (1 hunks)
- packages/primitive/components/slot/package.json (1 hunks)
- packages/primitive/components/slot/src/components/composed-child.tsx (1 hunks)
- packages/primitive/components/slot/src/components/slottable.tsx (1 hunks)
- packages/primitive/components/slot/src/index.ts (1 hunks)
- packages/primitive/components/slot/src/slot.tsx (1 hunks)
- packages/primitive/components/slot/src/utils/composed-ref.ts (1 hunks)
- packages/primitive/components/slot/src/utils/get-element-ref.ts (1 hunks)
- packages/primitive/components/slot/src/utils/is-slottable.ts (1 hunks)
- packages/primitive/components/slot/src/utils/merge-props.ts (1 hunks)
- packages/primitive/components/slot/stories/slot.stories.tsx (1 hunks)
- packages/primitive/components/slot/tsconfig.json (1 hunks)
- packages/primitive/components/slot/tsup.config.ts (1 hunks)
- turbo/generators/templates/component/package.json.hbs (1 hunks)
Files skipped from review due to trivial changes (6)
- packages/primitive/components/divider/README.md
- packages/primitive/components/slot/package.json
- packages/primitive/components/slot/src/index.ts
- packages/primitive/components/slot/tsconfig.json
- packages/primitive/components/slot/tsup.config.ts
- turbo/generators/templates/component/package.json.hbs
Additional context used
Biome
packages/primitive/components/slot/src/components/slottable.tsx
[error] 3-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/primitive/components/slot/src/components/composed-child.tsx
[error] 14-14: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (16)
packages/primitive/components/slot/src/utils/is-slottable.ts (1)
1-7: LGTM!유틸리티 함수의 구현이 올바르게 되어 있습니다. React의
isValidElement를 사용하여 주어진 자식이 유효한 React 요소인지 확인하고, 해당 요소의 타입을Slottable컴포넌트와 비교하여Slottable컴포넌트인지 판단합니다.packages/primitive/components/slot/src/components/slottable.tsx (1)
1-11: LGTM!
Slottable컴포넌트의 구현이 올바르게 되어 있습니다. 단순히 자식 요소를 렌더링하는 역할을 수행합니다.Tools
Biome
[error] 3-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/primitive/components/slot/src/utils/composed-ref.ts (1)
1-11: LGTM!
composeRefs유틸리티 함수의 구현이 올바르게 되어 있습니다. 여러 개의 ref를 하나의 ref 콜백으로 합성하는 역할을 수행합니다. 제공된 ref들을 순회하면서, 함수형 ref인 경우 해당 노드를 인자로 호출하고, ref 객체인 경우current속성에 노드를 할당합니다.packages/primitive/components/slot/src/utils/get-element-ref.ts (1)
1-18: 코드 변경 사항이 승인되었습니다.이 파일의 코드는 잘 구조화되어 있고 모범 사례를 따르고 있습니다. 개선이 필요한 부분은 없습니다.
packages/primitive/components/slot/src/components/composed-child.tsx (1)
1-13: 코드 변경 사항이 승인되었습니다.이 파일의 나머지 코드는 잘 구조화되어 있고 모범 사례를 따르고 있습니다. 개선이 필요한 부분은 없습니다.
Also applies to: 15-39
packages/primitive/components/slot/src/slot.tsx (1)
1-47: 코드 변경 사항이 승인되었습니다.이 파일의 코드는 잘 구조화되어 있고 모범 사례를 따르고 있습니다. 개선이 필요한 부분은 없습니다.
packages/primitive/components/slot/src/utils/merge-props.ts (3)
4-6: LGTM!코드 변경 사항이 승인되었습니다.
8-16: LGTM!코드 변경 사항이 승인되었습니다.
18-20: LGTM!코드 변경 사항이 승인되었습니다.
packages/primitive/components/slot/README.md (1)
1-151: 문서 변경 사항이 승인되었습니다!README 문서가 잘 작성되었습니다.
Slot과Slottable컴포넌트의 사용법과 주요 기능을 명확한 예제와 함께 잘 설명하고 있습니다.packages/primitive/components/slot/__tests__/slot.test.tsx (5)
7-21: 테스트 케이스가 승인되었습니다!
Slot컴포넌트의 렌더링과 ref 전달을 테스트하는 두 가지 테스트 케이스가 올바르게 구현되었습니다.
23-37: 테스트 케이스가 승인되었습니다!
Slottable컴포넌트의 렌더링을 테스트하는 테스트 케이스가 올바르게 구현되었습니다.
39-79: 테스트 케이스가 승인되었습니다!
Slot컴포넌트의 이벤트 핸들링을 테스트하는 세 가지 테스트 케이스가 올바르게 구현되었습니다.Slot컴포넌트에 이벤트 핸들러를 전달하는 경우, 자식 컴포넌트에 이벤트 핸들러를 전달하는 경우, 그리고 두 컴포넌트 모두에 이벤트 핸들러를 전달하는 경우를 잘 다루고 있습니다.
81-102: 테스트 케이스가 승인되었습니다!
Slot컴포넌트의 props 전달을 테스트하는 두 가지 테스트 케이스가 올바르게 구현되었습니다. className과 style props를Slot컴포넌트에 전달하는 시나리오를 잘 다루고 있습니다.
104-152: 테스트 케이스가 승인되었습니다!
Slot컴포넌트를 사용하는Link컴포넌트를 테스트하는 네 가지 테스트 케이스가 올바르게 구현되었습니다.asChildprop에 따라Link컴포넌트가 앵커 태그 또는Slot컴포넌트로 렌더링되는 시나리오를 잘 다루고 있습니다. 또한asChild가 true일 때Link컴포넌트의 props 전달과 이벤트 핸들링도 잘 테스트하고 있습니다.packages/primitive/components/slot/stories/slot.stories.tsx (1)
1-288: Storybook 스토리가Slot과Slottable컴포넌트의 사용법을 잘 보여주고 있습니다.이 파일은
Slot과Slottable컴포넌트를 사용하여 다양한 UI 컴포넌트(Button, Card, ListItem, FormInput)를 구현하는 방법을 비교하는 Storybook 스토리를 포함하고 있습니다. 스토리는Slot과Slottable이 어떻게 유연한 컴포넌트 구성을 만드는데 사용될 수 있는지 명확한 예시를 제공합니다.이 스토리는
Slot과Slottable컴포넌트의 기능을 문서화하고 쇼케이스하는 좋은 방법입니다. 일반적인 UI 컴포넌트를 폭넓게 다루고 있으며Slot과Slottable이 제공하는 유연성을 잘 보여줍니다. 코드는 잘 구조화되어 있고 예제는 명확하고 이해하기 쉽습니다.
packages/primitive/components/slot/src/components/composed-child.tsx
Outdated
Show resolved
Hide resolved
minai621
added
VRT
packages/primitive/components/slot/src/components/composed-child.tsx
Outdated
Show resolved
Hide resolved
packages/primitive/components/slot/src/components/composed-child.tsx
Outdated
Show resolved
Hide resolved
… leftIcon와 rightIcon로 변경
- componentName을 kebabCase로 변경하여 directory 경로 수정
|
🐱 스토리북이 배포되었습니다: https://www.chromatic.com/build?appId=667edcfe9b923ba1892556f4&number=70 🐱 |
VRT 테스트 성공VRT 테스트가 성공적으로 완료되었습니다. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
packages/primitive/components/slot/README.md (3)
15-45: "Props 위임" 섹션이 잘 설명되어 있습니다.
Slot컴포넌트를 사용한 props 위임 개념이 명확하게 설명되어 있고, 코드 예제도 적절합니다.asChildprop의 사용과 스타일 병합을 잘 보여주고 있습니다.코드 예제에 간단한 주석을 추가하여 각 부분의 역할을 설명하면 더욱 이해하기 쉬울 것 같습니다. 예를 들어:
// Button 컴포넌트 정의 const Button = React.forwardRef< HTMLButtonElement, React.ButtonHTMLAttributes<HTMLButtonElement> & { asChild?: boolean } >(({ asChild, ...props }, ref) => { const Comp = asChild ? Slot : "button"; return <Comp {...props} ref={ref} />; }); // Button 컴포넌트 사용 예시 <Button asChild style={{ padding: "10px", borderRadius: "5px", background: "blue", color: "white" }} > {/* a 태그를 Button의 자식으로 사용 */} <a href="#" onClick={(e) => { e.preventDefault(); alert("Slot 버튼이 클릭되었습니다!"); }} style={{ textDecoration: "none", color: "inherit" }} > 클릭하세요 (버튼 스타일의 a 태그입니다) </a> </Button>;
47-92: "Props 병합" 섹션이 상세하고 명확합니다.
Slot컴포넌트가 부모와 자식 컴포넌트의 props를 어떻게 병합하는지 잘 설명되어 있습니다. 코드 예제와 병합 결과에 대한 주석이 이해를 돕는 데 매우 유용합니다.병합 결과를 보여주는 주석 부분을 실제 코드 블록으로 변경하면 가독성이 더 좋아질 것 같습니다. 예를 들어:
// 병합 결과: const mergedProps = { id: "child-button", "data-test": "parent", className: "child-class parent-class", onClick: chainFunctions([() => console.log('Child 클릭'), () => console.log('Parent 클릭')]), style: { padding: "10px", color: "red", backgroundColor: "blue" }, };이렇게 하면 문법 하이라이팅이 적용되어 더 읽기 쉬워집니다.
94-121: "Slottable 컴포넌트" 섹션이 잘 구성되어 있습니다.
Slottable컴포넌트의 유연한 커스터마이징 기능이 잘 설명되어 있습니다.Button컴포넌트 내에서Slottable을 사용하는 코드 예제가 적절합니다.코드 예제에서
iconLeft와iconRightprops가 사용되고 있지만,Button컴포넌트 정의에서는leftIcon과rightIcon으로 명명되어 있습니다. 일관성을 위해 이름을 통일하는 것이 좋겠습니다. 예를 들어:const Button = React.forwardRef< HTMLButtonElement, React.ButtonHTMLAttributes<HTMLButtonElement> & { asChild?: boolean; leftIcon?: React.ReactNode; rightIcon?: React.ReactNode; } >(({ asChild = false, leftIcon, rightIcon, ...props }, forwardedRef) => { const Comp = asChild ? Slot : "button"; return ( <Comp {...props} ref={forwardedRef}> {leftIcon} <Slottable>{children}</Slottable> {rightIcon} </Comp> ); }); // 사용 예시 <Button asChild leftIcon={<Icon name="link" />}> <a href="https://example.com">Visit Website</a> </Button>;packages/primitive/components/slot/stories/slot.stories.tsx (3)
32-60: 컴포넌트 정의가 잘 되어 있습니다. 작은 개선 제안이 있습니다.Button, Card, ListItem, FormInput 컴포넌트들이 일관성 있게 잘 구현되어 있습니다.
forwardRef를 사용하여 ref를 적절히 처리하고 있으며,asChildprop을 통해 렌더링 방식을 유연하게 조절할 수 있습니다.성능 최적화를 위해 각 컴포넌트의
Comp변수 선언을 메모이제이션할 수 있습니다. 예를 들어, Button 컴포넌트에서:const Button = forwardRef< HTMLButtonElement, ButtonHTMLAttributes<HTMLButtonElement> & { asChild?: boolean } >(({ asChild, ...props }, ref) => { const Comp = useMemo(() => asChild ? Slot : "button", [asChild]); return <Comp {...props} ref={ref} />; });이렇게 하면 불필요한 재렌더링을 방지할 수 있습니다.
62-116: ButtonComparison 스토리가 잘 구현되어 있습니다. 접근성 개선을 위한 제안이 있습니다.ButtonComparison 스토리가 Slot과 Slottable의 사용을 효과적으로 비교 설명하고 있습니다. 세 가지 시나리오(Slot 없음, Slot 사용, Slottable 사용)를 통해 각 접근 방식의 차이점을 잘 보여주고 있습니다.
접근성 향상을 위해 버튼에
aria-label을 추가하는 것이 좋습니다. 예를 들어:<button onClick={() => alert("클릭되었습니다!")} style={{ padding: "10px", borderRadius: "5px" }} aria-label="기본 버튼" > 클릭하세요 </button>이렇게 하면 스크린 리더 사용자들에게 더 명확한 정보를 제공할 수 있습니다.
118-291: CardComparison, ListItemComparison, FormInputComparison 스토리들이 잘 구현되어 있습니다. 일관성을 위한 작은 개선 제안이 있습니다.각 스토리가 Slot과 Slottable 컴포넌트의 유연성을 효과적으로 보여주고 있습니다. 인터랙티브 요소와 스타일링을 통해 각 접근 방식의 시각적, 기능적 차이를 명확히 제시하고 있습니다.
일관성 향상을 위해 모든 스토리에서 스타일 객체를 분리하여 정의하는 것이 좋습니다. 예를 들어:
const cardStyles = { padding: "20px", border: "1px solid #ccc", borderRadius: "10px", }; // 사용 시 <Card style={cardStyles}> {/* 내용 */} </Card>이렇게 하면 스타일의 재사용성이 높아지고, 일관된 스타일 적용이 용이해집니다.
📜 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 (10)
- .eslintrc.json (1 hunks)
- package.json (2 hunks)
- packages/primitive/components/slot/README.md (1 hunks)
- packages/primitive/components/slot/tests/slot.test.tsx (1 hunks)
- packages/primitive/components/slot/package.json (1 hunks)
- packages/primitive/components/slot/src/components/composed-child.tsx (1 hunks)
- packages/primitive/components/slot/src/slot.tsx (1 hunks)
- packages/primitive/components/slot/src/utils/merge-props.ts (1 hunks)
- packages/primitive/components/slot/stories/slot.stories.tsx (1 hunks)
- turbo/generators/templates/component/package.json.hbs (1 hunks)
🧰 Additional context used
🪛 Biome
packages/primitive/components/slot/src/components/composed-child.tsx
[error] 14-14: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (17)
turbo/generators/templates/component/package.json.hbs (1)
20-20: 변경 사항이 적절히 구현되었습니다.이전 리뷰 의견을 반영하여
componentName을 kebab case로 변환하도록 수정되었습니다. 이는 패키지 명명 규칙과 디렉토리 구조의 일관성을 개선하며, 템플릿의 유연성과 유지보수성을 향상시킵니다.packages/primitive/components/slot/package.json (5)
1-9: 기본 패키지 정보가 적절히 작성되었습니다.패키지 이름, 버전, 라이선스, 설명, 작성자 및 홈페이지 정보가 모두 정확하게 제공되었습니다. 이전 리뷰 코멘트에서 요청한 대로 설명이 추가되었습니다.
10-24: 패키지 구성이 적절합니다.패키지에 포함될 파일, 공개 액세스 설정, 저장소 및 이슈 트래커 정보가 올바르게 구성되어 있습니다. 이는 공개 npm 패키지에 적합한 표준 구성입니다.
25-37: 모듈 내보내기 구성이 우수합니다.ES 모듈과 CommonJS 형식을 모두 지원하도록 구성되어 있어 다양한 환경에서의 호환성이 뛰어납니다. 또한, 두 형식 모두에 대한 타입 정의가 포함되어 있어 TypeScript 사용자들에게 큰 도움이 될 것입니다.
38-41: 스크립트 구성이 적절합니다.빌드와 개발을 위한 스크립트가 잘 구성되어 있습니다. tsup을 사용하여 TypeScript 프로젝트를 빌드하는 것은 좋은 선택입니다. 개발 시 watch 모드를 사용할 수 있어 편리합니다.
42-47: 의존성 구성을 확인해주세요.React와 React DOM이 peer dependencies로 올바르게 지정되어 있습니다. 그러나 직접적인 dependencies와 devDependencies가 비어 있습니다. 이는 문제가 아닐 수 있지만, 일반적이지 않습니다.
다음 사항을 확인해주세요:
- 직접적인 dependencies가 정말로 필요하지 않은지 검토해주세요.
- devDependencies가 비어 있는 이유를 확인해주세요. 모노레포의 루트에서 개발 도구를 관리하고 있다면 문제가 없습니다.
필요한 경우 의존성을 추가하거나, 의도적으로 비워둔 것이라면 주석으로 그 이유를 설명해주시면 좋겠습니다.
packages/primitive/components/slot/src/utils/merge-props.ts (1)
4-6: isEventProp 함수 구현이 적절합니다.이 함수는 React 이벤트 핸들러 prop을 식별하는 간단하고 효율적인 방법을 제공합니다. "on"으로 시작하고 세 번째 문자가 대문자인 prop 이름을 확인하는 로직이 정확합니다.
package.json (1)
19-21: 테스트 라이브러리 추가 및 타입스크립트 버전 유지 확인
"@testing-library/jest-dom"과 "@testing-library/user-event" 라이브러리의 추가를 승인합니다. 이는 프로젝트의 테스트 기능을 향상시킬 것으로 보입니다.
타입스크립트 버전이 "^5.4.5"로 유지되고 있음을 확인했습니다. 이는 현재 변경사항에 대해 타입스크립트 주요 업데이트가 필요하지 않았음을 시사합니다.
AI 요약에서 "@testing-library/jest-dom"의 중복 항목이 제거되었다고 언급되었지만, 제공된 코드 스니펫에서는 이를 확인할 수 없습니다. 이 불일치에 대해 명확히 해주시겠습니까?
중복 항목 제거에 대한 언급을 확인하기 위해 다음 스크립트를 실행해 주세요:
이 스크립트는 이전 버전의 package.json 파일에서 "@testing-library/jest-dom"의 발생을 확인합니다. 결과에 따라 중복 항목의 존재 여부를 판단할 수 있습니다.
Also applies to: 44-44
.eslintrc.json (1)
74-79: 스토리북 파일에 대한 ESLint 규칙 변경이 적절해 보입니다.이 변경사항은
*.stories.tsx파일에 대해react/display-name규칙을 비활성화합니다. 이는 스토리북 컴포넌트에서 종종 익명 컴포넌트를 사용하거나 명시적인 디스플레이 이름이 필요하지 않기 때문에 적절한 변경으로 보입니다. 이 변경으로Slot컴포넌트의 스토리북 예제 작성이 더 용이해질 것입니다.packages/primitive/components/slot/README.md (3)
1-13: README 파일의 소개 및 설치 섹션이 명확하고 포괄적입니다.소개 부분에서
slot컴포넌트의 목적과 이점을 잘 설명하고 있습니다. 설치 지침도 다양한 패키지 관리자에 대해 제공되어 있어 사용자 친화적입니다.
123-143: "Ref 핸들링" 섹션이 명확하고 유용합니다.
Slot컴포넌트가 ref를 어떻게 처리하는지 잘 설명되어 있습니다. 코드 예제에서useRef와useEffect를 사용하여 ref 전달과 사용 방법을 적절히 보여주고 있습니다.이 섹션은
Slot컴포넌트의 고급 기능을 이해하는 데 도움이 될 것입니다.
1-143: 전반적으로 우수한 README 파일입니다.이 README 파일은
@warrr-ui/primitive-slot패키지의 모든 핵심 측면을 포괄적으로 다루고 있습니다. 각 섹션은 명확한 설명과 관련 코드 예제를 제공하여 사용자가 패키지를 이해하고 사용하는 데 큰 도움이 될 것입니다.구조가 잘 잡혀 있고 정보가 풍부하며, 코드 예제들이 실제 사용 사례를 잘 보여주고 있습니다. 제안된 몇 가지 작은 개선 사항들을 반영하면 더욱 완벽한 문서가 될 것 같습니다.
packages/primitive/components/slot/stories/slot.stories.tsx (2)
1-30: LGTM: 임포트와 메타 객체 정의가 적절합니다.임포트 문과 메타 객체 정의가 잘 구성되어 있습니다. Storybook에 필요한 정보를 적절히 제공하고 있으며, 컴포넌트 설명과
childrenprop에 대한 설명도 포함되어 있습니다.
1-291: 전반적으로 우수한 품질의 Storybook 스토리 파일입니다.이 파일은 Slot 컴포넌트의 기능을 다양한 예시를 통해 효과적으로 보여주고 있습니다. 코드 구조가 명확하고 일관성 있게 작성되어 있어 가독성과 유지보수성이 뛰어납니다. 각 스토리는 Slot과 Slottable 컴포넌트의 사용 사례를 잘 설명하고 있으며, 인터랙티브 요소를 통해 사용자가 쉽게 이해할 수 있도록 구성되어 있습니다.
제안된 개선 사항들(메모이제이션, 접근성, 스타일 분리)을 적용하면 코드의 품질을 더욱 향상시킬 수 있을 것입니다. 전체적으로 이 파일은 Slot 컴포넌트의 기능과 유연성을 잘 보여주는 우수한 예시 자료입니다.
packages/primitive/components/slot/src/components/composed-child.tsx (1)
16-35: 구현이 적절합니다.
ComposedChild컴포넌트는 props와 ref를 적절히 처리하여 자식 요소를 확장하고 있습니다.packages/primitive/components/slot/src/slot.tsx (1)
26-28: 타입 단언 없이도 타입 추론이 가능합니다.
isSlottable함수가 ReactElement에 대한 타입 가드 역할을 하므로,(newElement as ReactElement)와 같은 타입 단언 없이도 타입이 올바르게 추론될 수 있습니다.packages/primitive/components/slot/__tests__/slot.test.tsx (1)
27-42:Slottable컴포넌트에 대한 테스트가 적절하게 작성되었습니다
Slottable컴포넌트가 올바르게 렌더링되고 있는지 확인하는 테스트가 정확합니다.
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| type AnyProps = Record<string, any>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
eslint-disable 주석 대신 더 구체적인 접근 방식을 고려해 보세요.
현재 파일 전체에 대해 @typescript-eslint/no-explicit-any 규칙을 비활성화하고 있습니다. 이는 타입 안전성을 저해할 수 있습니다.
다음과 같은 대안을 고려해 보세요:
- 가능한 경우
any대신 더 구체적인 타입을 사용합니다. any를 사용해야 하는 특정 라인에 대해서만 규칙을 비활성화합니다.- 프로젝트의
.eslintrc파일에 이 규칙에 대한 예외를 추가합니다.
예를 들어, AnyProps 타입에 대해서만 규칙을 비활성화하려면 다음과 같이 할 수 있습니다:
// eslint-disable-next-line @typescript-eslint/no-explicit-any
type AnyProps = Record<string, any>;이렇게 하면 필요한 곳에서만 any를 사용하고 나머지 코드에서는 타입 안전성을 유지할 수 있습니다.
| function chainFunctions(...callbacks: any[]) { | ||
| return (...args: any[]) => { | ||
| callbacks.forEach((callback) => { | ||
| if (typeof callback === "function") { | ||
| callback(...args); | ||
| } | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
chainFunctions의 타입 안전성을 개선해 보세요.
현재 구현은 여러 콜백을 효과적으로 체인화하고 있습니다. 하지만 any 타입의 사용으로 인해 타입 안전성이 떨어집니다.
다음과 같이 제네릭을 사용하여 타입 안전성을 개선할 수 있습니다:
function chainFunctions<T extends any[]>(...callbacks: Array<((...args: T) => void) | undefined>): (...args: T) => void {
return (...args: T) => {
callbacks.forEach((callback) => {
if (typeof callback === "function") {
callback(...args);
}
});
};
}이 방식을 사용하면 콜백 함수의 인자 타입을 보존하면서도 undefined를 허용하여 유연성을 유지할 수 있습니다.
| }); | ||
| } | ||
|
|
||
| return Children.count(children) > 1 ? Children.only(null) : null; |
There was a problem hiding this comment.
Children.only(null)의 사용에 문제가 있습니다
Children.only는 하나의 유효한 React 자식 요소를 기대하며, null을 전달하면 오류가 발생합니다. 자식이 둘 이상인 경우 적절한 오류 처리를 하는 것이 좋습니다.
다음과 같이 수정하는 것을 제안합니다:
-return Children.count(children) > 1 ? Children.only(null) : null;
+if (Children.count(children) > 1) {
+ throw new Error("ComposedChild 컴포넌트는 하나의 자식만 받아야 합니다.");
+}
+return null;📝 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.
| return Children.count(children) > 1 ? Children.only(null) : null; | |
| if (Children.count(children) > 1) { | |
| throw new Error("ComposedChild 컴포넌트는 하나의 자식만 받아야 합니다."); | |
| } | |
| return null; |
| it("정상적으로 렌더링되어야 합니다", () => { | ||
| const { container } = render(<Slot>Hello</Slot>); | ||
|
|
||
| expect(container).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("ref가 전달되어야 합니다", () => { | ||
| const ref = createRef<HTMLElement>(); | ||
|
|
||
| render( | ||
| <Slot ref={ref}> | ||
| <div>Hello</div> | ||
| </Slot> | ||
| ); | ||
|
|
||
| expect(ref.current).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
p2;
저번에 말씀드렸던 이 두 테스트 케이스만 수정하면 될 거 같습니다
There was a problem hiding this comment.
refactor: render, ref 테스트 케이스 수정
반영했습니다.
|
🐱 스토리북이 배포되었습니다: https://www.chromatic.com/build?appId=667edcfe9b923ba1892556f4&number=79 🐱 |
VRT 테스트 성공VRT 테스트가 성공적으로 완료되었습니다. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/primitive/components/slot/__tests__/slot.test.tsx (1)
168-176: Link 컴포넌트 구현 위치 조정 필요테스트 파일 내에 실제 컴포넌트 구현이 포함되어 있습니다. 이는 테스트 코드의 관심사 분리 원칙에 위배됩니다.
Link 컴포넌트의 구현을 별도의 파일(예:
Link.tsx)로 분리하고, 테스트 파일에서는 이를 임포트하여 사용하는 것을 추천드립니다.// src/Link.tsx export interface LinkProps extends ComponentProps<"a"> { asChild?: boolean; } export const Link = ({ asChild, ...props }: LinkProps) => { const Comp = asChild ? Slot : "a"; return <Comp {...props} />; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/primitive/components/slot/tests/slot.test.tsx (1 hunks)
🔇 Additional comments (2)
packages/primitive/components/slot/__tests__/slot.test.tsx (2)
21-32: ref 전달 테스트가 잘 구현되었습니다ref의 존재 여부와 타입을 적절히 검증하고 있습니다.
4-4:⚠️ Potential issueuserEvent 모듈의 임포트 방식 수정 필요
userEvent는 기본 내보내기(default export)이므로 중괄호 없이 임포트해야 합니다.
- import { userEvent } from "@testing-library/user-event"; + import userEvent from "@testing-library/user-event";Likely invalid or redundant comment.
| describe("Slottable 컴포넌트", () => { | ||
| it("Slottable로 감싼 요소가 렌더링되어야 합니다", () => { | ||
| render( | ||
| <Slot> | ||
| <div> | ||
| Hello | ||
| <Slottable> | ||
| <strong>World</strong> | ||
| </Slottable> | ||
| </div> | ||
| </Slot> | ||
| ); | ||
|
|
||
| expect(screen.getByText("World")).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Slottable 컴포넌트 테스트 보완 필요
현재는 텍스트 렌더링만 확인하고 있습니다. 컴포넌트의 구조와 중첩된 엘리먼트들의 관계도 검증하면 좋을 것 같습니다.
it("Slottable로 감싼 요소의 구조가 올바르게 렌더링되어야 합니다", () => {
const { container } = render(
<Slot>
<div>
Hello
<Slottable>
<strong>World</strong>
</Slottable>
</div>
</Slot>
);
const div = container.querySelector('div');
expect(div).toBeInTheDocument();
expect(div?.firstChild?.textContent).toBe('Hello');
expect(div?.querySelector('strong')).toHaveTextContent('World');
});| describe("이벤트 핸들러", () => { | ||
| const handleClick = jest.fn(); | ||
| const handleSlotClick = jest.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| handleClick.mockReset(); | ||
| handleSlotClick.mockReset(); | ||
| }); | ||
|
|
||
| it("Slot에 전달된 onClick 핸들러가 호출되어야 합니다", async () => { | ||
| render( | ||
| <Slot onClick={handleClick}> | ||
| <div>Click me</div> | ||
| </Slot> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByText("Click me")); | ||
|
|
||
| expect(handleClick).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("Slot의 자식 요소에 전달된 onClick 핸들러가 호출되어야 합니다", async () => { | ||
| render( | ||
| <Slot> | ||
| <button onClick={handleClick}>Click me</button> | ||
| </Slot> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByText("Click me")); | ||
|
|
||
| expect(handleClick).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("Slot과 자식 요소 모두에 onClick 핸들러가 전달되었을 때, 두 핸들러 모두 호출되어야 합니다", async () => { | ||
| render( | ||
| <Slot onClick={handleSlotClick}> | ||
| <button onClick={handleClick}>Click me</button> | ||
| </Slot> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByText("Click me")); | ||
|
|
||
| expect(handleSlotClick).toHaveBeenCalledTimes(1); | ||
| expect(handleClick).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이벤트 핸들러 테스트 케이스 추가 제안
현재 테스트는 정상적인 클릭 이벤트만 다루고 있습니다. 에러 케이스나 이벤트 버블링 중단과 같은 시나리오도 테스트하면 좋을 것 같습니다.
it("이벤트 핸들러에서 에러가 발생해도 다른 핸들러가 실행되어야 합니다", async () => {
const handleError = jest.fn(() => { throw new Error('Test error'); });
const handleSuccess = jest.fn();
render(
<Slot onClick={handleSuccess}>
<button onClick={handleError}>Click me</button>
</Slot>
);
await userEvent.click(screen.getByText("Click me"));
expect(handleError).toHaveBeenCalled();
expect(handleSuccess).toHaveBeenCalled();
});| describe("Props 전달", () => { | ||
| it("Slot에 전달된 props가 자식 요소에 전달되어야 합니다", () => { | ||
| render( | ||
| <Slot className="parent" data-testid="child"> | ||
| <div>Child</div> | ||
| </Slot> | ||
| ); | ||
|
|
||
| const child = screen.getByText("Child"); | ||
| expect(child).toHaveClass("parent"); | ||
| }); | ||
|
|
||
| it("Slot에 전달된 style props가 자식 요소에 전달되어야 합니다", () => { | ||
| render( | ||
| <Slot style={{ color: "red" }}> | ||
| <div>Child</div> | ||
| </Slot> | ||
| ); | ||
|
|
||
| expect(screen.getByText("Child")).toHaveStyle({ color: "red" }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Props 전달 테스트 케이스 보완 필요
현재는 className과 style prop만 테스트하고 있습니다. data-* 속성, aria-* 속성, 그리고 prop 병합 시나리오도 테스트하면 좋을 것 같습니다.
it("자식 컴포넌트의 props가 Slot의 props와 올바르게 병합되어야 합니다", () => {
render(
<Slot className="parent" aria-label="parent">
<div className="child" aria-description="child">Child</div>
</Slot>
);
const element = screen.getByText("Child");
expect(element).toHaveClass("parent", "child");
expect(element).toHaveAttribute("aria-label", "parent");
expect(element).toHaveAttribute("aria-description", "child");
});| describe("Slot 컴포넌트", () => { | ||
| it("정상적으로 렌더링되어야 합니다", () => { | ||
| const { getByTestId } = render( | ||
| <Link asChild data-testid="slot"> | ||
| <a href="www.naver.com">Naver</a> | ||
| </Link> | ||
| ); | ||
|
|
||
| const slot = getByTestId("slot"); | ||
|
|
||
| expect(slot).toBeInTheDocument(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
렌더링 테스트 강화 제안
현재 테스트는 기본적인 렌더링 여부만 확인하고 있습니다. Link 컴포넌트의 중요한 속성들도 함께 검증하면 좋을 것 같습니다.
expect(slot).toBeInTheDocument();
+ expect(slot).toHaveAttribute('href', 'www.naver.com');
+ expect(slot.textContent).toBe('Naver');📝 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.
| describe("Slot 컴포넌트", () => { | |
| it("정상적으로 렌더링되어야 합니다", () => { | |
| const { getByTestId } = render( | |
| <Link asChild data-testid="slot"> | |
| <a href="www.naver.com">Naver</a> | |
| </Link> | |
| ); | |
| const slot = getByTestId("slot"); | |
| expect(slot).toBeInTheDocument(); | |
| }); | |
| describe("Slot 컴포넌트", () => { | |
| it("정상적으로 렌더링되어야 합니다", () => { | |
| const { getByTestId } = render( | |
| <Link asChild data-testid="slot"> | |
| <a href="www.naver.com">Naver</a> | |
| </Link> | |
| ); | |
| const slot = getByTestId("slot"); | |
| expect(slot).toBeInTheDocument(); | |
| expect(slot).toHaveAttribute('href', 'www.naver.com'); | |
| expect(slot.textContent).toBe('Naver'); | |
| }); |
예상 동작 (Expected Behavior)
Slot컴포넌트를 사용하여 부모 컴포넌트와 자식 컴포넌트 간의 관계를 재정의하거나 스타일을 변경할 수 있습니다.부모 컴포넌트에서 전달한 props와 자식 컴포넌트에서 정의한 props가 자동으로 병합되어 적용됩니다.
Slottable컴포넌트를 사용하여 컴포넌트의 특정 부분을 유연하게 대체할 수 있습니다.상위 컴포넌트에서 전달된 ref는
Slot컴포넌트에 의해 처리되어 슬롯에 전달된 요소의 ref와 병합됩니다.고민했던 내용 (Considerations)
안쓰는 interface들은 삭제할지 고민을 많이 했는데, forwardRef나 다른 타입과의 상호작용이 생길 수 있어 그냥 빈타입으로 생성하였습니다. (어차피 js 번들에는 안들어가서 빈 채로 두는게 가독성이 더 좋은 것 같습니다)
Slot 컴포넌트의 사용 방법에 대해 이해하기 어려울 것 같아 storybook에 예시를 최대한 많이 작성하였으니 참고 부탁드립니다.
radix-ui slot
render delegation에 대한 블로그 게시글
관련 이슈
Summary by CodeRabbit
New Features
@warrr-ui/divider에서@warrr-ui/primitive-divider로 변경되었습니다.Slot및Slottable컴포넌트가 새롭게 추가되었습니다.Slot컴포넌트가 자식 컴포넌트를 유연하게 관리하고 렌더링하는 기능을 제공합니다.Slot컴포넌트에 대한 Storybook 스토리가 추가되었습니다.Link컴포넌트가 추가되어,Slot또는 앵커 태그로 렌더링할 수 있습니다.Slot및Slottable컴포넌트에 대한 포괄적인 테스트가 추가되었습니다.Bug Fixes
Documentation
@warrr-ui/primitive-slot패키지에 대한 문서가 추가되었습니다.Slot컴포넌트의 사용법과 예시가 포함된 스토리가 추가되었습니다.