Feat/#140 노래 조회 페이지 구현 #158

ukkodeveloper · 2023-07-30T12:39:23Z

📝작업 내용

노래 조회 페이지 구현

작업내용

webpack public url 추가
useCopyClipBoard 훅 분리
ToggleSwitch 컴포넌트 구현
ToggleGroup 컴포넌트 구현
노래 조회 페이지 구현

리뷰 참고사항

사용자 사용성 측면
디자인 측면
youtube component에 대해서도 이야기하면 좋을 것 같아요.

#️⃣연관된 이슈

연관된 이슈 번호를 모두 작성

close #140
close #152

github-actions · 2023-07-30T12:40:39Z

Unit Test Results

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

Results for commit bb8e545.

♻️ This comment has been updated with latest results.

cruelladevil

고생하셨습니다👍👍
킬링파트 감상하기 좋은 페이지네요😊

아직 쿼리 작성은 되지 않은 것인가요?
좀 더 마무리 짓고 approve하면 될 것 같아요 :)

cruelladevil · 2023-07-31T01:57:06Z

frontend/src/components/@common/ToggleGroup/ToggleGroup.style.tsx

+  display: flex;
+  justify-content: center;
+  width: 100%;
+  gap: 20px;


💬 common 컴포넌트인데 gap을 미리 지정해준 이유가 있으신가요?

외부에서 Spacing으로 스타일링하는 게 좋겠네요

cruelladevil · 2023-07-31T02:18:44Z

frontend/src/components/@common/ToggleGroup/ToggleGroup.tsx

+  const toggleContext = useContext(ToggleContext);
+
+  if (!toggleContext) {
+    throw new Error('toggle group 사용 시에 반드시 Context API를 공유해야합니다.');
+  }
+
+  const { selected, setSelected, onChangeValue } = toggleContext;


💬 ToggleButton 내에서 하는 일이라기보다 ToggleContext의 역할에 더 가까운 것 같아요.

custom hook으로 만들어서 Button에서 분리해내면 좋을 것 같아요.

const useToggleContext = () => { const toggleContext = useContext(ToggleContext); if (!toggleContext) { throw new Error('toggle group 사용 시에 반드시 Context API를 공유해야합니다.'); } const { selected, setSelected, onChangeValue } = toggleContext; return { selected, setSelected, onChangeValue }; }; const ToggleButton = ({ value, children }: ToggleButtonProps) => { const { selected, setSelected, onChangeValue } = useToggleContext(); ... };

cruelladevil · 2023-07-31T02:23:30Z

frontend/src/components/@common/ToggleSwitch/ToggleSwitch.tsx

