[Refactor] 모달 띄우는 로직, 데이터 상태 로직 리팩토링.. #197

cindy-chaewon · 2024-08-01T11:37:34Z

해당 이슈 번호

closed #187

체크리스트

📌 내가 알게 된 부분

네… 모달 띄우는 로직 싹 다 갈아 엎었습니다…

( 로직을 2-3번 갈아엎고,,, 갈아엎었는데 이전에 없던 오류가 우수수 생겨나고,,, 그래서 오래걸렸습니다…)

이전의 방식은 상태관리 useState를 사용해서 useModal내에서 모달 내 current content를 띄우는 방식이었습니다.

이런 방식에서 마음에 안 들었던 점이 LeftSidebar와 아카이빙 페이지에 모달 띄우는 로직 코드가 길어진다는 점이었습니다..

그래서 저는 생각했습니다.. 모달을 전역으로 관리해보자~~~!

찾아보니 context api, zustand, jotai 등등으로 모달을 전역적으로 상태 관리하는 방법이 있었습니다.

처음의 저의 방식은 다음과 같았습니다

원래는 분리되어 있었던 모달 공통 컴포넌트와 모달 컨텐츠 컴포넌트들을 합쳐서 하나의 모달 컴포넌트로 사용하자!! 예를 들어 WorkSpaceName 코드가 있으면 그 코드에 모달 공통 컴포넌트 를 임포트하여 로 감싸주어 하나의 모달 그 자체로 사용하는 것입니다..

근데 문제가 이방법이 나중에 다 구현하고 보니까 다음 모달로 넘어갈때 엄청나게 깜빡 거리더라고요,,, 엄청나게… (띄워져있던 모달을 닫고 다시 새로운 모달을 띄우는 형태여서 그런 거 같아요…)

그래서 원래 방식이었던 모달 공통 컴포넌트 따로 모달 컨텐츠 따로 해서 갈아끼는 로직으로 다시 진행했습니다…!

그리고 다음 모달로 넘어가는 로직 코드가 원래는 LeftSidebar, 아카이빙 페이지에 길게 있었잖아요..!! (handleNext 어쩌구로 길게..) 근데 그 거를 해당 모달 컴포넌트에서 처리하도록 구현했습니다! 모달의 step을 지정해주어서 useNextStep을 이용해 다음 모달로 넘겨지게 했습니다.

그리고 고민이 있습니다…

모달 관련 데이터 처리 코드를 최대한 다른 컴포넌트에 안 쓰려고 context api로 모달 내 데이터 관리를 했는데요,,, 사실 이게 좋은 방법 인지는 모르겠어요,, 관련 코드를 LeftSidebar나 아카이빙 페이지에 쓰더라도 props로 넘기는게 더 좋을지… 고민이 됩니다.. (LeftSidebar 가 App 내 에 있어서.. contextProvider를 App 에 선언해야지 오류가 안나더라고요,, 이것도 마음에 안듦…)

생각보다 로직을 다 바꿔서… 이것도 계속 고쳐나가야 할 거 같아요!! 코리 마구마구 부탁드립니다!!

두번째 리팩토링

전역 상태 관리: Zustand로 모든 모달의 전역 상태를 관리.
Context API: 각 모달 플로우(워크스페이스, 블럭, 삭제)에 필요한 상태와 로직을 Context API로 관리.
최상위 모달 컴포넌트: 전역 상태를 기반으로 모달을 렌더링.

📌 질문할 부분

📌스크린샷 (선택)

wuzoo

일단, 첫 번째로 고민해볼 지점은 전역 상태와 context api를 어떻게 구분할 것이냐인 것 같아요.

저희가 그동안 진행했던 멘토링에서 멘토님들께서도 말씀하셨다시피,

먼저 Context는 상위 어플리케이션 지점에서 구독하여 사용한다면 렌더링 성능적으로 문제가 발생할 수도 있을 거에요
그렇다면 손수 리렌더링을 막기 위해 메모이제이션 처리를 해주어야하는데, 저희는 zustand를 도입한 상태이므로, 써드 파티 라이브러리의 도움을 빌려도 될 것 같다는 생각입니다. 최상위 도메인에서 모달 컨텐츠를 모두 띄울 수 있어야 하는 상황이므로, 최상위 어플리케이션 지점에서 모두 접근해야 한다면, 전역 상태가 맞는 것 같기도 해요

두 번째로 고민해볼 건, 무엇을 전역상태로 관리할 것이냐 ! 인 것 같아요

채원님은 현재 pr에 남겨주신 말씀대로, 지금 공통 컴포넌트인 모달 컴포넌트로 한 컨텐츠에 해당하는 모달을 모두 만들어준 다음에, 이를 모두 전역상태로 선언하여 갈아끼워주는 식으로 하고 있는 것 같아요.
아마도 ? 전역에서 모든 Step에 해당하는 컴포넌트 상태들을 계속하여 갈아끼워넣어주는 것은 성능적으로도, 코드의 가독성적으로도 좋지 않을 것 같다고.. 저는 생각해요
그렇다면, 전역에서는 띄워질 모달 컨텐츠를 관리하자 ! 가 최선인 것 같아요. 그 말은 즉슨, 전역 상태로는 "지금은 워크스페이스 생성하는 플로우를 모달로 띄울거야" 혹은 "지금은 블록 생성 플로우의 모달을 띄울거야"를 관리하는 거죠.
제가 방금 짧게 생각해본 바로는 전역 상태로 반환할 JSX까지는 가지고 있어도 좋을 것 같아요. 즉, 전역 상태 id가 workspace면, 전역 상태 content는 <WorkSpaceModalFlow />야 ! 인 식인거죠 ?

