feat/#87 slot 컴포넌트 개발 #100
Conversation
minai621
requested review from
gs0428,
ghdtjgus76 and
ShinYoung-Kim
as code owners
Walkthrough이 변경 사항은 Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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이 제공하는 유연성을 잘 보여줍니다. 코드는 잘 구조화되어 있고 예제는 명확하고 이해하기 쉽습니다.
minai621
added
VRT
minai621
added
VRT
minai621
added
VRT
|
🐱 스토리북이 배포되었습니다: https://www.chromatic.com/build?appId=667edcfe9b923ba1892556f4&number=59 🐱 |
VRT 테스트 성공VRT 테스트가 성공적으로 완료되었습니다. |
| @@ -0,0 +1,11 @@ | |||
| import { PropsWithChildren } from "react"; | |||
|
|
|||
| export interface SlottableProps {} | |||
There was a problem hiding this comment.
Props는 객체 형태일 예정이라 interface 고정으로 쓰겠습니다! (코드 컨벤션의 통일이랑, 다시 type -> interface로 변경하는 작업 생략)
github-actions
bot
added
D-1
| iconLeft?: React.ReactNode; | ||
| iconRight?: React.ReactNode; |
There was a problem hiding this comment.
예시이기는 하나 leftIcon, rightIcon이라는 네이밍이 조금 더 적합해보입니다
| import { fireEvent, render, screen } from "@testing-library/react"; | ||
|
|
There was a problem hiding this comment.
fireEvent를 사용해서 사용자 이벤트 관련 테스팅 처리를 하셨는데,
이 레퍼런스 참고해보시면 좋을 거 같습니다!
fireEvent와 userEvent의 차이점에 대해서 설명하고 있는데, fireEvent는 직접 DOM 이벤트를 발생시키고, userEvent는 브라우저에서 사용자가 직접 이벤트를 발생시키는 것과 유사하게 발생시킨다고 합니다.
rtl 문서에서도 userEvent를 권장하고 있구요
그래서 userEvent로 수정하면 좋을 거 같습니다
| it("Slot에 전달된 props가 자식 요소에 전달되어야 합니다", () => { | ||
| render( | ||
| <Slot className="parent" data-testid="child"> | ||
| <div>Child</div> | ||
| </Slot> | ||
| ); | ||
|
|
||
| // 자식 요소가 Slot에서 전달된 props를 가지고 있는지 확인 | ||
| const child = screen.getByTestId("child"); | ||
| expect(child).toHaveClass("parent"); | ||
| }); |
There was a problem hiding this comment.
Slot 컴포넌트의 props로 클래스명과 테스트 아이디를 정의해둔 상황이라 자식 요소에서 해당 props를 가지고 있는지 판단하기는 어려울 거 같다는 생각이 듭니다
const childElement = screen.getByText("Child");
위와 같이 div 요소를 가져와서 해당 요소에서 Slot 컴포넌트의 클래스명과 테스트 아이디를 가지고 있는지 판단하는 것이 더 적합해보입니다
| it("asChild prop이 true이면 Slot으로 래핑되어야 합니다", () => { | ||
| render( | ||
| <Link asChild> | ||
| <button>Button</button> | ||
| </Link> | ||
| ); | ||
| expect(screen.getByRole("button")).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
이 테스트는 Slot 컴포넌트로 button 요소를 감싸는지 테스트하기보다는 button 요소가 존재하는지 여부를 판단하는 부분에 더 중점을 둔 거 같아서 수정이 필요해보입니다
Slot 컴포넌트가 감싼다는 부분을 검증해야할 듯합니다
| expect(screen.getByRole("button")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("asChild prop이 true일 때 전달된 props가 자식 요소에 전달되어야 합니다", () => { |
There was a problem hiding this comment.
설명 부분에서 자식 요소에 전달보다는 자식 요소의 props와 병합이라는 텍스트가 더 적합해보입니다
| @@ -0,0 +1,11 @@ | |||
| export function composeRefs<T>(...refs: Array<React.Ref<T> | undefined>): React.RefCallback<T> { | |||
There was a problem hiding this comment.
이 부분 undefined를 추가해주신 이유를 알 수 있을까요?
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| type AnyProps = Record<string, any>; |
There was a problem hiding this comment.
이 eslint 주석 부분도 나중에 지워주시면 좋을 거 같네요
| console.log(mergeClassNames(parentValue, childValue)); | ||
| result.className = mergeClassNames(parentValue, childValue); |
There was a problem hiding this comment.
불필요한 콘솔문은 지워주시면 좋을 거 같아요
|
|
||
| export interface SlotCloneProps {} |
There was a problem hiding this comment.
SlotClone이라는 네이밍으로 지으신 이유가 있을까요?
ComposedChildProps라는 네이밍이 더 좋아보이긴 합니다
|
|
||
| export { ComposedChild }; |
There was a problem hiding this comment.
divider에서는 export default 방식으로 내보내주고 있던데 이 부분도 통일해보면 좋을 거 같습니다
There was a problem hiding this comment.
divider와 md 문서 작성하는 방식을 통일시킬 필요가 있을 것 같습니다! 아직 합의된 방식이 없어 추후 진행될 회의에서 합의하거나 해당 코드 리뷰에서 합의해야할 것 같은데 다들 어떻게 생각하세요?
@minai621 @ghdtjgus76 @gs0428
There was a problem hiding this comment.
- 엔터로 줄바꿈 vs 이어 적기
- 설치 방법 코드 개별 제공 vs 한 코드 블럭으로 제공
- md 목차
예상 동작 (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 스토리가 추가되었습니다.Bug Fixes
Documentation
@warrr-ui/primitive-slot패키지에 대한 문서가 추가되었습니다.Slot컴포넌트의 사용법과 예시가 포함된 스토리가 추가되었습니다.