Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat: 공용 모달 컴포넌트 추가 #11
Feat: 공용 모달 컴포넌트 추가 #11
Changes from 2 commits
795f170
be2908e
46962d6
238a283
12fc904
f63caee
3ec583e
6794a9c
fa6c2ce
801369a
a3eaf2a
f325a2f
5799a19
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@Nahyun-Kang
이 부분이 추가 됐습니다! 속성값으로 검정바탕 눌렀을때 발생할 핸들러 전달주시면 됩니다~! :)
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.
확인했습니다!! :D 알려주셔서 감사해용!! 🙇♀️
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.
이벤트핸들러 타입을 지정하실 때 핸들러 자체 타입으로 지정하신 이유가 궁금합니다!!
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.
사실 타입이 너무 어려워서, 오류가 뜨면 수정해나가면서 하고 있는데요!🥲
button 태그의 onClick 타입이 핸들러 자체 타입인 것 같아 그대로 적었습니다🙈
핸들러 자체 타입 지정 대신 다른 타입으로 지정하는 것이 좋을까요??🤔
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.
서영님 이 부분은 사소하지만 createPortal을 바로 불러와도 좋을 것 같습니다.
또, 모달이 브라우저 상태일때만 렌더링될 수 있도록 체크하는 조건을 넣어주는 방법도 불필요한 렌더링을 막을 수 있어서 참고로 말씀드려 봅니다!!
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.
앗 소현님! 이 부분은 제가 구현한 컴포넌트인데 조언 감사합니다!☺️ 말씀하신대로 수정해놓겠습니다👍
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.
꼼꼼하게 봐주셔서 감사합니다 :)
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.
서영님 토글을 useCallback으로 감싸기 너무 좋네요!!
매번 열림, 닫힘 코드를 작성하지 않고 상태를 관리하는 훅 너무 좋은 방법이네요!!!
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.
우왓 boolean 값 추론이 되겠군요!! 꼼꼼하게 봐주셔서 감사합니다🙇♀️
네 맞습니다!! 그런데 이러면 defaultValue 의미가 없어지는군요ㅠㅠ
추가했던 이유는 이전에 useToggle 훅을 만들어 모달을 관리한 적이 있는데 vercel 배포과정에서 strictmode가 꺼지면서 모든 모달이 오픈되는 오류가 발생했었습니다. (이유를 정확히는 모르겠지만) 렌더링이 되면서 뭔가 잘못되는 것 같아, 저 코드를 넣었더니 해결이 됐습니다 🤔
(1) 정확한 원인도 모르는 해결책이니 우선 지우고 문제 발생시 다시 생각해본다.
(2) 혹시를 대비하여 아래처럼 코드를 수정한다
저는 1번으로 제거 해볼까 하는데, 소현님 의견도 궁금합니다 :)
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.
서영님 그나저나 훅 달인이시네요.. 어떻게 이렇게 만드시나요ㅜㅜ!! 배워갑니다 진짜루.. 훅을 훅훅 만드시네요..막 이래😶🌫️
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.
꺄 감사합니다 나현님🙇♀️ 사실 저번 팀프로젝트들에서 얻은 것들을 주섬주섬 주워다가 쓰고 있습니다 ㅎㅎ
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.
오! 매번 outSideClick 함수를 버블링으로만 구현했었는데 이렇게 ref를 이용하는 방법도 있군요!
이 방법이 팝오버 메뉴에 적용할 때 더 유용할 것 같다는 생각이 드네요, 또 서영님 덕분에 touchstart 이벤트도 알아갑니당👍
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.
궁금해서 찾아보니 touchstart가 모바일/태블릿 기기 전용 이벤트 였군요!! 서영님 세심함에 감탄하고 갑니다 ㅎㅎ 👍
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.
헉 감사합니다 🙇♀️
저는 항상 ref로만 구현 해봐서, 버블링으로 구현하는 방법 너무 궁금합니다! 소현님 이전 프로젝트 구경가야겠어요 🏃♀️