-
Notifications
You must be signed in to change notification settings - Fork 4
✨ feat : 모달 컴포넌트 구현 및 리팩토링 #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@jeschun is attempting to deploy a commit to the projects Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
sohyun0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다! 코멘트 확인부탁드립니다 ~~! :)
| {/* Footer (버튼)*/} | ||
| {/* Footer */} | ||
| <div className='flex items-center justify-center gap-3 px-6 pb-6 pt-6'> | ||
| {secondaryText && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 secondary 버튼은 text 유무만 체크해서 버튼을 보여주고 있는데요! onSecondary 와 함께 관리되는것이 좋을 것 같습니다!
이유 : secondaryText 만 작성하고 onSecondary 를 작성하지 않았을때 버튼은 노출되지만 액션실행이 되지 않는데요 만약 일단 생성만 해두고 액션은 다음에 만든다라고 했을때 이 로직을 기억하지 않는다면 이미 완성된 버튼으로 착각할 수도 있을것을 방지할 수 있습니다!
secondaryText && onSecondary && 로 작성시 둘다 입력해야 정상 노출 + 정상 실행 될 수 있다는 것을 알 수 있게 하는게 DX 관점에서 조금더 좋지않을까 생각합니다! :)
| className, | ||
| }: ModalProps) { | ||
| // ESC로 닫기 | ||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분을 추후 커스텀 훅으로 뺼수도 있을것 같아요! (제안)
만약 그렇게 분리를 한다면 dimmed 을 사용하지않는 필터나 알림에서도 응용이 가능할것 같습니다 !
src/components/ui/modal/modal.tsx
Outdated
| )} | ||
| > | ||
| {/* Header (아이콘, 제목) */} | ||
| {/* Header */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모달의 경우 header / body / footer 로 컴포넌트 분리도 해볼 수 있을것 같습니다 (제안)
📝 작업 개요 (필수)
공통 모달 규격 통일(타이틀 간격, ESC/딤 클릭 닫힘, Portal 옵션, 스토리 문구 통일).
✨ 작업 내용 (필수)
📸 스크린샷
🧐 해결해야 하는 문제
🤔 리뷰어 확인 필요 사항
🔗 관련 이슈
🛠️ 후속 작업
✅ 체크리스트 (필수)