[ Refactor ] JuniorPromiseRequest 라우팅 분리 #221

ijieun · 2024-08-12T15:07:50Z

#️⃣ Related Issue

Closes #218

✅ Done Task

야리무와 합쳐져 있던 약속신청 파일들 이사 보내기 (이사한 장소: juniorPromiseRequest)
라우팅 페이지 생성하기
선배리스트 페이지에서 약속 신청 페이지로 이동하는 로직 코드 변경

☀️ New-insight

✨ useNavigate 의 두번째 인자에 state 값을 넣어 useLocation로 전달받을 수 있다.

값 넣는 코드
state에 선배 id와 선배 닉네임을 넣음.

        navigate('/juniorPromiseRequest', {
          state: {
            seniorId: seniorId,
            seniorNickname: seniorNickname,
          },
        })

값 받는 코드
location.state에서 선배 id 와 선배 닉네임을 받아와서 변수에 저장함.

const [seniorId] = useState(location.state?.seniorId);
const [seniorNickname] = useState(location.state?.seniorNickname);

💎 PR Point

🌿약속신청 파일들 이사 보내기

juniorPromiseRequest 폴더를 새로 만들어서 파일들 전부 이사 보냈습니다!
pages/juniorPromiseRequest/ 아래에 자잘한 컴포넌트들을 담는 상위 컴포넌트인 JuniorPromiseRequestPage 페이지를 생성했습니다.

🌿선배리스트 페이지에서 약속 신청 페이지로 이동하는 로직 코드 변경

본래 조건부 렌더링으로 isPromiseClicked가 true가 되면 <JuniorPromiseRequestPage seniorId={seniorId} seniorNickname={seniorNickname} /> 컴포넌트가 렌더링 되도록 구현되어있었는데요, useNavigate를 이용해서 페이지가 navigate되도록 수정했습니다.
그리고 이때, JuniorPromisePage 에서 선택한 선배의 id와 nickname을 navigate의 두번째 인자에 넣어서 선배 id 와 nickname을 useLocation으로 가져올 수 있도록 수정했습니다.

  const location = useLocation();
...
  const [seniorId] = useState(location.state?.seniorId);
  const [seniorNickname] = useState(location.state?.seniorNickname);

파일이 분리되어서 코드 찾기 편해지고, 라우팅이 분리되어서 선배 리스트 url 부터 확인하지 않아도 되어서 넘 편하네요 .. 😏

📸 Screenshot

url 부분을 주목해주세요!!

2024-08-12.11.53.37.mov

se0jinYoon

라우팅 분리하니까 조타 ... 코멘트 확인해주세요!

se0jinYoon · 2024-08-19T13:12:31Z

src/pages/juniorPromise/JuniorPromisePage.tsx

      {isPromiseClicked ? (
-        <PromiseRequestPage seniorId={seniorId} seniorNickname={seniorNickname} />
+        // 약속 신청하기 클릭하면 후배 약속 신청 페이지로 이동
+        navigate('/juniorPromiseRequest', {
+          state: {
+            seniorId: seniorId,
+            seniorNickname: seniorNickname,
+          },
+        })


p1) 기존에는 약속신청하기가 눌리면 다른 컴포넌트를 하나의 파일에서 보여줘야해서 isPromiseClicked의 상태값을 가지고 렌더링을 했었는데, 지금은 약속신청하기 버튼이 눌리면 Navigate 하도록 동작을 추가하면 되기때문에 이 state를 지워도 될 것 같아요! 지우고, 약속 신청하기가 눌렸을 때 실행되는 함수의 로직 내부에 Navigate하도록 연결해주면 될 것 같슴니다~

state 없이 navigate가 되기 때문에 상태관리는 안해도 되겠군요!! 반영했슴다 :>

se0jinYoon · 2024-08-19T13:13:34Z

src/pages/juniorPromise/JuniorPromisePage.tsx

+            seniorId: seniorId,
+            seniorNickname: seniorNickname,
+          },
+        })
      ) : isSeniorCardClicked ? (


p5) 기억 안 남 이슈인데 여기서 isSeniorCardClicked로 분기처리되는 건 어떤거였죠 ..?

첫번째 사진 화면에서 선배 카드를 선택하면 isSeniorCardClicked가 true가 되어 그 다음 페이지가 조건부로 보여지는 것입니다!

이부분 혹시 이렇게 구현되어있는 것에 대해 다른분들은 어떻게 생각하실지 궁금해여!

목록 페이지 -> 하나의 상세페이지로 들어가게 되는건데,
이 부분의 구현 방식이