그럼 이제 Flow에 관련된 컴포넌트만을 생성하면 될 것 같아요. 제가 Context를 전역 수준에서 관리하는 것은 좋지 않을 수도 있다고 말씀드렸지만, 이렇게 Step만을 국소적으로 관리하도록 Flow 컴포넌트 수준에서 Provider로 구독하게 한다면 나쁘지 않을 것 같아요. 그렇다면 특정 한 컴포넌트에서, 현재 보여줄 모달 플로우에 대한 각 Step 컴포넌트들을 관리할 수 있을 것 같아요.

아마도 전역 상태에는 현재 보여줄 모달 Flow 컴포넌트를 관리해야 하므로, 공통 컴포넌트 모달이 주입받았던 isOpen, onClose 등을 관리하겠죠.
id에 따른 content만을 관리할 것이므로, onClose 같은 동작들은 id를 null로 만들어주거나와 같은 행위로 적절하게 판단하면 될 것 같아요.

채원님의 코드를 보고, 아마도 Context를 다시 뜯어고치게 되면, 거의 모든 부분의 코드가 수정될 것 같아서 이렇게 바로 코멘트 남깁니당.

namdaeun

이전 코드에 비해서 확실히 모달 로직의 분리가 보이는 것 같아서 넘 좋다고 생각합니다 !
전역상태관리를 사용해서 모달을 구현하는 게 여간 복잡한 일이 아니군요 ,,, 고생많으셨습니다
주용님 말대로 수정된 코드도 기대할게요 🔥

namdaeun · 2024-08-01T15:43:15Z

src/shared/util/modal.tsx

+  return activeModalType ? (
+    <Modal isOpen={true} onClose={handleClose}>
+      {ModalContent}
+    </Modal>
+  ) : null;


const activeModalType = Object.keys(modals).find((key) => modals[key as ModalType]) as ModalType | undefined;

modalType이 undefined일 경우가 있는지 궁금합니다
undefined일 필요가 없다면 삼항연산자 대신 && 연산자를 사용해서 리턴해줄 수 있을 것 같습니다 !

return activeModalType && <Modal isOpen={true} onClose={handleClose}> {ModalContent} </Modal>

namdaeun · 2024-08-01T15:50:25Z

src/shared/store/modalContext.tsx

+  const [blockName, setBlockName] = useState('');
+  const [blockType, setBlockType] = useState('');
+  const [startDate, setStartDate] = useState('');
+  const [endDate, setEndDate] = useState('');


const [blockData, setBlockData] = useState<타입>({ blockName: '', blockType: '', startDate: '', endDate: '', });

네 개의 상태를 객체로 묶어서 상태관리하면 코드가 더 간결해질 것 같습니다 !

namdaeun · 2024-08-01T16:28:27Z

src/page/archiving/createTimeBlock/component/Upload/UploadModal.tsx

@@ -77,19 +74,20 @@ const UploadModal = ({ onClose, teamId, type, blockData }: UploadModalProps) =>
  };

  const data = {
-    name: blockData.blockName,
+    name: blockName,
    color: getRandomColor(),


요기 서버에서 컬러색상코드를 전달해주니까 그 값을 넘겨주는 방향으로 수정 부탁드립니다 ㅎㅎ

수정 완료 했습니다!

wuzoo

전역 상태로 관리해야할 것과 아닌 것들.. 정말 잘 분리하신 것 같아요 채원님.

채원님 코드 뜯어보면서, 진짜 간만에 뭔가 재밌었던 것 같아요 .. 전역 상태를 써도 어디까지가 전역에서 처리되어야 하는지 등등 저도 같이 고민할 수 있어서 좋았습니다.

다른 팀원분들 의견도 들어보면서 코드 반영해주시면 좋겠어요

wuzoo · 2024-08-02T04:36:42Z

src/shared/component/createWorkSpace/category/WorkSpaceCategory.tsx

category, image, name 등등.. 모두 워크스페이스를 생성하는 모달 폼 과정에서의 Step 들이라고 생각해요!

그렇다면 하나의 폴더안에서 관리하는 것이 훨씬 더 가독성에 좋을 것 같아요. 특정 폴더 하위에, 각 컨텐츠에 해당하는 컴포넌트들을 모아 둔다면 훨씬 더 알아보기 쉬울 것 같아요 !

주용님 말에 동의합니다!
그리고 createWorkSpace 대신 createWorkSpaceModal이라는 폴더명은 어떤지 제안해봅니다
DeleteModal 폴더명처럼 모달 컴포넌트와 관련된 코드라는 걸 더 잘 알 수 있을 것 같아요 !

저도 동의합니다! 현재 createWorkSpaceModal 폴더안에 modalContents 폴더를 생성하여 그 안에 category, image, name과 같은 모달 폼 과정의 코드들을 넣었는데요..! 이렇게 바꾸는 거 괜찮을까요??

wuzoo · 2024-08-02T04:41:07Z

src/shared/component/createWorkSpace/category/WorkSpaceCategory.tsx

  useEffect(() => {
    if (selected) {
-      onCategory(selected);
+      setCategory(selected);
+      close?.();
    }
-    close?.();
-  }, [selected, onCategory, close]);
+  }, [selected, setCategory, close]);


특정 상태의 값에 따라서 다른 상태의 값을 업데이트하는 행위를, useEffect 안에서 처리하는 것은 안티 패턴 중 하나에요 !

분명 구조를 효율적으로 짠다면, 이벤트핸들러 안에서도 처리가 가능할 것이라고 생각합니다. handleSelect 안에서 처리 불가능한가요 ?

앗 가능합니다!! 수정 완료 했습니다!

wuzoo · 2024-08-02T05:22:41Z

src/shared/component/LeftSidebar/LeftSidebar.tsx

  const handleWorkspaceClick = () => {
    setIsWorkspaceClicked(true);
-    openModal(<WorkSpaceName onNext={handleNext1} setName={setName} />);
+    openModal(
+      'workspace',
+      <WorkSpaceProvider>
+        <WorkSpaceFlow />
+      </WorkSpaceProvider>,
+      handleModalClose
+    );
  };


