Feat/11/modal component #22
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
워크플로우 추가를 위한 메인 병합
…1/modal-component
…t/11/modal-component
donghunee
force-pushed
the
feat/11/modal-component
branch
from
2b25cf1
to
23b69f0
Compare
klmhyeonwoo
reviewed
Jul 7, 2024
leeminhee119
approved these changes
Jul 7, 2024
src/app/MainPage.tsx
Outdated
Comment on lines
17
to
19
| <Modal onClose={close} isModalOpen={isOpen}> | ||
| <div>냠냠</div> | ||
| </Modal> |
There was a problem hiding this comment.
Suggested change
| <Modal onClose={close} isModalOpen={isOpen}> | |
| <div>냠냠</div> | |
| </Modal> | |
| {isOpen && | |
| <Modal onClose={close}> | |
| <div>냠냠</div> | |
| </Modal> | |
| } |
요렇게 하고 Modal 안에서 스크롤 방지하는 로직을 useModal 훅으로 옮기면 isModalOpen props가 없어져도 될 것 같아요!
There was a problem hiding this comment.
prop로 넘기는 것보다 더 깔끔한 것 같아요~!! 감사합니다:)
There was a problem hiding this comment.
isOpen을 계속 써야하는 번거로움이 있을 것 같아서 코드를 개선해봤습니다~!!
src/component/common/modal/Modal.tsx
Outdated
Comment on lines
14
to
16
| useEffect(() => { | ||
| document.body.style.overflow = isModalOpen ? "hidden" : "auto"; | ||
| }, [isModalOpen]); |
There was a problem hiding this comment.
이 내용을 useModal 훅 안에서 isModalOpen 대신 isOpen으로 대체해서 옮기면 어떨까요 ??
klmhyeonwoo
approved these changes
Jul 8, 2024
| export type ModalType = { | ||
| isOpen: boolean; | ||
| title: string; | ||
| content: JSX.Element | string; |
src/types/modal.ts
Outdated
| isOpen: boolean; | ||
| title: string; | ||
| content: JSX.Element | string; | ||
| callBack?: () => any; |
There was a problem hiding this comment.
callBack을 any 로 정의하신 이유가 궁금합니다!
There was a problem hiding this comment.
void로 해야하는걸 실수 했네요 수정해놓겠습니다:)
src/component/common/modal/Modal.tsx
Outdated
| return () => { | ||
| document.removeEventListener("mousedown", listener); | ||
| }; | ||
| }, [close, modalRef]); |
There was a problem hiding this comment.
modalRef를 의존성 배열에 넣지 않으면 동작하지 않는 코드인가요?! 궁금해요!🤔
There was a problem hiding this comment.
아이고 체크를 못했네요.. close랑 modalRef 둘다 넣지 않아도 됩니다:)
|
너무너무 고생하셨습니다 🏄🏼♂️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🏄🏼♂️ Summary (요약)
🫨 Describe your Change (변경사항)
🧐 Issue number and link (참고)
2024-07-07.10.44.00.mov
📚 Reference (참조)