상세 페이지를 보여줄지 여부를 관리하는 state

누구의 상세 페이지를 보여줘야 할지를 나타내는 id state

그사람의 활동명을 관리하는 nickname state

이렇게 세개의 상태를 분리해서 관리하는게 비효율적이라고 생각돼요.
차라리 url을 분리해서 juniorPromise 에서 한 depth 더 들어가서 juniorPromise/1 (뒤에 붙는 숫자는 회원id) 이런식으로 가져가는게 더 자연스럽지 않을까요?
그렇게 되면 상세페이지로 갔을 때 뒤로가기 버튼에 걸리는 onClick 함수도 상세페이지를 off 하는 형태가 아닌, Navigate(-1), 즉 진짜 뒤로가기의 역할을 해줄 수 있게 될 것 같아요

결론적으로 쉽게 말하면,
state를 통해 컴포넌트를 보여줄지 여부를 on/off로 관리하는 방식은
모달을 띄우고, 끌때에만 적합하지, 페이지 이동간에는 자연스럽지 못한 방식이라고 개인적으로 생각합니다!1

저는 숭언니 의견에 동의합니다!! 선배 개인마다 하나의 세부 페이지를 가지니까, 회원 번호로 라우터를 가지도록 분리하는게 자연스러울 거 같아요!! 사실 juniorPromiseRequest 페이지에서 뒤로가기를 했을때 선배리스트 태초 마을로 돌아가져서 불편했거든요! 라우터가 분리된다면 이 문제도 고쳐질거 같아요!

요 라우팅 변경과 관련된 부분은 다른 이슈를 파서 진행중인건가요? 아니면 아직 반영이 안되어있는 건가요~!

오호 리드님 의견이 궁금해서 남겨놨었는데 이후에 이슈 파서 진행할 것 같습니다~!

se0jinYoon · 2024-08-19T13:14:12Z

src/pages/juniorPromiseRequest/JuniorPromiseRequestPage.tsx

+import { useNavigate } from 'react-router-dom';
+import { useLocation } from 'react-router-dom';


p1) 얘네 한 번에 작성해줍시다! eslint가 안먹히나 ?

넷!! 요거 합치는 eslint 는 따로 적용이 안되는거 가타요

요부분 확인해보고 따로 이슈파서 합쳐지도록 반영하겠습니다..!

se0jinYoon · 2024-08-19T13:15:41Z

src/pages/juniorPromiseRequest/JuniorPromiseRequestPage.tsx

+  const [seniorId] = useState(location.state?.seniorId);
+  const [seniorNickname] = useState(location.state?.seniorNickname);


p2) 흠 얘네를 state로 관리하는 이유가 있을까요? 한 번 받아오고 나서 해당 상태들을 업데이트 하는 로직이 이 컴포넌트에는 없어서 그냥 구조분해할당으로 받아와도 될 거 같아요

state 수가 줄어들어서 복잡성 감소에 좋겠군요 역시 멋져,,, 반영했슴다!!!

lydiacho

아주 잘하고 있어용 🤙🏻

lydiacho · 2024-08-19T14:58:18Z

src/pages/juniorPromise/JuniorPromisePage.tsx

+      state: {
+        seniorId: seniorId,
+        seniorNickname: seniorNickname,
+      },


Suggested change

state: {

seniorId: seniorId,

seniorNickname: seniorNickname,

},

state: {

seniorId,

seniorNickname,

},

p4) 이런건 축약해줄 수 있어여 !

변수 이름과 속성 이름이 동일할 때에 사용할 수 있겠네요!! 적용했습니다 :>

lydiacho · 2024-08-19T15:11:49Z

src/pages/juniorPromise/JuniorPromisePage.tsx