쇼케이스가 클릭되었느냐, 워크스페이스가 클릭되었느냐 등등..을 판단하는데에 너무 불필요한 상태들이 있는 것 같아요 !

selectedItem 혹은 selectedId 상태를 하나만 두고, 이것을 바꿔가면서 클릭된 여부는 해당 아이디가 내 아이디와 같냐만 판단하면 되지 않을까요 ?

물론 채원님이 짠 코드인지 아닌지는 모르겠지만.. ㅎㅎ 수정해주시면 좋을 것 같아요

넵! 하나의 상태로 통합해서 선택된 항목을 관리하는게 더 깔끔하겠네용! 수정 완료 했습니다!

wuzoo · 2024-08-02T05:27:47Z

src/shared/component/DeleteModal/DeleteModal.tsx

  const { mutateAsync: blockMutate } = useDeleteBlockMutation();
  const { mutateAsync: documentMutate } = useDeleteDocumentMutation();


반환되는 Promise를 통해 추가적인 작업을 하지 않을 것이라면 mutate를 사용하는 것이 더욱 좋을 것 같아요 !

넵!! mutate로 수정 완료 했습니다!

wuzoo · 2024-08-02T05:37:13Z

src/shared/store/modal.tsx

+export const useModalStore = create<ModalState>((set) => ({
+  id: null,
+  isOpen: false,
+  content: null,
+  closeCallback: null,
+  openModal: (id, content, closeCallback = null) => set({ id, content, isOpen: true, closeCallback }),
+  closeModal: () => {
+    set((state) => {
+      if (state.closeCallback) {
+        state.closeCallback();
+      }
+      return { id: null, content: null, isOpen: false, closeCallback: null };
+    });
+  },
+}));


요 부분 ! zustand로 너무 잘 구현하신 것 같아요.

그런데 closeCallback을 따로 받아야된다면, 말 그대로 콜백 함수! (이후에 호출되는 함수)잖아요 ! 따라서 이렇게 상태를 하나 더 선언해주는 것이 아니라, closeModal 호출 시 아예 파라미터로 콜백을 넘기면 어떨까요 ? onSuccess와 같이 Close가 성공되었을 때 실행시킬 콜백 함수요 !

그리고 제가 처음 코리남겼을 때, id가 무엇이면 content를 어떻게 띄워라. 이런식으로 말씀드렸잖아요 !

근데 생각해보니, id가 필요하나..? 라는 생각도 드네요. 왜냐면 id에 따른 content를 띄워주려는게 아니라, 어차피 content를 해당 스토어를 이용하는 외부에서 주입하는 상황안데 ! 그렇다면 지금 Store는 어떤 컨텐츠가 어떤 아이디인지를 모르니, id는 지워줘도 되지 않을까요 ?

다른 분들의 의견 궁금합니다 !

동의합니다 ! 스토어로 리턴하는 모달을 id로 구분해줄 필요가 없다는 생각이 드네요

실제로 content만으로도 필요한 모든 정보를 전달하고 있어서 id는 없어도 될 거 같네요!!
그리고 closeCallback부분이 원래 레프트 사이드바 워크스페이스 아이템 선택관련 로직 하나 때문에 필요했거든요..! 근데 이번에 워크스페이스 생성 선택 로직을 바꿨더니 필요없는 거 같아서 콜백 관련 코드는 삭제 했습니다!

wuzoo · 2024-08-02T05:40:26Z

src/shared/store/useWorkSpaceContext.tsx

+
+  switch (step) {
+    case 1:
+      return <WorkSpaceName />;
+    case 2:
+      return <WorkSpaceCategory />;
+    case 3:
+      return <WorkSpaceImage />;
+    case 4:
+      return <WorkSpaceComplete />;
+    default:
+      return null;
+  }


스위치문으로 관리하는 것보다, step을 각 Step에 해당하는 컴포넌트들한테 넘겨주면 어떨까요 ? step이 특정 값이면, 나는 보인다/안보인다로 관리하면 될 것 같아요 ! 이에 관해서 정말 유사한 구조가 Funnel 구조에요 ! 한 번 참고해보시면 정말 좋을 것 같습니당.

제가 말씀드린대로 일단은 모든 스텝 컴포넌트들을 JSX 안에 넣어놓는다면, 전역 상태로 관리하는 함수 openModal을 호출 시 Provider를 같이 넘겨주는 것이 아니라, 여기 Flow 컴포넌트에서 처음부터 Provider를 구독하게 해놓으면 될 것 같아요

추가적으로 파일 네이밍 수정하면 좋을 것 같습니다 ! context에 관한 커스텀 훅을 따로 훅 폴더에 만들고, Flow 같은 컴포넌트들은 shared의 컴포넌트에 두면 어떨까요 ?

요건 다른 분들의 의견도 궁금하네요

주용님이 앞서 말씀하신 것처럼 step 값을 컴포넌트에 전달해서 렌더링 하는 게 코드의 가독성 측면에서 더 좋을 것 같다고 생각해요 !

그리고 context관련 코드들은 다른 컴포넌트에서 재사용이 되는 훅쪽에 더 가깝다고 생각하기 때문에 hook 폴더로 분리하는 거 찬성합니다! flow도 비지니스 로직이 포함되어 있으니 shared에 두는 게 맞다고 생각해요

다만 지금의 폴더구조에 대해 제안하고 싶은 게 있습니다.
createWorkSpace의 경우 쇼케이스 페이지(하나의 도메인)에서만 사용되는 모달입니다. 초기에 저희가 다같이 상의했던 게, 비지니스 로직이 한 도메인에만 쓰인다면 page 컴포넌트 안에 배치시키기로 얘기가 됐었어요. 따라서 최상위 shared 폴더에 위치시키기 보다는 page 폴더 내부의 shared에 위치시키는 게 어떨까요 ?

라고 생각했으나 사이드바의 모달이라서 아카이빙 페이지에서도 쓰이네요
하하 마지막 문단 말은 취소할게요~

뭐하세요 다은시?

const { step } = useWorkSpaceContext(); return ( <div> {step === 1 && <WorkSpaceName />} {step === 2 && <WorkSpaceCategory />} {step === 3 && <WorkSpaceImage />} {step === 4 && <WorkSpaceComplete />} </div> );

이런 식으로 변경 완료 했습니다! 그리고 파일 네이밍이랑 구조도 주용님말에 동의하여 변경 완료했습니다!! 좋은 의견 감사해요!

요거 분기처리 말구, step prop을 컴포넌트에 주입해주어서 사용하는 것은 어때요 ? 자신의 스탭이 아니라면 안보이게 각 Step 컴포넌트에 처리를 위임해줘도 될 것 같아요

좋아요!!

const { step } = useWorkSpaceContext(); return ( <> <WorkSpaceName step={step} /> <WorkSpaceCategory step={step} /> <WorkSpaceImage step={step} /> <WorkSpaceComplete step={step} /> </> );

이런식으로 flow를 변경하여
컴포넌트에서는

const WorkSpaceCategory = ({ step }: WorkSpaceCategoryProps) => {

이렇게 props로 받아와

if (step !== 2) return null;

자신의 컴포넌트에서는 안보이게 처리했습니다!

wuzoo · 2024-08-02T05:44:41Z

src/shared/store/useWorkSpaceContext.tsx

+interface WorkSpaceContextType {
+  step: number;
+  nextStep: () => void;
+  reset: () => void;
+  name: string;
+  setName: (name: string) => void;
+  category: string;
+  setCategory: (category: string) => void;
+  fileUrlData: string;
+  setFileUrlData: (url: string) => void;
+}


Block과 WorkSpace 컨텍스트 둘다 따로 따로 하나의 상태로 분리되어있는 상태들을 객체로 만들면 어떨까요 ?

name, category, urlData는 워크스페이스에서 입력 되어야 하는 상태들이므로 하나의 객체로 묶는거에요. 그리고 또 WorkSpaceFormData와 같은 타입하나를 더 생성해서 무엇을 워크스페이스 플로우에서 입력받을 것인지 해당 객체 타입의 상태의 타입을 지정하는거죠 ! 그렇다면 더욱 가독성에 좋을 것 같아요.

동의합니당!! 여러 개의 상태를 각각 관리하는 대신, 관련된 상태들을 하나의 객체로 묶어서 관리하면 코드가 더 깔끔해질 거 같아요! 반영 완료 했습니다!

wuzoo · 2024-08-02T05:49:18Z

src/App.tsx

@@ -43,6 +45,7 @@ const App = () => {
  return (
    <ErrorBoundary fallback={ErrorPage} onReset={handleResetError}>
      <Login>
+        <ModalManager />


ModalContainer..?

옹,.. ModalContainer 로 이름 변경 하라는 건가용??!

전역 상태 모달을 관리하는 것은 전역 상태이고, 현재 해당 컴포넌트는 모달을 보여주는 곳이니 Manager보다는 Container가 적합하다고 생각했어용 ..

namdaeun

채원이 리팩하느라 고생 많았습니다 !!!!
전역상태관리 사용해서 모달띄우기... 코리하면서도 모르는 개념들이 많아서 정말 많이 배워가는 코드였어요 ✨

namdaeun · 2024-08-02T06:36:12Z

src/shared/component/createWorkSpace/category/WorkSpaceCategory.tsx

주용님 말에 동의합니다!
그리고 createWorkSpace 대신 createWorkSpaceModal이라는 폴더명은 어떤지 제안해봅니다
DeleteModal 폴더명처럼 모달 컴포넌트와 관련된 코드라는 걸 더 잘 알 수 있을 것 같아요 !

namdaeun · 2024-08-02T06:46:11Z

src/shared/store/modalContext.tsx

요기 코드 이제 안 쓰면 파일 지워줍시다 ~!

namdaeun · 2024-08-02T07:22:26Z

src/shared/store/useWorkSpaceContext.tsx

+
+  switch (step) {
+    case 1:
+      return <WorkSpaceName />;
+    case 2:
+      return <WorkSpaceCategory />;
+    case 3:
+      return <WorkSpaceImage />;
+    case 4:
+      return <WorkSpaceComplete />;
+    default:
+      return null;
+  }


주용님이 앞서 말씀하신 것처럼 step 값을 컴포넌트에 전달해서 렌더링 하는 게 코드의 가독성 측면에서 더 좋을 것 같다고 생각해요 !

그리고 context관련 코드들은 다른 컴포넌트에서 재사용이 되는 훅쪽에 더 가깝다고 생각하기 때문에 hook 폴더로 분리하는 거 찬성합니다! flow도 비지니스 로직이 포함되어 있으니 shared에 두는 게 맞다고 생각해요

다만 지금의 폴더구조에 대해 제안하고 싶은 게 있습니다.
createWorkSpace의 경우 쇼케이스 페이지(하나의 도메인)에서만 사용되는 모달입니다. 초기에 저희가 다같이 상의했던 게, 비지니스 로직이 한 도메인에만 쓰인다면 page 컴포넌트 안에 배치시키기로 얘기가 됐었어요. 따라서 최상위 shared 폴더에 위치시키기 보다는 page 폴더 내부의 shared에 위치시키는 게 어떨까요 ?

namdaeun · 2024-08-02T07:25:10Z

src/shared/util/modal.tsx

요기도 이제 안 쓰인다면 파일 지워줍시당 !

rtttr1

고생 많으셨어요~~ 확실히 모달 상태들을 전역으로 관리하니 모달을 사용하는 컴포넌트에서의 코드양이 확 줄어드네요!! 너무 좋아요.....

rtttr1 · 2024-08-02T08:51:09Z

src/page/archiving/index/component/DocumentItem/DocumentItem.tsx

@@ -68,7 +67,6 @@ const DocumentItem = ({ documentId, children, selectedId, blockName, fileUrl, co
          <TrashBox width={20} height={20} onClick={(e) => handleTrashClick(e)} css={{ cursor: 'pointer' }} />
        </Flex>
      </li>
-      <Modal isOpen={isOpen} children={currentContent} onClose={closeModal} />


오 요 modal 컴포넌트가 굉장히 애매했는데 이제 없앨수 있군요!
그러면 지금
태그와 태그를 감싸고 있던 빈 <> 태그를 지워도 되겠네요~~!!

맞아요! 빈태그도 지워줬습니다!!

rtttr1 · 2024-08-02T09:09:12Z

src/shared/store/modal.tsx

+      return { id: null, content: null, isOpen: false, closeCallback: null };
+    });
+  },
+}));


zustand 기본구조 배워갑니다~~ 생각보다 되게 간결하네요!

감사합니다!

채원님 ! 저번에 제가 Zustand관련 리팩토링 PR에서 말씀드렸다시피, 최소한의 아토믹한 selector를 사용하고, 이를 따로 커스텀 훅에 적용하는 것 어떠세요 ??

렌더링과 소비자 입장에서 좋을 것 같아요 !

좋습니다!!

export const useModalIsOpen = () => useModalStore((state) => state.isOpen); export const useModalContent = () => useModalStore((state) => state.content); export const useOpenModal = () => useModalStore((state) => state.openModal); export const useCloseModal = () => useModalStore((state) => state.closeModal); ```이런식으로 아토믹하게 나누어서 커스텀훅으로 불러와 사용하는 방법으로 바꾸었습니다!

이제는 진짜 state인 것들과 action을 분리해주는 것은 어떨까요 ?

action을 분리하면서 진짜 상태인 값들과, 상태를 업데이트하는 함수들을 분리할 수 있고, action 안에 포함된 함수들은 하나의 커스텀 훅으로 export하면서 성능에 영향을 주지 않고 액션과 관련된 함수들을 동시에 노출시킬 수 있어요 !

티키 노션의 아티클란에 있는 Zustand 한번 읽어보시면 좋을 것 같아요 !

rtttr1 · 2024-08-02T09:12:22Z

src/shared/store/useWorkSpaceContext.tsx

+
+  switch (step) {
+    case 1:
+      return <WorkSpaceName />;
+    case 2:
+      return <WorkSpaceCategory />;
+    case 3:
+      return <WorkSpaceImage />;
+    case 4:
+      return <WorkSpaceComplete />;
+    default:
+      return null;
+  }


뭐하세요 다은시?

rtttr1 · 2024-08-02T09:19:09Z

src/shared/store/useWorkSpaceContext.tsx

+  setFileUrlData: (url: string) => void;
+}
+
+const WorkSpaceContext = createContext<WorkSpaceContextType | undefined>(undefined);


이거 타입스크립트를 사용해서 context state의 타입을 지정해주면 초기값을 지정안해줘도 IDE에서 자동완성이 다 뜨나요?? 제가 자스로만 해서 공부했을땐 context를 생성할때 초기값을 설정 안해주면 자동완성이 안뜬다고 배워가지고 궁금해서 여쭤봅니다!

맞아요..! 초기값 설정안해서 자동 완성은 안됩니다...! 그래서 초기값 설정하지 않아도 undefined가 될 수 있게 하는 코드

export const useWorkSpaceContext = () => { const context = useContext(WorkSpaceContext); if (!context) { throw new Error('useWorkSpaceContext must be used within a WorkSpaceProvider'); } return context; }; ``` 를 추가해 주었습니다!

rtttr1 · 2024-08-02T09:20:20Z

src/shared/store/useWorkSpaceContext.tsx

+
+  return (
+    <WorkSpaceContext.Provider
+      value={{ step, name, category, fileUrlData, setName, setCategory, setFileUrlData, nextStep, reset }}>


화랑씨의 아티클로 안 사실인데 setState를 prop으로 바로 넘기는게 좋지 않다고 하네요! 번거로울수 있고 코드가 늘어나긴 하겠지만 setState 함수들을 Handler함수를 선언해서 한번 감싸서 전달하거나 익명함수로 전달하는것도 좋을것 같다는 생각이 들었어요~~!!

좋은 의견 감사합니다!! 반영 완료 했습니다!

Bowoon1216

역시 기대를 저버리지 않는 채원님.. 고민을 많이 한게 느껴지는 코드네요.
로직을 모두 갈아 엎다니, 정말 수고 많으셨습니다! 😄

Bowoon1216 · 2024-08-01T21:30:04Z

src/common/component/Modal/ModalManager.tsx

+const ModalManager = () => {
+  const { isOpen, content, closeModal } = useModalStore();
+
+  if (!isOpen || !content) return null;


if 조건문 안에 or 연산 넣은거 깔끔하고 이해도 잘되네용 👍

감사합니당!

wuzoo

고생하셨습니다 채원님 ~! 몸도 아프셨는데 정말 신경써서 리팩토링 해주셨네요... 리뷰 남긴 것들 참고해보시면 좋을 것 같습니다 !

wuzoo · 2024-08-06T17:09:11Z

src/shared/store/modal.tsx

+      return { id: null, content: null, isOpen: false, closeCallback: null };
+    });
+  },
+}));


채원님 ! 저번에 제가 Zustand관련 리팩토링 PR에서 말씀드렸다시피, 최소한의 아토믹한 selector를 사용하고, 이를 따로 커스텀 훅에 적용하는 것 어떠세요 ??

렌더링과 소비자 입장에서 좋을 것 같아요 !

wuzoo · 2024-08-06T17:23:27Z

src/shared/component/createWorkSpaceModal/modalContents/category/WorkSpaceCategory.tsx

+  const { data } = useCategoryListQuery();
+  const categoryList = data?.data?.categories?.filter((category) => category !== '전체') || [];


Query의 select 옵션 고려해보시면 좋을 것 같아요 ! select 옵션을 사용하면, 기본적으로 데이터가 존재할때만 호출되기 때문에 undefined일 때를 고려하지 않아도 될 거에요 !

한 번 적용해보시면 좋을 것 같습니다.

좋습니당!! 카테고리 리스트 불러오는 게 쇼케이스 페이지, 그리고 워크스페이스 카테고리 두 군데에서 현재 쓰이고 있어서 ,,!
useCategoryListQuery에서 '전체'가 포함되냐 안되냐 를 includeAll의 boolean 값을 통해 구분하였습니다!

// 쇼케이스 페이지 '전체' 포함 ==> includeAll = true // 모달 내 사용할 때 '전체' 미포함 ==> includeAll = false const useCategoryListQuery = (includeAll = true) => { return useSuspenseQuery({ queryKey: ['category'], queryFn: () => getCategoryList(), select: (data) => { const categories = data?.data?.categories || []; return includeAll ? categories : categories.filter((category) => category !== '전체'); }, }); };

wuzoo · 2024-08-06T17:24:44Z

src/shared/component/createWorkSpaceModal/modalContents/image/WorkSpaceImage.tsx

