[FEAT] Modal 컴포넌트, 스토리북 제작 #19
Conversation
|
Warning Rate limit exceeded@ujinsim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" 워크스루이 풀 리퀘스트는 모달 컴포넌트와 관련된 새로운 기능을 추가합니다. 변경 사항
시퀀스 다이어그램sequenceDiagram
participant User
participant Button
participant Modal
participant Portal
User->>Button: 클릭
Button->>Modal: isOpen = true
Modal->>Portal: 렌더링
Portal->>Document: 모달 표시
User->>Modal: 외부 클릭
Modal->>Modal: closeModal() 호출
Modal->>Portal: 모달 숨김
연결된 이슈 평가
관련 가능한 풀 리퀘스트
추천 리뷰어
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
Documentation and Community
|
|
Update: 2025년 02월 01일 22시 27분 36초 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/shared/ui/Modal/useOutsideClick.ts (1)
7-11: 이벤트 타입 안전성 개선이 필요합니다MouseEvent 타입의 target을 Node로 캐스팅하기 전에 EventTarget 타입인지 확인하는 것이 좋습니다.
const handleClick = (e: MouseEvent) => { - if (ref.current && !ref.current.contains(e.target as Node)) { + if (ref.current && e.target instanceof Node && !ref.current.contains(e.target)) { callback(); } };src/shared/ui/Modal/Modal.tsx (2)
29-33: 중복된 onClick 이벤트 핸들러를 제거해주세요두 개의 div 요소에서 동일한 e.stopPropagation() 호출이 중복되어 있습니다. 상위 div에서만 처리하는 것으로 충분합니다.
<div ref={modalRef} onClick={(e) => e.stopPropagation()} className="flex items-center justify-center" > <motion.div className="w-full max-w-4xl flex-col items-center rounded-xl bg-white p-10 text-4xl text-black shadow-xl md:h-[70%]" - onClick={(e) => e.stopPropagation()} initial={{ scale: 0.8 }} animate={{ scale: 1 }} exit={{ scale: 0.8 }} transition={{ duration: 0.3 }} >Also applies to: 34-41
35-35: 스타일 관리 방식 개선이 필요합니다하드코딩된 Tailwind 클래스들이 너무 길어 가독성이 떨어집니다. 별도의 상수나 설정 파일로 분리하는 것을 고려해보세요.
// styles.ts export const MODAL_CONTENT_STYLES = { base: "w-full flex-col items-center rounded-xl bg-white p-10 text-black shadow-xl", size: "max-w-4xl text-4xl md:h-[70%]" }; // Modal.tsx에서 사용 className={`${MODAL_CONTENT_STYLES.base} ${MODAL_CONTENT_STYLES.size}`}src/shared/ui/Modal/Modal.stories.tsx (2)
30-34: openModal 함수의 이벤트 타입이 명시되어 있지 않습니다TypeScript의 타입 안전성을 위해 이벤트 파라미터의 타입을 명시해주세요.
- const openModal = (e) => { + const openModal = (e: React.MouseEvent<HTMLButtonElement>) => {
53-60: 인라인 스타일을 CSS 모듈이나 Tailwind로 마이그레이션하는 것이 좋습니다스토리북에서 인라인 스타일을 사용하고 있는데, 프로젝트의 일관성을 위해 CSS 모듈이나 Tailwind 클래스를 사용하는 것이 좋습니다.
예시:
// Modal.stories.module.css 또는 Tailwind 클래스 사용 const buttonClasses = "bg-gray-400 text-white px-3 py-1.5 border border-white rounded";Also applies to: 81-91
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.storybook/preview-body.html(1 hunks)package.json(1 hunks)src/shared/ui/Modal/Modal.stories.tsx(1 hunks)src/shared/ui/Modal/Modal.tsx(1 hunks)src/shared/ui/Modal/useOutsideClick.ts(1 hunks)src/shared/ui/Portal/Portal.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .storybook/preview-body.html
🔇 Additional comments (2)
src/shared/ui/Modal/Modal.stories.tsx (1)
9-22: argTypes에 누락된 props가 있습니다Modal 컴포넌트의 모든 props가 argTypes에 정의되어 있지 않습니다. isOpen, closeModal, modalRef도 컨트롤할 수 있도록 추가하는 것이 좋습니다.
package.json (1)
21-21: framer-motion 버전 업데이트의 호환성 및 보안 검증이 필요합니다.framer-motion을 11.15.0에서 11.18.2로 업데이트하셨네요. Modal 컴포넌트에 사용될 애니메이션 기능과 관련하여 버전 업데이트가 필요했을 것으로 보입니다.
다음 스크립트를 실행하여 버전 업데이트와 관련된 잠재적인 문제를 확인해주세요:
✅ Verification successful
framer-motion 버전 업데이트가 안전하게 확인되었습니다.
11.18.2 버전은 안정적인 마이너 업데이트이며, 의존성 호환성 문제나 보안 취약점이 발견되지 않았습니다. Modal 컴포넌트 구현을 위한 적절한 선택입니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: framer-motion 버전 업데이트 관련 호환성 및 보안 이슈 확인 # 최신 버전 및 취약점 정보 확인 echo "framer-motion 최신 버전 정보 확인 중..." npm view framer-motion versions --json | jq -r '.[-5:]' # 의존성 호환성 검사 echo "의존성 호환성 검사 중..." npm view [email protected] peerDependencies --json # GitHub 보안 권고 확인 echo "보안 권고 확인 중..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "framer-motion") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 803
| useEffect(() => { | ||
| document.addEventListener('click', handleClick); | ||
| return () => { | ||
| document.removeEventListener('click', handleClick); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
useEffect의 의존성 배열이 누락되었습니다
useEffect의 의존성 배열이 비어있지 않아 불필요한 리렌더링이 발생할 수 있습니다. 또한 ref나 callback이 변경될 때마다 이벤트 리스너를 다시 등록해야 합니다.
useEffect(() => {
document.addEventListener('click', handleClick);
return () => {
document.removeEventListener('click', handleClick);
};
- });
+ }, [ref, callback]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/shared/ui/Modal/Modal.tsx (2)
18-28: 애니메이션 설정을 커스터마이즈 가능하도록 개선해보세요.현재 구현은 잘 되어있지만, 다음 사항들을 고려해보시면 좋겠습니다:
- 애니메이션 duration을 props로 받아 커스터마이즈 가능하도록 하기
- z-index 값(30)을 상수로 분리하여 관리하기
+ type ModalProps = { + ... + animationDuration?: number; + }; - transition={{ duration: 0.3 }} + transition={{ duration: animationDuration ?? 0.3 }}
29-41: 스타일링 옵션을 더 유연하게 만들어보세요.현재 모달의 스타일링이 고정되어 있습니다. 다음과 같은 개선사항을 제안드립니다:
type ModalProps = { ... + className?: string; }; - className="items-center justify-center rounded-lg shadow-lg" + className={`items-center justify-center rounded-lg shadow-lg ${className ?? ''}`}src/shared/ui/Modal/Modal.stories.tsx (2)
1-10: 스토리북 설정을 더 풍부하게 만들어보세요.현재 기본적인 설정만 되어있습니다. argTypes를 추가하여 더 다양한 상호작용이 가능하도록 개선해보세요:
export default { title: 'components/common/Modal', component: Modal, + argTypes: { + className: { control: 'text' }, + animationDuration: { control: 'number', defaultValue: 0.3 }, + }, } as Meta<typeof Modal>;
34-46: 모달 컨텐츠 구현을 개선해보세요.다음과 같은 개선사항들을 제안드립니다:
- 하드코딩된 텍스트를 상수로 분리
- 스타일 클래스를 상수로 분리
- 레이아웃 시프트 방지를 위한 AnimatePresence 활용
+ const MODAL_TEXTS = { + title: 'dding-dong 모달입니다', + closeButton: '모달닫기', + }; + const MODAL_STYLES = { + container: 'flex flex-col items-center gap-4 rounded-lg bg-white p-6', + title: 'p-2 text-2xl font-semibold', + closeButton: 'rounded-lg bg-red-200 px-4 py-2 font-bold text-white', + }; - {isOpen && ( + <AnimatePresence> + {isOpen && ( <Modal isOpen={isOpen} closeModal={closeModal} modalRef={modalRef} {...args}> - <div className="flex flex-col items-center gap-4 rounded-lg bg-white p-6"> + <div className={MODAL_STYLES.container}> - <div className="p-2 text-2xl font-semibold">dding-dong 모달입니다</div> + <div className={MODAL_STYLES.title}>{MODAL_TEXTS.title}</div> <button onClick={closeModal} - className="rounded-lg bg-red-200 px-4 py-2 font-bold text-white" + className={MODAL_STYLES.closeButton} > - 모달닫기 + {MODAL_TEXTS.closeButton} </button> </div> </Modal> - )} + )} + </AnimatePresence>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/shared/ui/Modal/Modal.stories.tsx(1 hunks)src/shared/ui/Modal/Modal.tsx(1 hunks)src/shared/ui/Portal/Portal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/ui/Portal/Portal.tsx
🔇 Additional comments (2)
src/shared/ui/Modal/Modal.tsx (2)
1-13: 잘 구성된 타입 정의와 임포트 구조입니다!모달 구현에 필요한 의존성들이 잘 구성되어 있으며, Props 타입이 명확하게 정의되어 있습니다.
15-17: modalRef가 null일 경우에 대한 처리가 필요합니다.useOutsideClick 훅을 사용할 때 modalRef가 null인 경우에 대한 처리가 없습니다.
다음과 같이 modalRef의 유효성을 검사하는 것이 좋습니다:
- useOutsideClick(modalRef, closeModal); + if (modalRef.current) { + useOutsideClick(modalRef, closeModal); + }
| const openModal = (e) => { | ||
| e.stopPropagation(); | ||
| setIsOpen(true); | ||
| }; | ||
|
|
||
| const closeModal = () => { | ||
| setIsOpen(false); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이벤트 핸들러에 타입 안전성을 추가해주세요.
openModal 함수의 이벤트 파라미터에 타입이 지정되어 있지 않습니다.
- const openModal = (e) => {
+ const openModal = (e: React.MouseEvent<HTMLButtonElement>) => {📝 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 openModal = (e) => { | |
| e.stopPropagation(); | |
| setIsOpen(true); | |
| }; | |
| const closeModal = () => { | |
| setIsOpen(false); | |
| }; | |
| const openModal = (e: React.MouseEvent<HTMLButtonElement>) => { | |
| e.stopPropagation(); | |
| setIsOpen(true); | |
| }; | |
| const closeModal = () => { | |
| setIsOpen(false); | |
| }; |
| </button> | ||
|
|
||
| {isOpen && ( | ||
| <Modal isOpen={isOpen} closeModal={closeModal} modalRef={modalRef} {...args}> |
There was a problem hiding this comment.
| <Modal isOpen={isOpen} closeModal={closeModal} modalRef={modalRef} {...args}> | |
| <Modal isOpen={isOpen} closeModal={closeModal} modalRef={modalRef} > |
'modalRef'이(가) 두 번 이상 지정되어 이 사용량을 덮어씁니다. 에러가 발생하고 있어서 하나는 제거를 해야될 것 같아요. modalRef도 형식에러가 노출됩니다 ㅠ
There was a problem hiding this comment.
컨벤션을 지키지 않아 수정하였습니다 감사합니다
e25ef6b
| const [isOpen, setIsOpen] = useState(false); | ||
| const modalRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const openModal = (e) => { |
There was a problem hiding this comment.
| const openModal = (e) => { | |
| const openModal = (e: React.MouseEvent<HTMLButtonElement>) => { |
ts설정이 누락되어져있네용. 모달 상태나 모달 상태를 제어하는 함수는 커스텀 훅을 통해 관리해도 좋을 것 같아요.
src/shared/ui/Modal/Modal.tsx
Outdated
|
|
||
| import useOutsideClick from './useOutsideClick'; | ||
|
|
||
| import ModalPortal from '../Portal/Portal'; |
src/shared/ui/Modal/Modal.tsx
Outdated
| animate={{ opacity: 1 }} | ||
| exit={{ opacity: 0 }} | ||
| transition={{ duration: 0.3 }} | ||
| className="fixed inset-0 z-30 flex w-full items-center justify-center" |
There was a problem hiding this comment.
보통 모달이나 팝업창이 뜨는 경우 배경으로부터 팝업을 강조하기 위해 dimmed 설정을 하는 경우가 많아요. 요 부분은 디자이너님이랑 상의해 보신걸까요??
현재는 shadow가 하위 영역에만 짙어서 모달과 그 외 부분들이 경계가 잘 안 나타날 것 같아요!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/shared/ui/Portal/Portal.tsx (1)
10-13: SSR 환경에서의 에러 처리가 필요합니다현재 SSR 환경에서는 단순히 null을 반환하고 있습니다. 개발 과정에서의 디버깅을 위해 더 명확한 에러 처리가 필요합니다.
다음과 같이 개선하는 것을 추천드립니다:
if (typeof window === 'undefined' || !isOpen) { + if (typeof window === 'undefined') { + console.warn('Portal is not supported in SSR environment'); + } return null; }src/shared/ui/Modal/Modal.tsx (2)
8-13: Props 타입 정의 개선이 필요합니다.
modeprop이 AnimatePresence의 mode와 직접적으로 연관되어 있으므로, 타입을 더 명확하게 정의하면 좋을 것 같습니다.type Props = { isOpen: boolean; closeModal: () => void; children: React.ReactNode; - mode?: 'wait' | 'sync' | 'popLayout'; + mode?: AnimatePresenceProps['mode']; };
15-17: 모달 컴포넌트에 유용한 props를 추가해보세요.이전 리뷰 의견을 반영하여,
onClickOutside로 prop 이름을 변경하고 추가적인 커스터마이징을 위한 props를 제안드립니다.type Props = { isOpen: boolean; - closeModal: () => void; + onClickOutside: () => void; + className?: string; children: React.ReactNode; mode?: AnimatePresenceProps['mode']; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/shared/constants/motion.ts(1 hunks)src/shared/ui/Modal/Modal.stories.tsx(1 hunks)src/shared/ui/Modal/Modal.tsx(1 hunks)src/shared/ui/Modal/index.ts(1 hunks)src/shared/ui/Portal/Portal.tsx(1 hunks)src/shared/ui/Portal/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/shared/ui/Portal/index.tsx
- src/shared/constants/motion.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/ui/Modal/index.ts
- src/shared/ui/Modal/Modal.stories.tsx
🔇 Additional comments (4)
src/shared/ui/Portal/Portal.tsx (3)
1-8: 타입 정의가 명확하고 깔끔합니다!Props 인터페이스가 잘 정의되어 있으며, mode 타입이 구체적으로 지정되어 있습니다.
15-15: 🛠️ Refactor suggestiondocument.body 대신 전용 portal 컨테이너를 사용하는 것이 좋습니다
document.body를 직접 사용하면 스타일 충돌이나 다른 포털과의 충돌이 발생할 수 있습니다.
다음과 같이 전용 컨테이너를 생성하고 사용하는 것을 추천드립니다:
- const container = document.body as HTMLElement; + const portalRoot = document.getElementById('portal-root'); + if (!portalRoot) { + const element = document.createElement('div'); + element.id = 'portal-root'; + document.body.appendChild(element); + } + const container = document.getElementById('portal-root') as HTMLElement;Likely invalid or redundant comment.
10-21: 🛠️ Refactor suggestion컴포넌트 언마운트 시 정리(cleanup) 로직이 필요합니다
포털용 컨테이너를 동적으로 생성한 경우, 컴포넌트가 언마운트될 때 이를 정리하는 로직이 필요합니다.
useEffect를 사용하여 다음과 같이 cleanup 로직을 추가하는 것을 추천드립니다:
export function Portal({ isOpen, mode = 'sync', children }: Props) { + useEffect(() => { + const element = document.createElement('div'); + element.id = 'portal-root'; + document.body.appendChild(element); + + return () => { + document.body.removeChild(element); + }; + }, []); if (typeof window === 'undefined' || !isOpen) { return null; } - const container = document.body as HTMLElement; + const container = document.getElementById('portal-root') as HTMLElement; return ReactDom.createPortal( <AnimatePresence mode={mode}>{children}</AnimatePresence>, container ); }Likely invalid or redundant comment.
src/shared/ui/Modal/Modal.tsx (1)
36-44: 🛠️ Refactor suggestion모달 백드롭 스타일링이 필요합니다.
이전 리뷰에서 언급된 것처럼, 모달의 dimmed 효과가 부족합니다. 백드롭에 적절한 스타일을 추가하여 모달을 더 강조해야 합니다.
<motion.div {...MODAL_MOTION} - className="fixed inset-0 z-30 flex w-full items-center justify-center" + className="fixed inset-0 z-30 flex w-full items-center justify-center bg-black/50" onClick={handleBackdropClick} > <div ref={modalRef} - onClick={(e) => e.stopPropagation()} className="items-center justify-center rounded-lg shadow-lg" >Likely invalid or redundant comment.
🔥 연관 이슈
🚀 작업 내용
🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
새로운 기능
종속성 업데이트
framer-motion라이브러리를 버전 11.18.2로 업그레이드개선 사항