Skip to content

Conversation

@winternuary
Copy link
Member

@winternuary winternuary commented Jan 3, 2024

작업사항

헤더의 설정을 클릭했을 때 모달 창이 뜨도록 했습니다.
일단 로그아웃 버튼만 추가 해 두었습니다

너무 오래 잡고 있었던 것 같아 죄송합니다... 이슈에 남겨주신 요구사항들도 끝까지 해내고 싶었는데 그러지 못해 많이 아쉬워요
이번에 설정 모달 창 만들면서 엄청 길고 많은 코드를 제대로 뜯어보고 해석 해 볼 수 있는 기회여서 재밌게 했습니다!!
그리고 파일 분리의 중요성에 대해 깨닫게 된 것 같습니다. 이유 모를 에러를 만날 때 마다 아직 많이 부족해서 공부가 많이 필요하다는 생각이 절실히 들었습니다.. 감사합니다!!

스크린샷

image
image

@winternuary winternuary self-assigned this Jan 3, 2024
@Ubinquitous
Copy link
Member

#144

Copy link
Member

@Ubinquitous Ubinquitous left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!! 새해 복 많이 받으시길 바랍니다!!

export { default as QR } from "./QR.png";
export { default as TestBanner } from "./test_banner.png";
export { default as PageNotFound } from "./page_not_found.png";
export { default as Back } from "./Back.png";
Copy link
Member

Choose a reason for hiding this comment

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

/src/assets/XIcon.tsx 컴포넌트를 사용해보시는건 어떨까요?
다른 모달과 UX/UI적으로 공통적으로 관리하는 것도 괜찮을 것 같고,
이미 있는 svg를 재사용하는 것도 좋은 방법론 같습니다😀

@@ -0,0 +1 @@
export { default as useSettingModal } from "./useSettingModal";
Copy link
Member

Choose a reason for hiding this comment

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

re-export 좋습니다!!

import { useState } from "react";

const useSettingModal = () => {
const [isSettingModalOpen, setSettingModalOpen] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

사실 useModal이라는 훅을 이미 전역으로 사용하고있답니다...!!!!!
밑에서 더 자세히 설명드리고 싶어요!

Comment on lines 6 to 12
const openSettingModal = () => {
setSettingModalOpen(true);
};

const closeSettingModal = () => {
setSettingModalOpen(false);
};
Copy link
Member

Choose a reason for hiding this comment

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

event를 trigger하는 변수명을 지을 때 BSM에서는
[handle][이벤트내용][엘리먼트][이벤트명]과 같은 네이밍을 사용하고 있습니다!

예를 들어 버튼을 누르면 count를 +1 추가하는 함수는 handleIncreaseButtonClick 이라고 짓는답니다!
주관적으로 저는 특히 이벤트와 관련된 함수는 함수명이 길더라도 직관적이고 한번 보았을 때 바로 어떤
작업을 하는 함수인지를 알아차릴 수 있게 작성하는 게 좋다고 생각하며 코드를 작성합니다!

이를 도와주는게 'handle'이라는 event trigger를 명시하는 키워드와, 어디에서 쓰이는지를 명시하는 'button'과
어떤 이벤트인지를 알려주는 'click'등으로 명시할 수 있는 점이 좋은 것 같습니다!
어떻게 생각하시나요?

Comment on lines 17 to 19
<button onClick={onClose}>
<ExitButton src={Back} alt="Back" />
</button>
Copy link
Member

Choose a reason for hiding this comment

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

불필요한 element가 많이 사용되고 있는 것 같습니다!
위에서 말씀드린 것처럼, 컴포넌트화되어있는 svg인 XIcon을 사용하면 이를 더 깔끔하게 해결할 수 있으며,
XIcon은 svg props를 그대로 받아 넣어주기에 바로 props를 넣어 사용할 수 있습니다!

Suggested change
<button onClick={onClose}>
<ExitButton src={Back} alt="Back" />
</button>
<XIcon onClick={onClose} />


const LogoutBox = styled.div`
${flex.CENTER};
margin: 7% 0 0 0;
Copy link
Member

Choose a reason for hiding this comment

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

헉 지금 컨벤션에선 한 요소만 있을 땐 margin-top 등의 명시를 사용하기로 결정했답니다!
하지만 배운 점 잊지 않고 기억해주셔서 너무 감사할 따릅입니다🥹

margin: 7% 0 0 0;
padding: 2%;
border-radius: 5%;
background-color: #f1f3f5;
Copy link
Member

Choose a reason for hiding this comment

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

등록되지 않은 색을 사용하고 싶으시다면 styles/theme.ts에서 추가해 사용하여 매직 리터럴을 피하시는 걸 추천드려요!
그리고 white가 아닌 이 색상을 사용하신 이유가 있으신지 궁금합니다 !

Copy link
Member Author

Choose a reason for hiding this comment

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

로그아웃 버튼은 설정 박스랑 구분되면 좋을 거 같다고 생각해서 다른 색상으로 사용하게 되었습니다

Comment on lines 10 to 19
const { isSettingModalOpen, openSettingModal, closeSettingModal } =
useSettingModal();

return (
<Layout>
<Logo width={42} />
<Navigation />
<Setting />
<Setting onClick={openSettingModal} />
{isSettingModalOpen && <SettingModal onClose={closeSettingModal} />}
</Layout>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { isSettingModalOpen, openSettingModal, closeSettingModal } =
useSettingModal();
return (
<Layout>
<Logo width={42} />
<Navigation />
<Setting />
<Setting onClick={openSettingModal} />
{isSettingModalOpen && <SettingModal onClose={closeSettingModal} />}
</Layout>
import { useModal } from "@/@modal/hooks";
const Header = () => {
const { openModal } = useModal;
const handleOpenSettingModalClick = () => {
openModal({ component: <SettingModal/> });
}
return ... <Setting onClick={handleOpenSettingModalClick} />
}

위에서 말씀드린 제가 미리 만들어둔 전역 모달 훅을 사용하면 이렇게 훨씬 편하게 작성할 수 있답니다...!!!
그래도 모달 만든다고 수고하셨습니다!! 되게 값진 경험 되었을 것 같아요 굳굳

Comment on lines -12 to -27
export const event = (
action: Gtag.EventNames,
{ event_category, event_label, value }: Gtag.EventParams,
) => {
window.gtag("event", action, {
event_category,
event_label,
value,
});
};

export const useGtag = () => {
const router = useRouter();

useEffect(() => {
if (process.env.NODE_ENV === "development") return;
Copy link
Member

Choose a reason for hiding this comment

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

감사합니다ㅜㅜ 아직 Next13에선 GA 이슈가 있나봐요..!! 처리해주셔서 감사합니당

<ExitButton src={Back} alt="Back" />
</button>
</SettingBox>
<LogoutBox>로그아웃</LogoutBox>
Copy link
Member

Choose a reason for hiding this comment

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

헉 아직 API연결은 안된건가요??

@Ubinquitous Ubinquitous merged commit b341695 into main Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants