[feat] Modal 컴포넌트 제작#32
Hidden character warning
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR 검증 결과✅ TypeScript: 통과 |
KyeongJooni
left a comment
There was a problem hiding this comment.
수고하셨습니다! 코멘트 확인해주시면 좋을 것 같아요!!
There was a problem hiding this comment.
BannerCard에도 달았던 코멘트긴 하지만 variant가 없는데 tv 의존성을 사용할 필요가 없을 것 같습니다! 한번 확인해보시면 좋을 것 같아요
There was a problem hiding this comment.
variant 사용 안 해 Modal.style.ts파일로 새로 만들었습니다!
| <Button variant="outline" size="auto" className="w-full" onClick={onCancel}> | ||
| 취소 | ||
| </Button> | ||
| </div> | ||
|
|
||
| <div className={buttonWrapper()}> | ||
| <Button variant="fill" size="auto" className="w-full" onClick={onConfirm}> | ||
| 삭제 | ||
| </Button> |
There was a problem hiding this comment.
현재 취소 삭제 버튼을 하드코딩했지만 범용성을 위해 cancelText, confirmText props 로 사용하는 방법은 어떨까요?
There was a problem hiding this comment.
범용 모달 충족 위해 말씀해주신대로 변경했습니다! 감사합니다
| overlay: 'fixed inset-0 z-50 flex items-center justify-center bg-black/40', | ||
|
|
||
| container: | ||
| 'w-[347px] h-[167px] rounded-[var(--radius-l)] bg-[var(--color-green-500)] px-[32px] py-[24px] flex flex-col justify-between', |
There was a problem hiding this comment.
현재 토큰값으로 py-[24px]은 padding-xl이 있는것 같은데 토큰값 사용안하신 이유가 있을까요? px는 토큰값이 없기 때문에 새로 추가해서 둘다 하드코딩말고 토큰값으로 사용하면 좋을 것 같습니다! 또한 추후에 fade-in/out 애니메이션 추가하면 더 UX를 챙길 수 있을 것 같습니다!
There was a problem hiding this comment.
토큰 새로 추가하고 집어 넣었습니다!
애미네이션 또한 추가하여 부드럽게 in/out 생겨 더 UX 친화적이 되었습니다!
| onCancel={() => setIsModalOpen(false)} | ||
| onConfirm={() => { | ||
| setIsModalOpen(false); | ||
| console.log('삭제 확정'); |
There was a problem hiding this comment.
불필요한 console.log는 삭제부탁드립니다!
|
|
||
| export const modalVariants = tv({ | ||
| slots: { | ||
| overlay: 'fixed inset-0 z-50 flex items-center justify-center bg-black/40', |
There was a problem hiding this comment.
피그마 디자인 상 이름이 컴포넌트 네이밍이 모달이긴 한데 와이어프레임을 보면 모달이 아닌 ConfirmPopup이나 Toast 느낌이 좀 더 강한 것 같아요 overlay가 필요한지 다시 확인해보시면 좋을 것 같습니다! 😄
PR 검증 결과✅ TypeScript: 통과 |
There was a problem hiding this comment.
현재 구현만 보면 버튼을 통한 닫기(onCancel)만 제공되는 것 같네용 UX 관점에서 ESC 키나 오버레이 클릭으로도 닫히는 동작이 있으면 좋을 것 같아서 참고용으로 리뷰 남깁니다!
There was a problem hiding this comment.
esc키나 오버레이 배경 클릭하게 해도 닫기 버튼 말씀하신 대로 변경 완료 했습니다!
좋은 아이디어 감사합니다!
PR 검증 결과✅ TypeScript: 통과 ESLint 상세
src/shared/ui/Modal/Modal.tsx
|
…l/LOOPIT-FE into 26-feat/모달-컴포넌트-제작
PR 검증 결과✅ TypeScript: 통과 ESLint 상세
src/shared/ui/Modal/Modal.tsx
|
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
YeBeenChoi
left a comment
There was a problem hiding this comment.
변경 사항 확인했습니다! 고생 많으셨어요!! 👍👍
✨ 주요 변경사항
📝 작업 상세 내용
✅ 체크리스트
Close #번호추가📸 스크린샷 (선택)
모달 버튼을 누르면....
모달이 열립니다!
🔍 기타 참고사항
🔗 관련 이슈