Refactor/#143 킬링파트 등록 페이지 및 연관 컴포넌트 일괄 리팩토링 #179

Creative-Lee · 2023-08-01T18:46:47Z

📝작업 내용

SongDetailPage 컴포넌트 분리
VoteInterfaceContext 생성 / 관련 상태 및 로직 분리
VideoPlayerContext 생성 / 관련 상태 분리
연관된 컴포넌트 props 및 변수명 의미 개선

💬리뷰 참고사항

충돌이 **다수** 발생할 예정입니다.  
작업규모가 큰 제가 제일 마지막에 머지하는게 좋아 보여요. 
같이 모여서 충돌 해결 하시죠

작업 내용이 워낙 많아서 리뷰하기 힘드실 것 같습니다.
커밋로그로 리뷰하시면 그나마 수월하실 것 같아요.
혹은 저에게 10분 정도 전체 작업내용 요약해서 설명 들으시면 씽크도 잘 맞고 이해하기 쉬워질 것 같네요.

전체적으로 의도한바는 다음과 같습니다.

page단에 모여있던 상태와 꼬여있던 props정리
youtube 컴포넌트의 재사용성을 고려하여 Context 생성
VoteInterface 컴포넌트에만 사용될 Context 생성 및 로직 격리
import 경로 정리

TODO 주석 4개 넣어놨어요. 적당히 중요하다고 생각한 부분인데 처리하지 않았습니다.
당장 하지못하는 작업도 있고(API 관련 협의 필요)
무엇보다 페이지 내 로직에 대한 부분은 어느정도 정리가 되었다고 판단했어요.
작업 반경도 이미 충분히 커서 다른 이슈로 작업하는게 좋아보입니다.

#️⃣연관된 이슈

close #143
close #156
close #178

변수 자체에 시간의 의미를 포함

유튜브 ifram api는 https 요청을 받도록 고정되어있는 api이므로 수정하였음.

1. 컴포넌트 내부 불필요한 player 상태 삭제 2. 사용하지 않는 onStateChange 이벤트 핸들러 삭제 3. 외부에서 player 상태 및 setter 를 전달받도록 개선

VoteInterface 컴포넌트의 각 컴포넌트가 사용할 컨텍스트 생성

컨텍스트에서 제공하는 value에 해당하는 props 제거

1. 컨텍스트 내부에 상태 격리 2. page 단에 존재하기에 어색했던 구간반복 effetct 격리

1. 영상 시작점 props 옵셔널 및 기본값 처리 2. 향후 이벤트 사용 가능성을 고려하여, player set 형식 변경 3. 관련 함수 useCallback 처리

VoteInterface는 특정 컴포넌트 종속적인 provider인것에 비해 VideoPlayer 관련 컴포넌트와 상태는 재사용 할 가능성이 높다고 판단하였음. 이에 따라 provider를 분리하였음.

해당 과정에서 VoteInterfaceContext는 특정 컴포넌트 종속적임을 확정하였음.

github-actions · 2023-08-01T18:48:15Z

Unit Test Results

114 tests 114 ✔️ 3s ⏱️
  34 suites     0 💤
  34 files     0 ❌

Results for commit 3a89e6b.

♻️ This comment has been updated with latest results.

ukkodeveloper · 2023-08-02T08:38:12Z

frontend/src/components/VoteInterface/VoteInterfaceProvider.tsx

+interface VoteInterfaceContextProps {
+  partStartTime: TimeMinSec;
+  interval: KillingPartInterval;
+  updatePartStartTime: (timeUnit: string, value: number) => void;


💬 timeUnit을 union type으로 지정하는 것이 어떤가요?

ukkodeveloper

고생 많으셨어요 또밥. youtube player에 대한 리팩토링은 직접 이야기를 들어서 이해하기 편했습니다. 불필요한 부분 많이 걷어 내고 player 상태도 한 곳에서 관리할 수 있다는 점에서 많이 좋아졌어요. 나머지 부분은 코멘트로 달아놓았습니다.

frontend/src/components/Youtube/Youtube.tsx

ukkodeveloper · 2023-08-02T08:47:09Z

frontend/src/components/Youtube/api/loadIframeApi.ts

@@ -5,13 +5,11 @@ declare global {
  }
 }

-const PROTOCOL = window.location.protocol === 'http:' ? 'http:' : 'https:';


우리 서비스가 https:// 프로토콜을 사용하기 때문에 지우신 것 같은데 맞나요? 아주 그레잇이네요

api자체가 프로토콜 구분이 필요 없더라고요!
https로 리다이렉트 되도록 되어있습니당
그래서 삭제했어요 ~ㅎㅎ

ukkodeveloper · 2023-08-02T08:59:50Z

frontend/src/pages/SongDetailPage.tsx

-          </Share>
-        </Flex>
-      </Modal>
+      <VideoPlayerProvider>


💬 videoId를 Provider에서 주입 받아서 contextAPI로 뿌려주는 것은 어떨까요? 현재 코드로는 전역에 player을 하나만 둘 수 있을 것 같아서요. 도밥 의견이 궁금합니다~

같은 고민을 했었습니다 고려해보죠!

ukkodeveloper · 2023-08-02T09:36:36Z

frontend/src/components/VoteInterface/VoteInterface.tsx

+
+      <Modal isOpen={isOpen} closeModal={closeModal}>
+        <ModalTitle>킬링파트 투표를 완료했습니다.</ModalTitle>
+        <ModalContent>컨텐츠는 아직 없습니다.</ModalContent>


같이 의논해서 어떤 메세지를 보내줄지 고민해보면 좋을 것 같네요

ukkodeveloper · 2023-08-02T09:42:55Z

frontend/src/components/VoteInterface/VoteInterface.tsx

+      <RegisterTitle>당신의 킬링파트에 투표하세요🎧</RegisterTitle>
+      <KillingPartToggleGroup />
+      <IntervalInput
+        videoLength={videoLength}


💬 Provider에서 videoLength를 받고 Context로 뿌려주면, videloLength도 props drilling 할 필요 없을 것 같아요. 두밥 생각은 어떠신가요?

이것도 마찬가지로 고민했었어요! 고려해봅시다!

ukkodeveloper · 2023-08-02T12:32:13Z

frontend/src/components/VoteInterface/VoteInterfaceProvider.tsx

