[Wip] 공통 컴포넌트 제작 (9) MyModal #46
Conversation
- chakra-ui의 provider가 context-api라서 modal도 잘 작동 합니다..!
…pen_Hoil into feature/commomComponentMyModal
loevray
requested review from
hoilzz,
JaeHyunGround,
SeungHyune and
whdgur5717
| title: string; | ||
| children: ReactNode; | ||
| buttonText: string; | ||
| onSubmit: () => void; |
There was a problem hiding this comment.
onSubmit은 ModalProps에 있는 타입일까요? 모달 버튼 클릭 시 이벤트인거 같은데, 뭔가 submit 느낌인지는 잘 모르겠습니다...
There was a problem hiding this comment.
악...무의식 코딩의 잔재네요ㅋㅋㅋㅋ 하단 푸터에있는 버튼을 클릭하면 실행할 함수를 전달해주는 거였습니다
onButtonClick등으로 네이밍 바꿔보겠습니다 ㅎㅎ
There was a problem hiding this comment.
고생 많으셨습니다 !! 😊
1. Portal을 사용한 게 잘한걸까요?ㅎㅎ
Portal을 잘 사용해 주신 것 같습니다 ㅎㅎ
2. 모양은 거의 고정되어있다고 보시면 됩니다. 모양 변경이 필요하면, 그때 새로운 모달컴포넌트를 만드는 게 좋을 것 같은데, 어떻게 생각하시나요? compound패턴처럼 Modal내부 컴포넌트화가 잘 되어있어서 괜찮아보입니다!
저도 Modal의 경우 UI를 사용하게 만들어 놓고 사이즈만 조절해서 사용하는 것이 좋다고 생각합니다 !
3. 모달 내용은 children으로 받아오고있습니다. 적절한걸까요?
ui 구조가 달라졌을 경우 children을 사용하면 동적으로 모달의 내용을 변경할 수 있다는 장점이 있어서 children 사용이 적절하다고 생각합니다.
4. Modal네이밍이 겹쳐서 MyModal로 짓긴 했는데 더 좋은 이름이 있다면 알려주세요! 이름이 너무 구려보이네요..ㅎㅎ
저는 그냥 SettingModal과 같이 설정 모달이라는 이름도 괜찮을 것 같다는 생각이 드네요 ㅎㅎ (TimerSettingModal, DdaySettingModal)
| modal: { | ||
| h: '343px', | ||
| w: '392px', | ||
| header: { | ||
| h: '54px', | ||
| }, | ||
| button: { | ||
| w: '178px', | ||
| h: '36px', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
전역적으로 사이즈 지정해서 사용하는 것 너무 좋네요 ㅎㅎ 👍
| ...props | ||
| }: MyModal) => { | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
Fragment는 제거해도 괜찮지 않을까요 ?!! 👀🧐
혹시 따로 사용해 주신 이유가 있으실까요 ?!
There was a problem hiding this comment.
어엇 Button넣고 테스트하다가 감빢햇네요 이거 지우고 merge하겠습니다 ㅎㅎ
작업 사항
관련 이슈
#45 입니다
PR Point
참고사항