+interface ToggleSwitchProps {
+  off: () => void;
+  on: () => void;
+  defaultToggle?: boolean;


💬 개인적인 생각으로는 기본값이니 defaultValue로 해도 괜찮다고 생각하는데
몇 번 곱씹어보니 boolean이니 defaultToggle로 조금 더 시맨틱하게 네이밍해도 좋다고 생각이 들긴 하네요

감샤합니다.

cruelladevil · 2023-07-31T02:26:07Z

frontend/src/components/@common/ToggleSwitch/ToggleSwitch.tsx

+  const [toggle, setToggle] = useState(defaultToggle);
+
+  const onToggle = () => {
+    toggle ? off() : on();


삼항연산으로 함수를 실행하는 것은 가독성에 안좋다고 생각합니다.

https://ko.javascript.info/ifelse#ref-1209

간단한 로직은 삼항연산자 사용하는 게 오히려 더 간결하다는 느낌인데 저랑 의견이 다르시군요! 그래도 팀원이 보기 불편하다면 변경할 용기를 갖고 있습니다. 변경완

cruelladevil · 2023-07-31T02:28:32Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

+  const getPlayingTime = (startSec: number, endSec: number) => {
+    const timeList = [secondsToMinSec(startSec), secondsToMinSec(endSec)].flat().map((timeSec) => {
+      const timeString = timeSec.toString();
+      return timeString.length === 1 ? `0${timeString}` : timeString;
+    });
+    return `${timeList[0]}:${timeList[1]} ~ ${timeList[2]}:${timeList[3]}`;
+  };


💬 유틸함수로 빠져도 좋을 것 같아요

동의합니다.

cruelladevil · 2023-07-31T02:29:48Z

frontend/src/pages/SongPage/SongPage.tsx

+          <ToggleGroup.button value="1">{'1st'}</ToggleGroup.button>
+          <ToggleGroup.button value="2">{'2nd'}</ToggleGroup.button>
+          <ToggleGroup.button value="3">{'3rd'}</ToggleGroup.button>
+          <ToggleGroup.button value="4">{'전체'}</ToggleGroup.button>


Suggested change

<ToggleGroup.button value="1">{'1st'}</ToggleGroup.button>

<ToggleGroup.button value="2">{'2nd'}</ToggleGroup.button>

<ToggleGroup.button value="3">{'3rd'}</ToggleGroup.button>

<ToggleGroup.button value="4">{'전체'}</ToggleGroup.button>

<ToggleGroup.button value="1">1st</ToggleGroup.button>

<ToggleGroup.button value="2">2nd</ToggleGroup.button>

<ToggleGroup.button value="3">3rd</ToggleGroup.button>

<ToggleGroup.button value="4">전체</ToggleGroup.button>

좋습니다~

cruelladevil · 2023-07-31T02:31:10Z

frontend/src/pages/SongPage/SongPage.tsx

+  const onChangeValue = (value: string) => {
+    setKillingRank(value);
+  };


💬 onChangeKillingRank또는 changeKillingRank 라고 하면 좋을 것 같아요

어렵지 않죠! 좋은 네이밍있다면 언제든 말씀해주세요

cruelladevil · 2023-07-31T02:35:28Z

frontend/src/pages/SongPage/SongPage.style.tsx

+  display: flex;
+  align-items: center;
+  gap: 12px;
+`;


이 파일에 개행이 안되어있는 곳이 여럿 있는 것 같아요 😊

cruelladevil · 2023-07-31T02:52:41Z

frontend/src/pages/SongPage/SongPage.tsx

+  const { id } = useParams();
+  const [player, setPlayer] = useState<YT.Player | undefined>();
+  const [isRepeat, setIsRepeat] = useState(true);
+  const [killingRank, setKillingRank] = useState('');


💬 꼭 문자열로 관리해야 되는 이유가 있나요? 숫자로 형변환을 자주 시켜주는 것 같은데 숫자형으로 하여도 괜찮을 것 같습니다.

가장 큰 문제는 ToggleGroup에서 지정한 타입 때문인 것 같은데요. 유니온이나 제네릭을 활용하면 해결할 수 있을 것 같아요.

제네릭 컴포넌트
https://medium.com/edonec/creating-a-generic-component-with-react-typescript-2c17f8c4386e
https://ui.toast.com/posts/ko_20210505

제네릭 컴포넌트로 구현하려고 했는데, 생각보다 어렵더라고요. contextAPi, toggle group하위에 있는 버튼 컴포넌트도 제네릭으로 모두 같은 타입으로 일괄되게 검사하게끔하는 것에서 막혔습니다. 그리고 형변환하여 접근하는 문제는 다른 방법으로 문제를 해결해보도록 하겠습니다.

Creative-Lee

고생 많으셨습니다~!
오류난 부분도 찾아주셔서 감사해요 ㅎㅎ
코멘트 확인 해주시고 다시 요청주세요!

Creative-Lee · 2023-07-31T02:37:26Z

frontend/src/types/song.ts

@@ -7,7 +7,7 @@ export interface SongDetail {
  title: string;
  singer: string;
  videoLength: number;
-  songVideoUrl: VideoUrl;
+  videoUrl: VideoUrl;


이 부분 backend 파트하고 이야기 해봐야겠네요
마지막 기억은 songVideoUrl 로 기억합니다

헉! 알겠습니다.

Creative-Lee · 2023-07-31T08:09:03Z

frontend/src/components/@common/ToggleGroup/ToggleGroup.tsx

+
+const ToggleContext = createContext<ToggleContextValue | null>(null);
+
+const ToggleGroup = ({ defaultValue, onChangeValue, children }: ToggleGroupProps) => {


💬 onChangeValue는 사용처에서 넣어주는 것 같은데, 어떤 의도인가요?

ToggleGroup에서 값이 선택될 때, 사용처에서 선택된 값을 바탕으로 추가 로직을 수행할 수 있도록 열어둔 것입니다.

Creative-Lee · 2023-07-31T08:18:16Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

+      <Container>
+        <TimeWrapper>
+          <Img src={shook} alt="logo" />
+          <p>{getPlayingTime(start, end)}</p>


styled 컴포넌트로 이름이 지어지면 좋을 것 같아요.

컨벤션 정해보아요

Creative-Lee · 2023-07-31T08:19:14Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

+          </VoteBox>
+
+          <ShareBox onClick={shareUrl}>
+            <Img src={link} alt="?" />


💬 alt가 ? 인 이유가 있나요?

없어요 변경했습니다.

Creative-Lee · 2023-07-31T08:23:23Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

+    <Wrapper>
+      <Container>
+        <TimeWrapper>
+          <Img src={shook} alt="logo" />


단순 꾸밈 요소로 쓰인 이미지 태그의 alt 속성은 빈값으로 두는게 좋을 것 같아요.
접근성 측면에서 더 좋을 것 같습니다!

그렇네요! 완전 납득이입니다.

Creative-Lee · 2023-07-31T08:23:52Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

+
+        <RestWrapper>
+          <VoteBox>
+            <Img src={people} alt="vote" />


이 alt 태그도 꾸밈요소로 사용된것 같네요!

굳입니다.

Creative-Lee · 2023-07-31T08:37:07Z

frontend/src/hooks/useCopyClipBoard.ts

💬 클립보드 복사는 항상 토스트로 알려준다 라는 가정하에 만든 hook일까요~?

💬 유틸성향의 함수에 인자의 유무에 따라 토스트를 띄워주는 기능만 추가된걸로 보이는데요~!

hook의 의미를 생각해보면 아래 함수는 유틸함수로 분리되고,
사용처에서 클립보드유틸과 토스트훅을 사용하는 핸들러 함수를 만들어 사용하면 될 것 같다고 생각되어요.

네 맞아요.

좋은 생가입니다.

Creative-Lee · 2023-07-31T08:38:41Z

frontend/webpack.common.js

@@ -5,6 +5,7 @@ module.exports = {
  entry: './src/index.tsx',
  output: {
    assetModuleFilename: 'assets/[hash][ext]',
+    publicPath: '/',


이 설정 때문이였군요! 감사합니다!

react-router의 자식의 자식을 찾지 못하는 오류 개선내용
ex) s-hook.com/child-of/child/do-not/work

Creative-Lee · 2023-07-31T08:41:35Z

frontend/src/pages/SongPage/SongPage.tsx

+    if (isRepeat) {
+      timer.current = window.setInterval(
+        () => {
+          player?.seekTo(part.start, true);
+        },
+        (part.end - part.start) * 1000
+      );
+    }


반복듣기를 위한 로직인것 같아요.
💬 비슷한 로직이 songDetailPage에도 존재하는데 이부분 추후에 통합이 이뤄지면 좋을 것 같아요.

저두 그렇게 생각해요! 사실 이전에 만든 페이지와 성격이 비슷해서, 중복되는 로직이나 컴포넌트가 많은데, 이를 함부러 건드렸다가 충돌 날 것 같아서 과감하게 분리하지 못했네요!

Creative-Lee · 2023-07-31T08:59:18Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

+  const getPlayingTime = (startSec: number, endSec: number) => {
+    const timeList = [secondsToMinSec(startSec), secondsToMinSec(endSec)].flat().map((timeSec) => {
+      const timeString = timeSec.toString();
+      return timeString.length === 1 ? `0${timeString}` : timeString;
+    });
+    return `${timeList[0]}:${timeList[1]} ~ ${timeList[2]}:${timeList[3]}`;
+  };


변수로 표현해 줄거라면 구조분해 할당을 이용하면 어떨까요?
return 문 라인에서도 각 인덱스 값을 생각하지 않아도 될 것 같아요.

Suggested change

const getPlayingTime = (startSec: number, endSec: number) => {

const timeList = [secondsToMinSec(startSec), secondsToMinSec(endSec)].flat().map((timeSec) => {

const timeString = timeSec.toString();

return timeString.length === 1 ? `0${timeString}` : timeString;

});

return `${timeList[0]}:${timeList[1]} ~ ${timeList[2]}:${timeList[3]}`;

};

const getPlayingTime = (startSec: number, endSec: number) => {

const [startMinute, startSecond, endMinute, endSecond] = [secondsToMinSec(startSec), secondsToMinSec(endSec)].flat().map((timeSec) => {

const timeString = timeSec.toString();

return timeString.length === 1 ? `0${timeString}` : timeString;

});

return `${startMinute}:${startSecond} ~ ${endMinute}:${endSecond}`;

};

너무 좋네요

Creative-Lee

리뷰 반영까지 모두 확인했습니다! 고생 많으셨습니다~!!

Creative-Lee · 2023-08-01T19:06:21Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx


  if (!killingPart) return;

  const { voteCount, start, end, partVideoUrl } = killingPart;

  const shareUrl = () => {
-    copyClipboard(partVideoUrl, '클립보드에 영상링크가 복사되었습니다.');
+    copyClipboard(partVideoUrl);


너무 좋습니다!

cruelladevil

Approve 하겠습니다

cruelladevil · 2023-08-02T04:28:42Z

frontend/src/components/@common/ToggleGroup/hooks/useToggleContext.ts

+
+export const ToggleContext = createContext<ToggleContextValue | null>(null);
+
+const useToggleContext = () => {


cruelladevil · 2023-08-02T04:30:28Z

frontend/src/components/KillingPartInfo/KillingPartInfo.tsx

-    });
-    return `${timeList[0]}:${timeList[1]} ~ ${timeList[2]}:${timeList[3]}`;
-  };
+  const playingTimeText = getPlayingTimeText(start, end);


ukkodeveloper added 20 commits July 28, 2023 21:32

feat: toggle switch button 컴포넌트 구현

b65c363

fix: 노래의 KillingPart 타입에서 유니온을 number타입으로 변경

e7bcd12

feat: songs fixture 데이터 api맞게 수정

c95aee1

config: 웹팩 publicPath 추가하여 root 경로에서 빌드파일 접근

c3ef671

refactor: api에 맞게 SongDetail 타입 key값 이름 변경

e658509

refactor: msw songsHandler 목데이터에 맞게 수정 및 status 추가

5a7f40e

Merge branch 'main' into feat/#140

3b1d117

feat: SongPage router 설정 및 영상 로드

3c3115b

feat: Spacing, Flex 공용 컴포넌트로 분리

c837aa2

feat: ToggleGroup 컴포넌트 구현

1ec4ddc

chore: 링크, 슉, 사람들 모양 svg 파일 추가

d13d0f5

feat: 클립보드 복사 훅 구현

1eaa3c5

feat: killingPartInfo 컴포넌트 구현

9822447

fix: api에 맞게 partVideoUrl로 수정

8ff3ebf

feat: KillingPart 타입에 export 추가 및 interface로 변경

05cdece

feat: SongPage 개괄적인 틀 잡기 및 정보들 렌더

44eae18

feat: 노래 조회 페이지 추가

379653f

feat: togglegroup 컴포넌트 스타일 분리

9197e76

feat: toggleSwitch 컴포넌트 스타일 분리

899460a

refactor: import문 수정

89fde74

ukkodeveloper added [ 🌞 FE ] ✨ Feat labels Jul 30, 2023

ukkodeveloper requested a review from Creative-Lee as a code owner July 30, 2023 12:39

ukkodeveloper self-assigned this Jul 30, 2023

ukkodeveloper requested a review from cruelladevil as a code owner July 30, 2023 12:39

cruelladevil requested changes Jul 31, 2023

View reviewed changes

Creative-Lee requested changes Jul 31, 2023

View reviewed changes

ukkodeveloper added 2 commits August 1, 2023 16:38

feat: ToggleGroup gap을 외부에서 받도록 변경

3f29995

feat: toggle group 사용처에서 사용 수정 및 핸들러 이름 변경

e4352f6

ukkodeveloper added 8 commits August 1, 2023 18:42

refactor: useToggleContext 훅 분리

fab75d9

refactor: 삼항연산자를 조건문으로 변경

b0bf8a5

refactor: 재생시간 텍스트 유틸함수로 분리

e28dde6

style: SongPage.style 개행

caa56bf

refactor: img alt 의미있게 변경

7f8ddad

refactor: copyClipBoard 유틸로 분리

997214c

refactor: songDetail 타입 변경

66261fb

refactor: toggleGroup value에서 number인 index로 변경

289b44d

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

Creative-Lee approved these changes Aug 1, 2023

View reviewed changes

cruelladevil approved these changes Aug 2, 2023

View reviewed changes

Merge remote-tracking branch 'origin' into feat/#140

bb8e545

ukkodeveloper merged commit 75dc7f3 into main Aug 2, 2023

ukkodeveloper deleted the feat/#140 branch August 2, 2023 13:02


		const ToggleContext = createContext<ToggleContextValue \| null>(null);

		const ToggleGroup = ({ defaultValue, onChangeValue, children }: ToggleGroupProps) => {


		export const ToggleContext = createContext<ToggleContextValue \| null>(null);

		const useToggleContext = () => {

Feat/#140 노래 조회 페이지 구현 #158

Feat/#140 노래 조회 페이지 구현 #158

Conversation

ukkodeveloper commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝작업 내용

리뷰 참고사항

#️⃣연관된 이슈

Uh oh!

github-actions bot commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

cruelladevil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Creative-Lee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ukkodeveloper commented Jul 30, 2023 •

edited

Loading

github-actions bot commented Jul 30, 2023 •

edited

Loading

cruelladevil Sep 1, 2023 •

edited

Loading