+      value={{
+        partStartTime,
+        interval,
+        updatePartStartTime,


updatePartStartTime 사용처에서 partStartTime의 react 상태를 변경하는 함수라고 계속 염두하고 있어야 할 것 같네요. 보통은 set prefix를 통해 상태를 변경한다고 예측하니까요.

setter 함수를 바로 넘기지 않고 함수로 한번 감싸는 것으로 변동에 용이하게 하려 했어요!
이름은 비슷한 늬양스를 주기위한 단어로 선택했습니다!

ukkodeveloper · 2023-08-02T12:40:27Z

frontend/src/components/VoteInterface/VoteInterface.style.ts

+
+export const ModalContent = styled.p`
+  font-size: 16px;
+  color: #b5b3bc;


💬 모달에 계속 사용할 색이라면 theme에 추가하는 거 어떠신가요?

좋습니다!

이미 만들어진 유틸 추가

cruelladevil

Context를 활용해서 페이지에서 컴포넌트를 분리하고 관심사를 분리하려고 들어간 노력들이 곳곳에 보이는 것 같아요 👍👍

함께 이야기 나눈 것처럼 작은 단위부터 고쳐나가다보면 만족스러운 리팩토링이 되지 않을까 싶어요!
(여기서는 이만 Approve 하겠습니다! 고생하셨습니다👍👍)

주말 같은날 시간이 괜찮다면 함께 리팩토링 해보는 것도 좋을 것 같네요 😊

cruelladevil · 2023-08-02T09:54:43Z

frontend/src/components/Youtube/Youtube.tsx

        })
      );
    } catch (error) {
      console.error(error);
-      console.error('[createYoutubePlayer] Youtube Player를 생성하지 못하였습니다.');
+      console.error('Youtube Player를 생성하지 못하였습니다.');


💬 유튜브 플레이어 에러시 어떻게 처리할 것인지 추후 고려해봐야 할 듯 하네요.

다시 로드할 수 있는 방식을 제공하면 좋을 것 같네요!

cruelladevil · 2023-08-02T15:34:13Z

frontend/src/utils/convertTime.ts

+    const timeString = timeSec.toString();
+
+    if (timeString.length === 1) {
+      return `0${timeString}`;
+    } else {
+      return timeString;
+    }
+  });


String.prototype.padStart()를 활용하는 것은 어떨까요?

제가 짠 코드 같은데 이게 왜 도밥 PR에 있죠 ㅋㅋㅋ String.prototype.padStart() 를 활용하면 분기처리하지 않아도 되어서 좋겠네요! 굳

시간이 부족해서 먼저 땡겨와서 반영시켰습니다

cruelladevil · 2023-08-02T15:39:18Z

frontend/src/components/VoteInterface/VoteInterfaceProvider.tsx

+
+  useEffect(() => {
+    const timer = window.setInterval(() => {
+      const startSecond = minSecToSeconds([partStartTime.minute, partStartTime.second]);


partStartTime이 객체로 관리될 것이라면, convertTime의 유틸함수도 튜플보다는 객체면 더 편할 것 같네요

const startSecond = minSectoSeconds(partStartTime);

// @/utils/convertTime const minSectoSeconds = ({ minute, second }: TimeMinSec) => { ... };

추후 시간 단위에 대한 관리를 개편하면서 한번에 수정해봅시다!

cruelladevil · 2023-08-02T15:43:20Z

frontend/src/components/VoteInterface/VoteInterface.tsx

+      <Modal isOpen={isOpen} closeModal={closeModal}>
+        <ModalTitle>킬링파트 투표를 완료했습니다.</ModalTitle>
+        <ModalContent>
+          <Message>{voteTimeText}</Message>
+          <Message>파트를 공유해 보세요😀</Message>
+        </ModalContent>
+        <ButtonContainer>
+          <Confirm type="button" onClick={closeModal}>
+            확인
+          </Confirm>
+          <Share
+            type="button"
+            onClick={() => copyUrlClipboard(killingPartPostResponse?.partVideoUrl)}
+          >
+            공유하기
+          </Share>
+        </ButtonContainer>
+      </Modal>


💬 Modal이 VoteInterface의 관심사라고 생각하는 것인지 궁금합니다!

우선 page 단에서 컴포넌트 내부로 옮겨두긴 했습니다만, 코난은 어떻게 생각하시는지용?

…into refactor/#143

기존 유튜브 컴포넌트를 활용한 새로운 page에 context 적용

…into refactor/#143

import 경로 수정

Creative-Lee added 22 commits July 30, 2023 13:52

refactor: 사용하지 않는 결과 메세지 및 태그 스타일 정리

33015e9

refactor: 변수명 수정

307da3d

변수 자체에 시간의 의미를 포함

refactor: 변수명 수정

fbbcb23

변수 자체에 시간의 의미를 포함

refactor: 불필요한 number 변환 코드 제거

4d629ae

refactor: 유튜브 iframe load api 불필요한 코드 수정

77d38cd

유튜브 ifram api는 https 요청을 받도록 고정되어있는 api이므로 수정하였음.

refactor(Youtube): 컴포넌트 리팩토링

502c93a

1. 컴포넌트 내부 불필요한 player 상태 삭제 2. 사용하지 않는 onStateChange 이벤트 핸들러 삭제 3. 외부에서 player 상태 및 setter 를 전달받도록 개선

refactor: SongDetailPage 컴포넌트 분리 및 계층 생성

cee2e3b

refactor: VoteInterfaceContext 생성 및 훅 추가

dcfaef7

VoteInterface 컴포넌트의 각 컴포넌트가 사용할 컨텍스트 생성

refactor: VoteInterfaceContext 적용 및 컴포넌트 props 수정

887ba83

컨텍스트에서 제공하는 value에 해당하는 props 제거

refactor: VoteInterfaceContext 적용에 따른 test case 수정

f08119f

refactor: songDetailPage에 존재하던 video 상태 및 구간반복 effect 컨텍스트 내부로 분리

5de5670

1. 컨텍스트 내부에 상태 격리 2. page 단에 존재하기에 어색했던 구간반복 effetct 격리

refactor: videoPlayer 상태 컨텍스트 분리로 인한 props 삭제

3c7d16a

refactor: Youtube 컴포넌트 개선

19daff2

1. 영상 시작점 props 옵셔널 및 기본값 처리 2. 향후 이벤트 사용 가능성을 고려하여, player set 형식 변경 3. 관련 함수 useCallback 처리

refactor: videoPlayerProvider 생성 및 player 관련 상태 분리

9638490

VoteInterface는 특정 컴포넌트 종속적인 provider인것에 비해 VideoPlayer 관련 컴포넌트와 상태는 재사용 할 가능성이 높다고 판단하였음. 이에 따라 provider를 분리하였음.

refactor: 분리된 videoPlayerContext를 각 컴포넌트에 적용

d5652ff

해당 과정에서 VoteInterfaceContext는 특정 컴포넌트 종속적임을 확정하였음.

chore: TODO 주석 추가

6623457

refactor: 하드코딩된 songId 대체할 props 추가

85fefd8

chore: TODO 주석 추가

f3315b3

design: 기본 박스 스타일링 및 컨테이너 추가

660f9d9

refactor: provider 태그 위치 변경으로 의미 강조

a47fbbb

refactor: index 파일 생성 및 import 경로 정리

1392674

refactor(Youtube): index 파일 생성 및 import 경로 정리

f64305b

Creative-Lee added [ 🌞 FE ] 프론트엔드 크루들의 빛나는 개발 이야기 하나둘셋 호! 🔨 Refactor 꾸준한 개선이 더 나은 애플리케이션을 만든다 labels Aug 1, 2023

Creative-Lee added this to the 노래 목록 및 상세 정보 조회 기능 구현 milestone Aug 1, 2023

Creative-Lee requested review from cruelladevil and ukkodeveloper August 1, 2023 18:46

Creative-Lee self-assigned this Aug 1, 2023

refactor: 테스트케이스에 추가된 Context 적용

f00a4df

ukkodeveloper reviewed Aug 2, 2023

View reviewed changes

ukkodeveloper approved these changes Aug 2, 2023

View reviewed changes

Creative-Lee added 2 commits August 2, 2023 22:54

design: 사용하지 않는 코드 삭제 및 모달버튼 간격 조정

67d9890

feat: 킬링파트 지정 구간을 계산하는 유틸 추가 및 메세지 적용

462f5bc

이미 만들어진 유틸 추가

cruelladevil approved these changes Aug 2, 2023

View reviewed changes

Creative-Lee added 4 commits August 3, 2023 11:06

Merge branch 'main' of https://github.com/woowacourse-teams/2023-shook …

8d7b3c6

…into refactor/#143

fix: songPage에 player context 적용

179d43a

기존 유튜브 컴포넌트를 활용한 새로운 page에 context 적용

Merge branch 'main' of https://github.com/woowacourse-teams/2023-shook …

044beaa

…into refactor/#143

fix: main pull 후 발생하는 오류 해결

3a89e6b

import 경로 수정

Creative-Lee merged commit fc5edf9 into main Aug 3, 2023

Creative-Lee deleted the refactor/#143 branch August 3, 2023 03:19

Creative-Lee restored the refactor/#143 branch August 3, 2023 04:35

Creative-Lee deleted the refactor/#143 branch August 11, 2023 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/#143 킬링파트 등록 페이지 및 연관 컴포넌트 일괄 리팩토링 #179

Refactor/#143 킬링파트 등록 페이지 및 연관 컴포넌트 일괄 리팩토링 #179

Creative-Lee commented Aug 1, 2023

github-actions bot commented Aug 1, 2023 •

edited

Loading

ukkodeveloper Aug 2, 2023 •

edited

Loading

ukkodeveloper left a comment

ukkodeveloper Aug 2, 2023

Creative-Lee Aug 2, 2023

ukkodeveloper Aug 2, 2023

Creative-Lee Aug 3, 2023

ukkodeveloper Aug 2, 2023

ukkodeveloper Aug 2, 2023

Creative-Lee Aug 3, 2023

ukkodeveloper Aug 2, 2023

Creative-Lee Aug 3, 2023

ukkodeveloper Aug 2, 2023

Creative-Lee Aug 3, 2023

cruelladevil left a comment

cruelladevil Aug 2, 2023

Creative-Lee Aug 3, 2023

cruelladevil Aug 2, 2023

ukkodeveloper Aug 3, 2023

Creative-Lee Aug 3, 2023

cruelladevil Aug 2, 2023

Creative-Lee Aug 3, 2023

cruelladevil Aug 2, 2023

Creative-Lee Aug 3, 2023

Refactor/#143 킬링파트 등록 페이지 및 연관 컴포넌트 일괄 리팩토링 #179

Refactor/#143 킬링파트 등록 페이지 및 연관 컴포넌트 일괄 리팩토링 #179

Conversation

Creative-Lee commented Aug 1, 2023

📝작업 내용

💬리뷰 참고사항

#️⃣연관된 이슈

github-actions bot commented Aug 1, 2023 • edited Loading

Unit Test Results

ukkodeveloper Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

ukkodeveloper 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

cruelladevil 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

github-actions bot commented Aug 1, 2023 •

edited

Loading

ukkodeveloper Aug 2, 2023 •

edited

Loading