Skip to content
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

이미지 클릭 시 리뷰 모달 오픈 및 리뷰모달 글쓰기 모달 전체적인 스타일 변경 #364

Merged
merged 4 commits into from
Dec 8, 2022

Conversation

NaamuKim
Copy link
Member

@NaamuKim NaamuKim commented Dec 7, 2022

요약

이미지 클릭 시 리뷰 모달 오픈 및 리뷰모달 글쓰기 모달 전체적인 스타일 변경

연관 이슈

Pull Request 체크리스트

TODO

  • 최종 결과물을 확인했는가?
  • 의미 있는 커밋 메시지를 작성했는가?

@NaamuKim NaamuKim added ✨ Feature 기능 개발 🎨 Html&css 마크업 & 스타일링 labels Dec 7, 2022
@NaamuKim NaamuKim added this to the 5주차 마일스톤 milestone Dec 7, 2022
@NaamuKim NaamuKim requested a review from shyuuuuni December 7, 2022 17:59
@NaamuKim NaamuKim self-assigned this Dec 7, 2022
@NaamuKim NaamuKim removed this from the 5주차 마일스톤 milestone Dec 7, 2022
Copy link
Collaborator

@shyuuuuni shyuuuuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작업 고생 많으셨습니다!

@@ -15,6 +15,11 @@
@import "@/styles/global-style";
@import "@/styles/theme";

* {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에 다른 사이드 이펙트는 없었나요 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 확인하고 있던 뷰에선 없었습니다

const { id: postId, code, language } = useContext(PostContext);
const [openModal] = useCommonModalStore((state) => [state.openModal]);

const handleOpenReviewModal = useCallback(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에 useCallback을 사용하면 확실히 이득이 발생하나요?

보통 최적화를 진행해야 하는 부분이 props로 값이 넘어가서 참조가 동일한지 체크하는 부분이 있거나, 아니면 계산이 오래걸리거나 등에서 처리해야 할 것 같은데 위 코드는 그런 로직이 없어서 득보다 실이 더 클 것 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 commons 컴포넌트여서 혹시 가능하다면 onClick을 props로 넘겨주는 것이 더 좋을 것 같습니다. 이렇게 사용하면 모달에 종속되어버릴 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에 useCallback을 사용하면 확실히 이득이 발생하나요?
보통 최적화를 진행해야 하는 부분이 props로 값이 넘어가서 참조가 동일한지 체크하는 부분이 있거나, 아니면 계산이 오래걸리거나 등에서 처리해야 할 것 같은데 위 코드는 그런 로직이 없어서 득보다 실이 더 클 것 같습니다!

함수 재생성보다 캐싱에 비용이 더 많이 드는데 캐싱했을 수도 있을 것 같네요!
제 근거는 계속 렌더링 될 수 있는 포스트에서 렌더링 될때마다 함수를 저장하는 것보다는 거의 바뀔 일 없는 함수를 캐싱해놓는 편이 낫겠다였습니다.

그리고 commons 컴포넌트여서 혹시 가능하다면 onClick을 props로 넘겨주는 것이 더 좋을 것 같습니다. 이렇게 사용하면 모달에 종속되어버릴 것 같아요.
그렇네요! 수정하겠습니다!

<div className="modal-background" onClick={clickWrapperBackGround} />
<ModalContainer content={modalContent} />
<div className="modal-background" onClick={clickWrapperBackGround}>
<CloseIcon
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

닫는 아이콘을 추가하셨군요!

$modal-background-z-index: 1;
$modal-content-z-index: 2;
$modal-upper-z-index: 3;
$modal-background-z-index: 20000;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정리하신 부분 좋습니다!

@@ -0,0 +1,4 @@
export const isCloseModalElement = (element: HTMLElement): boolean =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분 의도가 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이벤트 버블링이 발생해서 이벤트 버블링을 막기 위해 닫혀야할 곳만 추가하였습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모달에 있는 버튼 클릭 이벤트에 이벤트 버블링을 제어하기 위해 작성한 코드입니다

Copy link
Collaborator

@shyuuuuni shyuuuuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다! 리뷰 확인하시고 이상 없으시면 머지 하셔도 될 것 같아요!!


return (
<img
ref={observeImage}
className={`progressive-image ${className}`}
src={placeholder}
onClick={handleOpenReviewModal}
onClick={handleClickImage}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onClick에 undefined가 들어가도 상관없으려나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 따로 값을 넣어주지 않으면 기본이 undefined인 것 같습니다.

@@ -12,6 +9,7 @@ interface ProgressiveImageProps {
width: number | "100%";
height: number | "100%";
alt: string;
handleClickImage?: () => void;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional로 두신 부분이 좋네요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발 🎨 Html&css 마크업 & 스타일링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants