feat(tooltip): Tooltip 컴포넌트 구현 #43
Conversation
developerjhp
requested review from
kimdaeyeobbb,
hy57in,
bae-sh,
noahluftyang,
froggy1014,
heeji289 and
synuns
as code owners
|
developerjhp
force-pushed
the
feat/tooltip
branch
from
5504bf3 to
1019dbc
Compare
developerjhp
force-pushed
the
feat/tooltip
branch
from
1019dbc to
c280183
Compare
developerjhp
force-pushed
the
feat/tooltip
branch
from
711788a to
a22a71f
Compare
…tional rendering for visibility
…n Tooltip component
…ape key detection
| "@testing-library/user-event": "^14.5.2", | ||
| "@types/react-dom": "^18.3.1", |
There was a problem hiding this comment.
두 패키지는 개발 단계에서만 사용하는 의존성이라서 devDependencies로 이동해줘~
| "@testing-library/user-event": "^14.5.2", | ||
| "@types/react-dom": "^18.3.1", | ||
| "clsx": "^2.1.1", | ||
| "react-dom": "^18.3.1" |
There was a problem hiding this comment.
react와 react-dom은 단일 인스턴스만 존재해야하기 때문에 peerDependencies로 옮겨줘야해~
| export type TooltipTrigger = 'hover' | 'click'; | ||
|
|
||
| export interface TooltipProps extends ComponentProps<'div'> { | ||
| tooltipContent: ReactNode; |
There was a problem hiding this comment.
tooltipContent와 tooltipStyle, tooltipClassName은 굳이 개별 props로 분리하는 것 보다 합성을 활용한 방식으로 풀면 확장성에 용이할 것 같아!
| if (!tooltipContent) { | ||
| return <>{children}</>; | ||
| } |
There was a problem hiding this comment.
tooltipContent가 없는 채로 툴팁을 사용하는 유즈케이스가 있을까 싶은데 이 부분은 구현에서 제외하면 어떨까?
| @@ -0,0 +1 @@ | |||
| export * from './useTooltip'; | |||
There was a problem hiding this comment.
배럴 파일(모듈을 re export해주는 파일)은 자바스크립트 성능에 영향을 줄 수 있는 요소라서 지양했으면 좋겠어!
https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-7/
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Walkthrough이 변경 사항은 Changes
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: 9
🧹 Outside diff range and nitpick comments (5)
packages/tooltip/src/Tooltip.tsx (1)
29-29: 'tooltipContent' 속성을 선택적으로 변경하세요'TooltipProps' 인터페이스에서 'tooltipContent'가 필수로 정의되어 있지만, 컴포넌트에서는 해당 속성이 없는 경우를 처리하고 있습니다. 따라서 'tooltipContent'를 선택적 속성으로 변경하는 것이 일관성에 좋을 듯합니다.
packages/tooltip/src/Tooltip.stories.tsx (3)
32-40: 템플릿의 유연성 개선이 필요합니다템플릿에 하드코딩된 값들이 재사용성을 제한할 수 있습니다:
- 고정된 마진 값 (
margin: '100px')- 하드코딩된 버튼 텍스트 ("Hover Me")
다음과 같이 수정하는 것을 제안합니다:
const Template: StoryFn<TooltipProps> = (args) => ( - <div style={{ margin: '100px', textAlign: 'center' }}> + <div style={{ margin: args.containerMargin ?? '100px', textAlign: 'center' }}> <Tooltip {...args}> <button type="button" style={{ padding: '8px 12px', cursor: 'pointer' }}> - Hover Me + {args.buttonText ?? 'Hover Me'} </button> </Tooltip> </div> );그리고
argTypes에 다음 속성들을 추가하세요:containerMargin: { control: 'text', description: '컨테이너의 마진 값' }, buttonText: { control: 'text', description: '버튼에 표시될 텍스트' }
42-82: 중복된 스타일 코드를 개선할 수 있습니다여러 스토리에서 비슷한 스타일 객체가 반복되고 있습니다. 특히
backgroundColor,color,padding,borderRadius등의 기본 스타일이 중복됩니다.공통 스타일을 추출하여 다음과 같이 사용하는 것을 제안합니다:
+ const baseTooltipStyle = { + backgroundColor: '#333', + color: '#fff', + padding: '8px 12px', + borderRadius: '4px', + }; export const Default = Template.bind({}); Default.args = { tooltipContent: 'This is a tooltip', placement: 'top', trigger: 'hover', - tooltipStyle: { - backgroundColor: '#333', - color: '#fff', - padding: '8px 12px', - borderRadius: '4px', - }, + tooltipStyle: baseTooltipStyle, };이렇게 하면 스타일 일관성을 유지하고 코드 중복을 줄일 수 있습니다.
84-102: 스타일링 방식을 개선할 수 있습니다현재 인라인 스타일을 사용하고 있는데, 이는 유지보수와 재사용성 측면에서 제한적일 수 있습니다.
CSS 모듈이나 styled-components를 사용하여 다음과 같이 개선하는 것을 제안합니다:
+ import styles from './Tooltip.stories.module.css'; // 또는 + import styled from 'styled-components'; + + const ExampleContainer = styled.div` + display: flex; + gap: 20px; + padding: 50px; + text-align: center; + `; export const AsChildExample = () => ( - <div - style={{ - display: 'flex', - gap: '20px', - padding: '50px', - textAlign: 'center', - }} - > + <ExampleContainer> {/* ... */} - </div> + </ExampleContainer> );packages/tooltip/src/Tooltip.test.tsx (1)
293-295: Tooltip 컨테이너의 역할과 태그 검증을 재고하세요
getByRole('tooltip')로 선택한 요소의tagName을 검증하고 있습니다. 그러나 요소의 태그명은 구현 방식에 따라 변경될 수 있으므로, 태그명을 검증하는 것은 권장되지 않습니다. 대신 요소의 존재 여부나 역할에 집중하는 것이 좋습니다.- expect(wrapperElement.tagName).toBe('DIV');
📜 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 (16)
packages/tooltip/.storybook/main.ts(1 hunks)packages/tooltip/.storybook/preview.ts(1 hunks)packages/tooltip/package.json(1 hunks)packages/tooltip/src/Tooltip.module.css(1 hunks)packages/tooltip/src/Tooltip.stories.tsx(1 hunks)packages/tooltip/src/Tooltip.test.tsx(1 hunks)packages/tooltip/src/Tooltip.tsx(1 hunks)packages/tooltip/src/env.d.ts(1 hunks)packages/tooltip/src/hooks/useTooltip/index.ts(1 hunks)packages/tooltip/src/hooks/useTooltip/useTooltip.md(1 hunks)packages/tooltip/src/hooks/useTooltip/useTooltip.tsx(1 hunks)packages/tooltip/src/index.ts(1 hunks)packages/tooltip/tsconfig.json(1 hunks)packages/tooltip/tsup.config.ts(1 hunks)packages/tooltip/vitest.config.ts(1 hunks)packages/tooltip/vitest.setup.ts(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- packages/tooltip/src/index.ts
- packages/tooltip/src/hooks/useTooltip/index.ts
- packages/tooltip/vitest.setup.ts
- packages/tooltip/src/env.d.ts
- packages/tooltip/tsconfig.json
- packages/tooltip/.storybook/preview.ts
- packages/tooltip/tsup.config.ts
- packages/tooltip/package.json
- packages/tooltip/vitest.config.ts
- packages/tooltip/src/Tooltip.module.css
🔇 Additional comments (1)
packages/tooltip/src/Tooltip.tsx (1)
51-53: 'tooltipContent'가 없는 경우 Tooltip 컴포넌트를 렌더링하지 않도록 고려하세요
'tooltipContent'가 없는 상황에서 Tooltip을 사용하는 유즈케이스가 드물어 보입니다. 이 부분은 구현에서 제외하는 것이 좋을 것 같습니다.
| <> | ||
| <Component | ||
| ref={wrapperRef} | ||
| role="tooltip" |
There was a problem hiding this comment.
'role="tooltip"' 속성을 툴팁 요소로 이동하세요
현재 'role="tooltip"' 속성이 트리거 요소에 적용되어 있습니다. WAI-ARIA 표준에 따르면, 'role="tooltip"'은 실제 툴팁 요소에 적용되어야 합니다. 트리거 요소에는 'aria-describedby'를 사용하여 툴팁을 참조하도록 수정해주세요.
| handleKeyDown, | ||
| } = useTooltip({ placement, gap, trigger }); | ||
|
|
||
| useImperativeHandle(ref, () => wrapperRef.current as HTMLElement); |
There was a problem hiding this comment.
'wrapperRef.current'의 null 여부를 확인하세요
'useImperativeHandle'에서 'wrapperRef.current'를 'HTMLElement'로 캐스팅하고 있습니다. 하지만 'wrapperRef.current'가 null일 수 있으므로, null 체크를 통해 안전하게 캐스팅해야 합니다.
|
|
||
| const handleKeyDown = (event: ReactKeyboardEvent<HTMLDivElement>) => { | ||
| if ( | ||
| (event.code === 'Enter' || event.code === 'space') && |
There was a problem hiding this comment.
'event.code'에서 'Space'의 대소문자를 확인하세요
'handleKeyDown' 함수에서 Spacebar를 감지하기 위해 'event.code === 'space''로 되어 있는데, 실제로는 'event.code === 'Space''로 대문자를 사용해야 합니다.
|
|
||
| ## Parameters | ||
|
|
||
| - `placement`: 'top' | 'bottom' | 'left' | 'right' (필수) - 툴팁이 나타날 위치를 지정합니다. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
'placement' 파라미터의 모든 가능한 값을 문서화하세요
현재 문서에서는 'placement'의 값으로 'top', 'bottom', 'left', 'right'만 나열되어 있습니다. 하지만 컴포넌트는 'top-left', 'top-right', 'bottom-left', 'bottom-right' 등의 값도 지원합니다. 모든 가능한 값을 포함하여 사용자가 정확한 정보를 얻을 수 있도록 해주세요.
| export const AllPlacements = () => ( | ||
| <div | ||
| style={{ | ||
| display: 'grid', | ||
| gridTemplateColumns: 'repeat(2, 1fr)', | ||
| gap: '20px', | ||
| padding: '50px', | ||
| }} | ||
| > | ||
| <Tooltip tooltipContent="Top Left" placement="top-left"> | ||
| <button type="button">Top Left</button> | ||
| </Tooltip> | ||
| <Tooltip tooltipContent="Top Left" placement="top-left" asChild={false}> | ||
| <button type="button">Top Left</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Top" placement="top"> | ||
| <button type="button">Top</button> | ||
| </Tooltip> | ||
| <Tooltip tooltipContent="Top" placement="top" asChild={false}> | ||
| <button type="button">Top</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Top Right" placement="top-right"> | ||
| <button type="button">Top Right</button> | ||
| </Tooltip> | ||
| <Tooltip tooltipContent="Top Right" placement="top-right" asChild={false}> | ||
| <button type="button">Top Right</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Left" placement="left"> | ||
| <button type="button">Left</button> | ||
| </Tooltip> | ||
| <Tooltip tooltipContent="Left" placement="left" asChild={false}> | ||
| <button type="button">Left</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Right" placement="right"> | ||
| <button type="button">Right</button> | ||
| </Tooltip> | ||
| <Tooltip tooltipContent="Right" placement="right" asChild={false}> | ||
| <button type="button">Right</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Bottom Left" placement="bottom-left"> | ||
| <button type="button">Bottom Left</button> | ||
| </Tooltip> | ||
| <Tooltip | ||
| tooltipContent="Bottom Left" | ||
| placement="bottom-left" | ||
| asChild={false} | ||
| > | ||
| <button type="button">Bottom Left</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Bottom" placement="bottom"> | ||
| <button type="button">Bottom</button> | ||
| </Tooltip> | ||
| <Tooltip tooltipContent="Bottom" placement="bottom" asChild={false}> | ||
| <button type="button">Bottom</button> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip tooltipContent="Bottom Right" placement="bottom-right"> | ||
| <button type="button">Bottom Right</button> | ||
| </Tooltip> | ||
| <Tooltip | ||
| tooltipContent="Bottom Right" | ||
| placement="bottom-right" | ||
| asChild={false} | ||
| > | ||
| <button type="button">Bottom Right</button> | ||
| </Tooltip> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
코드 중복을 줄일 수 있습니다
현재 각 배치 옵션에 대해 비슷한 코드가 반복되고 있으며, asChild={false} 케이스도 중복되어 있습니다.
배열과 매핑을 사용하여 다음과 같이 개선하는 것을 제안합니다:
const PLACEMENTS = [
{ value: 'top-left', label: 'Top Left' },
{ value: 'top', label: 'Top' },
{ value: 'top-right', label: 'Top Right' },
{ value: 'left', label: 'Left' },
{ value: 'right', label: 'Right' },
{ value: 'bottom-left', label: 'Bottom Left' },
{ value: 'bottom', label: 'Bottom' },
{ value: 'bottom-right', label: 'Bottom Right' },
];
export const AllPlacements = () => (
<div style={{
display: 'grid',
gridTemplateColumns: 'repeat(2, 1fr)',
gap: '20px',
padding: '50px',
}}>
{PLACEMENTS.map(({ value, label }) => (
<React.Fragment key={value}>
<Tooltip tooltipContent={label} placement={value}>
<button type="button">{label}</button>
</Tooltip>
<Tooltip tooltipContent={label} placement={value} asChild={false}>
<button type="button">{label}</button>
</Tooltip>
</React.Fragment>
))}
</div>
);이렇게 하면:
- 코드 중복이 줄어듭니다
- 새로운 배치 옵션 추가가 용이해집니다
- 유지보수가 더 쉬워집니다
| placement: { | ||
| control: { type: 'select' }, | ||
| options: ['top', 'bottom', 'left', 'right'], | ||
| description: '툴팁이 나타나는 위치', | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
placement 옵션이 불완전합니다
argTypes에 정의된 placement 옵션이 실제 스토리에서 사용되는 모든 옵션을 포함하고 있지 않습니다. 다음과 같은 옵션들이 누락되어 있습니다:
top-lefttop-rightbottom-leftbottom-right
다음과 같이 수정하는 것을 제안합니다:
placement: {
control: { type: 'select' },
- options: ['top', 'bottom', 'left', 'right'],
+ options: [
+ 'top',
+ 'top-left',
+ 'top-right',
+ 'bottom',
+ 'bottom-left',
+ 'bottom-right',
+ 'left',
+ 'right'
+ ],
description: '툴팁이 나타나는 위치',
},📝 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.
| placement: { | |
| control: { type: 'select' }, | |
| options: ['top', 'bottom', 'left', 'right'], | |
| description: '툴팁이 나타나는 위치', | |
| }, | |
| placement: { | |
| control: { type: 'select' }, | |
| options: [ | |
| 'top', | |
| 'top-left', | |
| 'top-right', | |
| 'bottom', | |
| 'bottom-left', | |
| 'bottom-right', | |
| 'left', | |
| 'right' | |
| ], | |
| description: '툴팁이 나타나는 위치', | |
| }, |
|
|
||
| const tooltip = await screen.findByText('This is a tooltip'); | ||
| expect(tooltip).toBeInTheDocument(); | ||
| expect(tooltip.className).toContain('top'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tooltip 위치 검증 방식을 개선하세요
현재 테스트에서 tooltip.className을 확인하여 위치를 검증하고 있습니다. CSS 모듈이나 스타일링 방식에 따라 클래스명이 변경될 수 있어 이 방식은 취약할 수 있습니다. 대신 데이터 속성(data-placement)을 활용하여 위치를 확인하는 것을 권장합니다.
- expect(tooltip.className).toContain('top');
+ expect(tooltip).toHaveAttribute('data-placement', 'top');📝 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.
| expect(tooltip.className).toContain('top'); | |
| expect(tooltip).toHaveAttribute('data-placement', 'top'); |
|
|
||
| const tooltip = await screen.findByText('Tooltip content'); | ||
| expect(tooltip).toBeInTheDocument(); | ||
| expect(tooltip.className).toContain(placement as string); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tooltip 위치 검증 방식을 개선하세요
현재 테스트에서 tooltip.className을 사용하여 위치를 검증하고 있습니다. CSS 모듈이나 스타일링 방식에 따라 클래스명이 변경될 수 있어 이 방식은 취약할 수 있습니다. 대신 데이터 속성(data-placement)을 활용하여 위치를 확인하는 것을 권장합니다.
- expect(tooltip.className).toContain(placement as string);
+ expect(tooltip).toHaveAttribute('data-placement', placement as string);Committable suggestion skipped: line range outside the PR's diff.
| '@storybook/addon-onboarding', | ||
| '@storybook/addon-links', | ||
| '@storybook/addon-essentials', | ||
| '@chromatic-com/storybook', |
There was a problem hiding this comment.
Chromatic 애드온의 패키지명을 확인하세요
Chromatic 애드온의 패키지명으로 '@chromatic-com/storybook'을 사용하고 있습니다. 그러나 올바른 패키지명은 '@chromaui/storybook'입니다. 패키지명을 확인하고 수정해 주세요.
- '@chromatic-com/storybook',
+ '@chromaui/storybook',📝 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.
| '@chromatic-com/storybook', | |
| '@chromaui/storybook', |
변경사항
시각자료
체크리스트
추가 논의사항
Summary by CodeRabbit
릴리스 노트
새로운 기능
문서화
useTooltip훅에 대한 사용 문서 추가.테스트