feat: ✨ LevelModal 컴포넌트 이관 #98
Conversation
| if (!levelData) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
levelData는 필수값인데 해당 로직은 필요없을 것 같습니다.
levelData이 실제로 optional 한 데이터라면 타입을 optional로 변경하거나, 외부에서 levelData라는 타입을 정확히 명시해
해당 데이터가 있어야 LevelModal 컴포넌트를 렌더링해주는게 좀 더 type safe한것 같습니다.
그리고 levelData라는 변수명도 다른 pr comment와 마찬가지로 array인지 알 수 있는 변수명이면 좋을것 같고 data라는 postfix없이도 의미는 명확하게 지을 수 있을 것 같아요
| const CloseIcon = styled(Icon)` | ||
| cursor: pointer; | ||
| `; |
There was a problem hiding this comment.
전체적으로 Icon에 cursor: pointer 스타일을 오버라이딩해서 사용하는 경우가 많은것 같은데, 이러면 그냥 cursor props를 하나 추가해도 좋을 것 같습니다
src/components/Modal/Modal.tsx
Outdated
| openModal?: () => void; | ||
| closeModal?: () => void; | ||
| open: boolean; | ||
| setOpen?: () => void; |
There was a problem hiding this comment.
openModal과 setOpen은 불필요한 prop인 것 같고, 코드상에 사용되는 부분이 없어보이는데
전달받아야하는 이유가 있나요??
There was a problem hiding this comment.
Modal 컴포넌트를 옮기는 과정에서 중복되는 prop이 생긴것 같아 정리하였습니다!
불필요하게 중복되는 prop은 제거하고, 네이밍을 통일했습니다
a292574
| {title && <Title>{title}</Title>} | ||
| {description && <Description>{description}</Description>} | ||
| {buttons && <ButtonContainer>{buttons}</ButtonContainer>} | ||
| {noMoreSee && <NoMoreSee onClick={closeModal}>다시 보지 않기</NoMoreSee>} |
There was a problem hiding this comment.
다시보지 않기의 onClick 이벤트가 closeModal만 들어가는것이 맞을까용..?
스토리지에 상태 저장 등 다른 로직이 포함될 여지가 있어보여서 onNoMoreSee Prop을 추가하거나
아예 공통 Modal 컴포넌트에서는 이부분이 없어도 될것같아요 (필요할때만 children으로 추가)
There was a problem hiding this comment.
넵 noMoreSee prop은 공통 Modal에 있기에 적합하지 않다는 의견에 동의합니다
noMoreSee 는 홈 화면의 모달에서만 사용되니 Modal 컴포넌트를 래핑한 홈 화면 전용 모달을 따로 만드는게 좋을 것 같아요
이 PR은 LevelModal 컴포넌트를 만들면서 필요한 Modal 컴포넌트를 이관한 것이니, 홈 화면 전용 모달을 만들때 NoMoreSee 부분을 리팩토링하면 좋을 것 같아요
There was a problem hiding this comment.
넵넵! 이슈나 투두 주석 작성해두면 좋을 것 같아요~
There was a problem hiding this comment.
#104 (comment) 추가했어요!
| @@ -14,13 +12,19 @@ interface Props { | |||
| } | |||
|
|
|||
| const LevelModal = ({ isLevelModalOpen, closeLevelModal, levels }: Props) => { | |||
| const [isModalOpen, setIsModalOpen] = useState(true); | |||
There was a problem hiding this comment.
여기 내부의 isModalOpen과 isLevelModalOpen상태는 어떻게 다른건가요??
|
효진님 스토리북에서 에러발생하나보네요! @hy57in |
📍 주요 변경사항
🔗 참고자료
💡 중점적으로 봐주었으면 하는 부분