+          nextStep();
+        },
+      }
+    );
  };

  return (


생각해보니 밑에 JSX문에 label 같은 것들은 저희가 기존에 만들어놨던 공통 컴포넌트 라벨 쓰면 좋지 않을까요 ??

추가적으로 각 모달의 Step마다 바뀌지않는 title 같은 텍스트들은 상수화 해주면 좋을 것 같네요 !!

label은 반영 완료 했습니다!! 그리고 각 모달의 Step마다 바뀌지않는 title 같은 텍스트들이

<WorkSapceInfo step="image" title="동아리 프로필 이미지 등록" info="우리 동아리의 프로필에 표시할 이미지를 등록해주세요" /> ``` 여기의 title을 의미하는게 맞을까요??

네네네 ! 맞긴 하다만, 지금 생각해보면 약간 굳이 인것 같기도 싶어요 ! 약간 하면 좋고, 안해도 무방이라는 느낌이네요

wuzoo · 2024-08-06T17:26:34Z

src/shared/hook/common/useBlockContext.tsx

+  nextStep: () => void;
+  reset: () => void;
+  formData: BlockFormData;
+  setFormData: (data: Partial<BlockFormData>) => void;


유틸리티 타입 👍

wuzoo · 2024-08-06T17:28:25Z

src/shared/hook/common/useWorkSpaceContext.tsx

Context에서 생성하는 함수들 useCallback 처리해주는 것은 어떨까요 ? 소비자가 해당 함수들을 사용할 때 최적화를 고려해볼 수도 있겠네요

좋습니다! 반영완료 했습니다!

wuzoo · 2024-08-06T17:34:44Z

src/shared/component/createWorkSpaceModal/modalContents/image/WorkSpaceImage.tsx

+      const newFileURL = URL.createObjectURL(file);
+      setFileURL(newFileURL);


뭔가 query의 enable과 같은 옵션과 함께, 이벤트 핸들러 안의 동작에서 해당 로직을 트리거한다면 useEffect를 안쓸 수도 있을 것 같다는 생각... 이 드네요.

아니면 적어도 이펙트의 의존성에 의한 반응형이 아닌 로직들은 외부 함수로 분리하여, 해당 함수는 이펙트 안에서만 실행시키도록 리팩토링이 가능할 것 같아요 !

handleImageChange 함수가 파일 선택 이벤트를 직접 처리하고, 이후에 refetchFileData 함수를 통해 새로운 파일의 프리사인드 URL을 가져온 후 handleFileUpload 함수를 호출하는 형식으로 바꿨습니당!! 코드 한번 확인해주세요!

wuzoo

채원님! 최근에 아프셨던 일도 있었고, 또 2박3일 동안 예상치 못하게 고생하셨던 일도 있으셨는데 .. 꾸준히 작업 최선을 다해주셔서 정말 정말 감사해요 !

티키에서 가장 잘하고 있으니 조급해하지말고 천천히 해주셔도 됩니다 ㅎㅎ! 가장 믿음직한 개발자에요 채원님은 !

고생하셨습니다 ~ (위에 저렇게 말해놓고 리뷰는 많이 남겼어요 ㅎㅎ...)

wuzoo · 2024-08-14T05:06:08Z

src/shared/store/modal.tsx

+      return { id: null, content: null, isOpen: false, closeCallback: null };
+    });
+  },
+}));


이제는 진짜 state인 것들과 action을 분리해주는 것은 어떨까요 ?

action을 분리하면서 진짜 상태인 값들과, 상태를 업데이트하는 함수들을 분리할 수 있고, action 안에 포함된 함수들은 하나의 커스텀 훅으로 export하면서 성능에 영향을 주지 않고 액션과 관련된 함수들을 동시에 노출시킬 수 있어요 !

wuzoo · 2024-08-14T05:06:56Z

src/shared/store/modal.tsx

+      return { id: null, content: null, isOpen: false, closeCallback: null };
+    });
+  },
+}));


티키 노션의 아티클란에 있는 Zustand 한번 읽어보시면 좋을 것 같아요 !

wuzoo · 2024-08-14T05:18:03Z

src/shared/component/LeftSidebar/LeftSidebar.tsx

+  const handleItemClick = (id: string, path: string) => {
+    setSelectedId(id);
+    if (id !== 'showcase') {
+      setTeamId(id);
+      localStorage.setItem('teamId', id);
+    } else {
+      localStorage.removeItem('teamId');
+    }
+    navigate(path);


teamId를 로컬스토리지에 저장하는 것이 아니라 url의 query params에 저장하기로 했어요 채원님 !

따라서 ?teamId= 처럼 url에 저장하는 대신 로컬스토리지에는 저장하지 않기로 했으니,

setSelectedId(id); navigate(`${PATH.ARCHIVING}?teamId=${teamId}`); close();

정도만 있으면 될 것 같습니다 !

사실 이제는 teamId에 대한 전역상태관리도 빼버려도 될 것 같네요 !

wuzoo · 2024-08-14T05:22:49Z

src/shared/component/LeftSidebar/LeftSidebar.tsx

  useEffect(() => {
    const teamId = localStorage.getItem('teamId');
    if (teamId) {
      setTeamId(teamId);
-      setClicked(teamId);
+      setSelectedId(teamId);
      navigate(`${PATH.ARCHIVING}?teamId=${teamId}`);
+    } else {
+      setSelectedId('showcase');
    }
  }, [setTeamId, navigate]);


여기서도 이제는 teamId를 url params 에 대한 값으로 가져온다음에 setSelectedId 적용하면 될듯 !!

wuzoo · 2024-08-14T05:40:16Z

src/page/archiving/index/ArchivingPage.tsx

+  const openModal = useOpenModal();
+
+  const handleOpenBlockModal = () => {
+    openModal(<BlockFlow />);


여기서 든 생각은, 제가 처음에 얘기했다시피 전역 상태로 content에 관한 컴포넌트를 가지고 있으면 더 편할 것 같다.. 라고 생각이 들긴 했습니다.

만약 그렇게 된다면, 호출하는 컴포넌트에서는 모달의 content 타입에 맞는 string 값을 호출하기만 하면 되기 때문이라고 생각해요.
뭔가 지금은 직접 openModal을 통해 BlockFlow라는 컴포넌트를 import 해와서 호출하고 있으니 결합도가 높다는 느낌을 받았어요.

openModal('create-block');

과 같이 컨텐츠 타입에 관한 특정 인자만 호출해준다면, 전역 상태의 content가 수정되고 이에 따라 ModalContainer 에서의 렌더링되는 플로우 컴포넌트가 바뀌는 과정으로 구현 된다면 더 좋을 것 같다고 생각이 드네요 ..

wuzoo · 2024-08-14T05:48:57Z

src/shared/component/createWorkSpaceModal/info/WorkSpaceInfo.tsx

현재 지금 shared/component 안에 createWorkSpaceModal이 있고, 이 안에 다시 hook과 component 폴더가 있어요.

일단 첫번 째로 개선해야될 점은 가독성, 즉 다른 팀원이 해당 폴더를 방문했을 때 어떤 파일인지를 빨리 알 수 있었으면 좋겠어요. 저는 차라리 WorkspaceModal 폴더 안에 info.tsx, category.tsx, complete.tsx 이런 식으로 있다면 훨씬더 빨리 알 수 있을 것 같아요. 왜냐하면 결국 info 파일, category 파일, complete 파일, image 파일 등등이 모두 워크스페이스 모달이라는 컴포넌트에 의존하므로 바로 1 depth 밑에 다 적어주는게 오히려 가독성에 좋을 것 같아요.

그렇다면 hook은 사실 component 안에서 사용되는 훅들이고, 해당 컴포넌트는 전역에서 컨트롤이 되는 상태이므로 shared/hook 으로 뺴는게 맞다고 생각합니다. 그렇다면 더욱 깔끔해질 것 같아요.

다른 분들도 피드백 해주세요 !

wuzoo · 2024-08-14T05:50:20Z

src/shared/component/createWorkSpaceModal/modalContents/image/WorkSpaceImage.tsx

+          nextStep();
+        },
+      }
+    );
  };

  return (


네네네 ! 맞긴 하다만, 지금 생각해보면 약간 굳이 인것 같기도 싶어요 ! 약간 하면 좋고, 안해도 무방이라는 느낌이네요

wuzoo · 2024-08-14T05:56:37Z

src/page/archiving/createTimeBlock/component/Block/BlockModal.tsx

추가적으로 폴더명이 동사 + 목적어 형태의 문장이 되어버리니까 좀 어색한 느낌이 드는 것 같아요.

createTimeBlock이나 createWorkSpaceModal 둘다 timeBlockModal 형태로 바꾸는 것은 어떤가요 ?

wuzoo · 2024-08-14T06:01:03Z

src/shared/component/ModalFlow/WorkSpaceFlow.tsx

+  return (
+    <>
+      <WorkSpaceName step={step} />
+      <WorkSpaceCategory step={step} />
+      <WorkSpaceImage step={step} />
+      <WorkSpaceComplete step={step} />
+    </>
+  );


위에 block flow나 지금의 workspace flow 둘 다 prop 네이밍을 다시 한번 고려해보면 좋을 것 같네요.

step이라고 표현하는 것보다는 "현재가 내 차례"이냐를 나타내야 하니까 isCurrent 혹은 isVisible로 표현해주고,
하위 스텝 컴포넌트들을 보여주고 말고는 Flow 컴포넌트가 해야할 역할이므로, step === 특정 스텝 과 같이 boolean 값을 넘겨주어서, 해당 스텝 컴포넌트들은 보여지고 말고만 판단하면 더 좋을 것 같네요 !

wuzoo

아직 다음 스프린트 기간까지 시간이 조금 더 남았으니, WorkSpaceImage 파일 리팩토링 한번 해보면 좋을 것 같아요. 비단 해당 파일이 아니라, 약간 상태관리나 로직을 보았을 때 가독성이 떨어지거나 효율적이지 못한 코드 ? 들 또한 리팩토링 해보면 좋을 것 같아요.

일단 첫 번째로, 하나의 함수에 너무 많은 역할이 수행되다보니까, 함수형 프로그래밍에서의 순수함수의 본질을 잃어버린 느낌이 드는 것 같아요. 가독성도 떨어질 수 밖에 없다고 생각하구요 !!
두 번째로, 분리할 수 있는 이미지 업로드에 대한 로직들은 커스텀 훅으로 위임시켜 비즈니스 로직을 분리해봐도 좋을 것 같아요. 그렇다면 가독성도 좋고, 비교적 결합도도 낮은 컴포넌트를 만들 수 있을 것 같다는 생각이 듭니다 !
세 번째로는, useQuery를 잘 사용하는 방법은 저는 "선언적"으로 사용하는 것이라고 생각해요. 즉, 서버의 상태를 관리하게 해주는 작업은 모두 쿼리에게 맡기고, 우리가 이로부터 따로 명령형으로 호출하거나 하지 않는다는 것이죠 !
예를 들면, refetch 또한 우리가 다시 데이터를 패칭해오도록 명령하는 것이 아니라, 특정 상태를 쿼리 키로 지정하면서 자연스럽게 리패칭되도록 "선언"하는 것처럼 말이에요 !

presigned url 까지 들어가니까 하나의 컴포넌트에서 너무 많은 역할을 수행할 수밖에 없게 되는 거 같아요 ㅜ. 채원님께서 정말 코드 잘 짜주셨지만, 채원님이 보았을 때도 개선할 점들이 눈에 보인다면 아직 시간이 많으니 충분히 리팩토링해봐도 좋을 것 같다는 생각입니다 !!

좋은 코드를 알아야 리팩토링할 수 있는 것이고, 또 리팩토링을 하면서 좋은 코드를 깨달아간다고 생각해요.

혼자 고민해보고 파헤쳐보는 것도 좋지만, 정말 정답을 모르겠다 싶은 것은 자유롭게 슬랙이나 카톡을 통해 팀원들과 공유해봐도 좋아요 채원님 ! ㅎㅎ

항상 고생이 많은 갓기 ! 화이팅 !

cindy-chaewon added 9 commits August 1, 2024 05:22

#187 feat: 모달 상태 store

23e3d6d

#187 fix: 모달 상태 store toggle 추가

2f33596

#187 feat: 모달 zustand로 띄우기

1034e74

#187 fix: 모달 닫힐 때 workspace 클릭 상태 해제

f566a02

#187 fix: 타임블록 생성도 적용 & 오류 해결

13021a7

#187 fix: 사진 미리보기 안되던 오류 해결

52a7280

#187 feat: delete 모달에도 적용

73ca6e8

#187 refactor: 필요없는 코드 삭제

77ee9fc

#187 fix: 절대경로 수정

5974228

cindy-chaewon added ♻️Refactor ✨Feature 🍀채원 labels Aug 1, 2024

cindy-chaewon requested review from rtttr1, namdaeun, Bowoon1216 and wuzoo August 1, 2024 11:37

cindy-chaewon self-assigned this Aug 1, 2024

wuzoo reviewed Aug 1, 2024

View reviewed changes

namdaeun reviewed Aug 1, 2024

View reviewed changes

#187 refactor: 워크스페이스, 블록생성

50f68e9

namdaeun reviewed Aug 1, 2024

View reviewed changes

cindy-chaewon added 2 commits August 2, 2024 02:01

#187 fix: 오류 해결

da808c4

#187 fix: 불필요한 코드 제거

2535167

wuzoo reviewed Aug 2, 2024

View reviewed changes

namdaeun reviewed Aug 2, 2024

View reviewed changes

wuzoo changed the title ~~모달 띄우는 로직, 데이터 상태 로직 리팩토링..~~ [Refactor] 모달 띄우는 로직, 데이터 상태 로직 리팩토링.. Aug 2, 2024

rtttr1 reviewed Aug 2, 2024

View reviewed changes

Bowoon1216 approved these changes Aug 3, 2024

View reviewed changes

cindy-chaewon added 2 commits August 6, 2024 22:20

#187 refactor: 코드리뷰 반영

3a1feff

#187 fix: 스토리북 코드 수정

a741a89

wuzoo reviewed Aug 6, 2024

View reviewed changes

cindy-chaewon added 3 commits August 13, 2024 23:43

#187 refactor: 코드리뷰 반영

44f9b0c

#187 fix: 스토리북 모달 수정

503f75f

#187 fix: useCallback 제거

bedbb83

wuzoo reviewed Aug 14, 2024

View reviewed changes

wuzoo reviewed Aug 15, 2024

View reviewed changes

cindy-chaewon added 2 commits September 14, 2024 00:25

#187 refactor: 삭제 모달 zustand 로

f68a155

#187 refactor: 리팩토링

f825ceb

		const { mutateAsync: blockMutate } = useDeleteBlockMutation();
		const { mutateAsync: documentMutate } = useDeleteDocumentMutation();

		const { data } = useCategoryListQuery();
		const categoryList = data?.data?.categories?.filter((category) => category !== '전체') \|\| [];

		const newFileURL = URL.createObjectURL(file);
		setFileURL(newFileURL);

[Refactor] 모달 띄우는 로직, 데이터 상태 로직 리팩토링.. #197

Are you sure you want to change the base?

[Refactor] 모달 띄우는 로직, 데이터 상태 로직 리팩토링.. #197

Conversation

cindy-chaewon commented Aug 1, 2024 • edited by wuzoo Loading

해당 이슈 번호

체크리스트

📌 내가 알게 된 부분

두번째 리팩토링

📌 질문할 부분

📌스크린샷 (선택)

wuzoo left a comment

Choose a reason for hiding this comment

namdaeun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namdaeun Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindy-chaewon Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namdaeun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namdaeun Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtttr1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bowoon1216 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindy-chaewon commented Aug 1, 2024 •

edited by wuzoo

Loading

namdaeun left a comment •

edited

Loading

namdaeun Aug 2, 2024 •

edited

Loading

cindy-chaewon Aug 13, 2024 •

edited

Loading

namdaeun left a comment •

edited

Loading

namdaeun Aug 2, 2024 •

edited

Loading