+            seniorId: seniorId,
+            seniorNickname: seniorNickname,
+          },
+        })
      ) : isSeniorCardClicked ? (


이부분 혹시 이렇게 구현되어있는 것에 대해 다른분들은 어떻게 생각하실지 궁금해여!

목록 페이지 -> 하나의 상세페이지로 들어가게 되는건데,
이 부분의 구현 방식이

상세 페이지를 보여줄지 여부를 관리하는 state

누구의 상세 페이지를 보여줘야 할지를 나타내는 id state

그사람의 활동명을 관리하는 nickname state

이렇게 세개의 상태를 분리해서 관리하는게 비효율적이라고 생각돼요.
차라리 url을 분리해서 juniorPromise 에서 한 depth 더 들어가서 juniorPromise/1 (뒤에 붙는 숫자는 회원id) 이런식으로 가져가는게 더 자연스럽지 않을까요?
그렇게 되면 상세페이지로 갔을 때 뒤로가기 버튼에 걸리는 onClick 함수도 상세페이지를 off 하는 형태가 아닌, Navigate(-1), 즉 진짜 뒤로가기의 역할을 해줄 수 있게 될 것 같아요

결론적으로 쉽게 말하면,
state를 통해 컴포넌트를 보여줄지 여부를 on/off로 관리하는 방식은
모달을 띄우고, 끌때에만 적합하지, 페이지 이동간에는 자연스럽지 못한 방식이라고 개인적으로 생각합니다!1

lydiacho · 2024-08-19T15:19:29Z

src/pages/juniorPromiseRequest/JuniorPromiseRequestPage.tsx

  useEffect(() => {
    setIsAllSelected(
      selectedTime.every((item) => item.selectedTime !== '' && item.clickedDay !== '') &&
-        (isAnyWorrySelected || isTextareaFilled),
+        (isAnyWorrySelected || isTextareaFilled)
    );
  }, [selectedTime, isAnyWorrySelected, isTextareaFilled]);


p2) isAllSelected 라는 state가 '모든 항목을 모두 다 잘 작성했는지'의 여부를 판단하는 state가 맞을까요?
그렇다면 굳이 별도의 stated와 useEffect로 관리하기 보다,
isAllSelected를 사용해주고 있는 약속신청하기 버튼 $isAllSelected prop에서 충족시켜줄 조건들을 검사하는 비교문을 넣어주는게 더 맞지 않을까 싶어요!!

쉽게 말하면,
지금 보면 결국 isAllSelected의 값을 좌지우지하는 변수들은 selectedTime, isAnyWorrySelected, isTextareaFilled 인데, 모두 useState와 종속되어있는 친구이기 때문에, 그 값의 변화를 이렇게 useEffect로 굳이 감시하지 않아도 값이 변하면 알아서 리렌더링이 되잖아요! 따라서 useEffect없이 그냥 Prop에서 자동으로 비교해줘도 충분히 원하는 목적을 달성할 수 있을 것 같아요 :)

만약 이해 안된다면 다시 코멘트 해주세요 ~

오호...!!! 그럼 이런식으로 버튼 $isAllSelected prop에서 조건식을 넣어주면 별다른 state 관리나 useEffect 사용 없이도 모든 항목을 작성했는지 체크가 가능하겠네요!!! 이게 바로 초수와 고수의 차이인가요? 적용했습니다!!

<button $isAllSelected={ selectedTime.every((item) => item.selectedTime !== '' && item.clickedDay !== '') && (isAnyWorrySelected || isTextareaFilled) } > 약속 신청하기 </button>

se0jinYoon · 2024-09-16T13:26:15Z

src/pages/juniorPromiseRequest/JuniorPromiseRequestPage.tsx

  const [activeButton, setActiveButton] = useState('선택할래요');
  const [isModalOpen, setIsModalOpen] = useState(false);
-  const [isAllSelected, setIsAllSelected] = useState(false);
+  const [isAllSelected] = useState(false);


p1) 이 부분에서 state값만 활용하는 거라면 아래에서 (79번줄)의 isAllSelected는 항상 false값을 가질텐데(setter함수로 업데이트하는 로직이 없어서) 그걸 의도한게 맞나요?

별도의 state로 이 부분을 관리하지 않을 생각이라면 79번줄에 isAllSelected이라는 state값이 들어가는게 아니라, isAllSelected를 만족하는 조건들을 조건문에 넣어줘야 할 것 같아요. 제가 로직을 잘못 이해한 것이라면말해주세요~!

반영했습니다!!

se0jinYoon · 2024-09-16T13:27:42Z

src/pages/juniorPromise/JuniorPromisePage.tsx

+            seniorId: seniorId,
+            seniorNickname: seniorNickname,
+          },
+        })
      ) : isSeniorCardClicked ? (


요 라우팅 변경과 관련된 부분은 다른 이슈를 파서 진행중인건가요? 아니면 아직 반영이 안되어있는 건가요~!

…-SEONYAK/SEONYAK_CLIENT into refactor/#218/separateRoute

se0jinYoon · 2024-09-23T13:11:29Z

src/pages/juniorPromiseRequest/JuniorPromiseRequestPage.tsx

-        ],
-      });
-    }
+    postAppointment({


p1) 근데 여기 기존에는 isAllSelected일 경우에만 실행되던 로직인 것 같은데, 해당 조건없이 실행되도록 두어도 괜찮은가요?

요게 첫번째 화면을 거쳐서 '적용할래요' 버튼을 누르면 그제서야 '약속 신청 post 요청'이 되는 로직입니다!!
원래 중복 검사를 했지만, 어차피 첫번째 화면처럼 모달이 open 되기 위해서는 isAllSelected 검사를 먼저 수행하고, '적용할래요' 버튼을 누르는 시점에서는 이미 검사를 수행한 이후라 불필요한 검사 과정인 것 같아서 뺐습니당!! 기억력 좋타.. 혹시 중복 검사를 하는게 더 낫다고 생각하시나요 ??!

요 화면에서 검사 진행함다

오호 아니요!! 굿입니다~~~ 머지해주세여!!

[ Fix ] self QA - 뒤로가기 버튼 수정, 약속신청 모달 위치 조정, 이미 있는 약속 신청시 alert

lydiacho

수고하셨습니다!! 코멘트 확인해주시고 수정하신 후에 머지하시면 될 것 같아요 ~~

커밋 21개 슬퍼요 담부터는 꼬옥 꼭 브랜치 쌈뽕하게 관리해주기 !_!

lydiacho · 2024-09-23T13:25:21Z

src/pages/juniorPromiseRequest/JuniorPromiseRequestPage.tsx

+    const isAllSelected =
+      selectedTime.every((item) => item.selectedTime !== '' && item.clickedDay !== '') &&


p2) 아래 $isAllselected에서 똑같은 식을 사용하고 있으니까 isAllselected const를 밖으로 빼줘서 컴포넌트에서 한번만 선언해주고 재사용해도 될 것 같아용!

중복 코드 줄여줘서 좋네요.. 반영했슴다!!

lydiacho · 2024-09-23T14:18:28Z

src/pages/juniorPromiseRequest/hooks/queries.ts

+    onError: (error) => {
+      let errorMessage = '알 수 없는 오류가 발생했습니다.';
+      if (axios.isAxiosError(error) && error.response) {
+        if (error.response?.status === 403 || error.response?.status === 400) {
+          errorMessage = '이미 약속을 신청한 선배입니다.';
+        } else {
+          errorMessage = '약속 신청에 실패했습니다.';
+        }
+      }
+      if (onErrorCallback) {
+        onErrorCallback(errorMessage);
+      }
+    },


p2) 이부분 코드가 살짝 어색한 것 같아요
react에서는 let을 잘 사용하지 않는데..! let을 사용해서 굳이 message 변수 값을 관리해줘야할까요?

그리고 이미 약속을 신청한 선배일 때 발생하는 에러 코드는 하나일텐데 왜 400과 403 에러 코드 둘다 union으로 처리해주고 있는지 궁금해요! 오히려 이미 약속 신청한 선배일 때 발생하는 에러 코드는 한개이고, 그 외의 모든 에러에 대해서는 더 범용적인 메시지인 '약속 신청에 실패했습니다' 메시지를 쓰는게 더 자연스럽지 않을까용??

오호 그런데 400 403 에러가 둘다 '이미 약속 신청한 선배' 에러더라구요!!
한번 서버파트에게 에러 코드 문의를 해봐야겠네요 .. 요 부분 다시 고민해보겠슴다!!

ijieun added 2 commits August 12, 2024 23:30

refactor: juniorPromiseRequest 파일 이동

511f901

refactor: 라우터 분리, 페이지 이동 방식 변경

89b7e92

ijieun added ♻️ Refactor 코드 리팩토링 지은 labels Aug 12, 2024

ijieun requested review from lydiacho and se0jinYoon August 12, 2024 15:07

ijieun self-assigned this Aug 12, 2024

github-actions bot added the size/m label Aug 12, 2024

se0jinYoon requested changes Aug 19, 2024

View reviewed changes

ijieun added 7 commits August 19, 2024 22:27

fix: react-router-dom의 import문 합치기

25e8f0b

refactor: state를 사용하지 않고 navigate 이동하도록 코드리뷰 반영

198d605

refactor: state없이 구조분해할당으로 받아오도록 코드리뷰 반영

fc0f7f4

fix: header < 클릭시 이전 페이지 이동

e619cd1

chore: 컴포넌트 제거

4cd44d2

design: 약속 신청 모달 가운데로 정렬

6c83100

fix: 이미 있는 약속을 신청할 시에 alert알림

b99474d

lydiacho requested changes Aug 19, 2024

View reviewed changes

ijieun and others added 3 commits August 23, 2024 14:17

fix: 변수, 속성 이름 축약

90020eb

refactor: 약속신청 버튼 인라인 조건부 렌더링

d7bd928

Merge branch 'develop' into refactor/#218/separateRoute

a3e410b

se0jinYoon reviewed Sep 16, 2024

View reviewed changes

ijieun and others added 5 commits September 17, 2024 12:28

refactor: isAllSelected 상태 함수 제거

6854730

Merge branch 'refactor/#218/separateRoute' of https://github.com/TEAM…

96d4527

…-SEONYAK/SEONYAK_CLIENT into refactor/#218/separateRoute

refactor: isAllSelected 상태 함수 제거

65a9cb8

Merge branch 'refactor/#218/separateRoute' of https://github.com/TEAM…

cd556c4

…-SEONYAK/SEONYAK_CLIENT into refactor/#218/separateRoute

refactor: 에러 핸들링 로직 분리

940f74f

se0jinYoon reviewed Sep 23, 2024

View reviewed changes

ijieun and others added 3 commits September 23, 2024 22:31

Merge branch 'refactor/#218/separateRoute' into fix/#222/patchSelfQA

d77d769

Merge pull request #225 from TEAM-SEONYAK/fix/#222/patchSelfQA

8a7345a

[ Fix ] self QA - 뒤로가기 버튼 수정, 약속신청 모달 위치 조정, 이미 있는 약속 신청시 alert

fix: 변수 맞춤법 수정

3e7f505

Merge branch 'develop' into refactor/#218/separateRoute

163adc4

github-actions bot added size/l and removed size/m labels Sep 23, 2024

se0jinYoon self-requested a review September 23, 2024 14:09

se0jinYoon approved these changes Sep 23, 2024

View reviewed changes

lydiacho approved these changes Sep 23, 2024

View reviewed changes

ijieun added 2 commits September 23, 2024 23:36

refactor: 중복 코드 합치기

f07cb14

refactor: 약속 신청 에러 핸들링 로직 변경

d1335ad

ijieun merged commit a6a879f into develop Sep 25, 2024
1 check passed

ijieun deleted the refactor/#218/separateRoute branch September 25, 2024 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Refactor ] JuniorPromiseRequest 라우팅 분리 #221

[ Refactor ] JuniorPromiseRequest 라우팅 분리 #221

ijieun commented Aug 12, 2024

se0jinYoon left a comment

se0jinYoon Aug 19, 2024

ijieun Aug 19, 2024

se0jinYoon Aug 19, 2024

ijieun Aug 19, 2024

lydiacho Aug 19, 2024

ijieun Aug 23, 2024

se0jinYoon Sep 16, 2024

ijieun Sep 19, 2024

se0jinYoon Aug 19, 2024

ijieun Aug 19, 2024

yarimu Sep 4, 2024

se0jinYoon Aug 19, 2024

ijieun Aug 19, 2024

lydiacho left a comment

lydiacho Aug 19, 2024

ijieun Aug 23, 2024

lydiacho Aug 19, 2024

lydiacho Aug 19, 2024

ijieun Aug 23, 2024 •

edited

Loading

se0jinYoon Sep 16, 2024

ijieun Sep 19, 2024

se0jinYoon Sep 16, 2024

se0jinYoon Sep 23, 2024

ijieun Sep 23, 2024 •

edited

Loading

se0jinYoon Sep 23, 2024

lydiacho left a comment

lydiacho Sep 23, 2024

ijieun Sep 23, 2024

lydiacho Sep 23, 2024

ijieun Sep 23, 2024

		import { useNavigate } from 'react-router-dom';
		import { useLocation } from 'react-router-dom';

		const [seniorId] = useState(location.state?.seniorId);
		const [seniorNickname] = useState(location.state?.seniorNickname);

		const isAllSelected =
		selectedTime.every((item) => item.selectedTime !== '' && item.clickedDay !== '') &&

[ Refactor ] JuniorPromiseRequest 라우팅 분리 #221

[ Refactor ] JuniorPromiseRequest 라우팅 분리 #221

Conversation

ijieun commented Aug 12, 2024

#️⃣ Related Issue

✅ Done Task

☀️ New-insight

✨ useNavigate 의 두번째 인자에 state 값을 넣어 useLocation로 전달받을 수 있다.

💎 PR Point

🌿약속신청 파일들 이사 보내기

🌿선배리스트 페이지에서 약속 신청 페이지로 이동하는 로직 코드 변경

📸 Screenshot

se0jinYoon 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

lydiacho 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

ijieun Aug 23, 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

ijieun Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lydiacho 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

ijieun Aug 23, 2024 •

edited

Loading

ijieun Sep 23, 2024 •

edited

Loading