Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor/#244 기존 SongDetailPage 컴포넌트에 새로운 플로우와 시안을 적용한다. #261

Merged
merged 49 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
9dae4e4
refactor: Youtube player 초기화관련 상태 제거 및 ref로 대체
Creative-Lee Aug 11, 2023
4d269dc
design: 버튼 길이 수정
Creative-Lee Aug 11, 2023
d008412
refactor: msw fixture 데이터 수정
Creative-Lee Aug 11, 2023
797e175
refactor: 썸네일 컴포넌트 사이즈 props 추가
Creative-Lee Aug 11, 2023
afa7c92
chore: 중첩 label 수정
Creative-Lee Aug 11, 2023
a40efc6
design: 반복 버튼 위치 변경
Creative-Lee Aug 11, 2023
c968fbd
chore: 중첩 label 추가
Creative-Lee Aug 11, 2023
99f3403
refactor: 기존 킬링파트 재생기능 적용
Creative-Lee Aug 11, 2023
8896774
test: props 변경으로 인한 스토리 수정
Creative-Lee Aug 11, 2023
8b96c7d
refactor: msw fixture 데이터 타입에 맞게 수정
Creative-Lee Aug 11, 2023
a2fb71e
refactor: 비디오 플레이 관련 함수 프로바이더로 분리
Creative-Lee Aug 11, 2023
477faa6
refactor: 반복재생 기능 비활성화
Creative-Lee Aug 11, 2023
378c172
refactor: SongDetailPage의 책임분리
Creative-Lee Aug 12, 2023
b1c4868
refactor: KillingPartInterface로 부터 전달되는 props 변경
Creative-Lee Aug 12, 2023
e2706fe
refactor: 플레이어 관련 변수 분리 및 함수 시그니처 변경
Creative-Lee Aug 12, 2023
9aa0718
refactor: 불필요한 컨텍스트 제거
Creative-Lee Aug 12, 2023
e5b9c39
test: props 및 state 타입 변경으로 인한 스토리 수정
Creative-Lee Aug 12, 2023
ad186bc
refactor: 코멘트 컴포넌트 props타입 변경
Creative-Lee Aug 12, 2023
58f05df
feat: Timer 컴포넌트 구현
Creative-Lee Aug 12, 2023
f7f67cb
refactor: Timer 컴포넌트 Context+provider 화
Creative-Lee Aug 12, 2023
7475e9c
chore: provider화 된 Timer 컴포넌트 스토리 삭제
Creative-Lee Aug 12, 2023
91717d6
refactor: 플레이어 상태 별 변수 분리 및 토글 함수 추가
Creative-Lee Aug 12, 2023
8d9f812
refactor: 변수 의미 개선 및 props 변경
Creative-Lee Aug 12, 2023
361cb1e
test: props 변경사항 반영
Creative-Lee Aug 12, 2023
9fd9485
refactor(TimerProvider): 함수명 개선 및 얼리 리턴 적용
Creative-Lee Aug 12, 2023
476d38a
chore: 일시정지 아이콘 추가
Creative-Lee Aug 13, 2023
4886a6f
refactor: 유튜브 컴포넌트 개선
Creative-Lee Aug 13, 2023
dee7353
refactor: 트랙 컴포넌트 정책 변경으로 인한 관련 프로바이더 개선
Creative-Lee Aug 13, 2023
af60a62
feat: 유튜브 플레이어 조작 시 포커스 아웃 이펙트 구현
Creative-Lee Aug 13, 2023
5fad8e4
refactor: 불필요한 변수 삭제
Creative-Lee Aug 13, 2023
007f133
refactor: 유튜브 컴포넌트 초기 플레이어 변수 추가
Creative-Lee Aug 13, 2023
0657970
feat: 접근성 관련 속성 추가
Creative-Lee Aug 13, 2023
ada932f
refactor: page컴포넌트에 존재하던 반복재생 effect 위치 변경 및 개선
Creative-Lee Aug 14, 2023
9aac9fb
refactor: 플레이어 프로바이더의 함수에 callback 적용
Creative-Lee Aug 14, 2023
4ced5f8
design: 트랙 개별 트랜지션 적용
Creative-Lee Aug 14, 2023
7dfcf04
test: 트랙 컴포넌트 스토리에 프로바이더 추가
Creative-Lee Aug 14, 2023
e8a3e38
test: 킬링파트 트랙 스토리 수정
Creative-Lee Aug 15, 2023
5d60a20
refactor: enum 타입 적용
Creative-Lee Aug 15, 2023
2488d6b
refactor: 썸네일 사이즈 상수 통합
Creative-Lee Aug 15, 2023
ee88e78
refactor: 플레이어 이벤트 관리 통일 및 타입 변경
Creative-Lee Aug 15, 2023
307d4c1
fix: 아이콘 함수 분기문의 잘못된 enum 타입 수정
Creative-Lee Aug 15, 2023
7c5960e
refactor: 불필요한 함수 래핑 분리
Creative-Lee Aug 15, 2023
98be716
fix: 버퍼링 시 플레이어 멈추는 상황 해결
Creative-Lee Aug 15, 2023
9d1c617
refactor: 댓글 상태 추가 및 사용성 개선
Creative-Lee Aug 15, 2023
c3d016e
refactor: 구간반복, 타이머 기능 개선
Creative-Lee Aug 16, 2023
84caa6c
refactor: 킬링파트 트랙은 타이머에 관여하지 않도록 개선
Creative-Lee Aug 16, 2023
145c1d1
test: 스토리북 내 미사용 변수 린트 예외처리
Creative-Lee Aug 16, 2023
a09260b
Merge branch 'main' of https://github.com/woowacourse-teams/2023-shoo…
Creative-Lee Aug 17, 2023
c3d437f
fix: main 머지 후 충돌 해결
Creative-Lee Aug 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions frontend/src/assets/icon/pause.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 4 additions & 8 deletions frontend/src/features/comments/components/CommentList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@ interface Comment {
}

interface CommentListProps {
songId: string;
songId: number;
partId: number;
}

const CommentList = ({ songId, partId }: CommentListProps) => {
const { showToast } = useToastContext();
const [newComment, setNewComment] = useState('');

const { data: comments, fetchData: getComment } = useFetch<Comment[]>(() =>
Expand All @@ -30,7 +31,6 @@ const CommentList = ({ songId, partId }: CommentListProps) => {
const { mutateData } = useMutation(() =>
fetcher(`/songs/${songId}/parts/${partId}/comments`, 'POST', { content: newComment.trim() })
);
const { showToast } = useToastContext();

const resetNewComment = () => setNewComment('');

Expand All @@ -52,15 +52,11 @@ const CommentList = ({ songId, partId }: CommentListProps) => {
getComment();
}, [partId]);

if (!comments) {
return null;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

refactor: 코멘트 컴포넌트 props타입 변경

💬 커밋 메세지로 확인 안되어서 그런데 이건 왜 고치신건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

커밋을 잘 나누지 못했네요...
만약 comments 배열 자체가 없거나, comments 배열은 있는데 코멘트가 없어도
랜더링은 진행되어서 댓글 0개는 보여주는게 좋겠다 생각했어요!

return (
<>
<Spacing direction="vertical" size={24} />
<SRHeading as="h3">댓글 목록</SRHeading>
<p>댓글 {comments.length}개</p>
<p>댓글 {comments?.length ?? 0}개</p>
<Spacing direction="vertical" size={24} />
<form onSubmit={submitNewComment}>
<Flex>
Expand All @@ -86,7 +82,7 @@ const CommentList = ({ songId, partId }: CommentListProps) => {
</form>
<Spacing direction="vertical" size={20} />
<Comments>
{comments.map(({ id, content, createdAt }) => (
{comments?.map(({ id, content, createdAt }) => (
<Comment key={id} content={content} createdAt={createdAt} />
))}
</Comments>
Expand Down
122 changes: 122 additions & 0 deletions frontend/src/features/songs/components/KillingPartInterface.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { useEffect, useRef, useState } from 'react';
import { styled } from 'styled-components';
import CommentList from '@/features/comments/components/CommentList';
import useVideoPlayerContext from '@/features/youtube/hooks/useVideoPlayerContext';
import Spacing from '@/shared/components/Spacing';
import ToggleSwitch from '@/shared/components/ToggleSwitch/ToggleSwitch';
import KillingPartTrackList from './KillingPartTrackList';
import type { KillingPart, SongDetail } from '@/shared/types/song';

interface KillingPartInterfaceProps {
killingParts: SongDetail['killingParts'];
songId: number;
}

const DEFAULT_PART_ID = -1;

const KillingPartInterface = ({ killingParts, songId }: KillingPartInterfaceProps) => {
const [nowPlayingTrack, setNowPlayingTrack] = useState<KillingPart['id']>(DEFAULT_PART_ID);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이제 변동 가능한 rank 대신 고정값인 id로 관리합니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 rank는 왜 변동가능하고 id는 왜 고정인가요? 둘 다 똑같은 KillingPart의 정보 아닌가요?
rank로 관리하나 id로 관리하나 무슨 차이인지 잘 모르겠어요

역으로 생각하면 서버에서 내려주는 killingParts에는 rank가 순서대로 고정적으로 담겨있고,
나머지 정보가 변동된다고 해석할 수도 있을 것 같은데 뭐가 고정이고 뭐가 변동이라는 말인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제 설명이 다소 애매하고 부족했네요.
생각했던 부분을 전부 나열하는게 오히려 좋을것 같아서 아래에 다 적어봅니다!

{killingPart && <CommentList songId={id} partId={killingParts[killingRank! - 1].id} />}

리팩터링 전에는 page컴포넌트가 KillingRank 상태를 가지고 있었고, rank로 killingPart를 찾고,
이걸 코멘트 리스트에 id를 넘기는데 쓰이고 있었어요.
이 부분에서 기존에 있던 KillingRank state대신, nowPlayingTrack state에 담는 값을 id로 바꾸면서
하나로 통일하면 좋겠다고 생각했어요~

또 이 과정에서 "part의 id는 순위가 변동된다고 변하지 않고, rank는 순위에 변동에 따라 바뀌니깐
rank가 어떤걸 정하는 내부적 기준이 되면 언젠가 의미적으로 햇갈릴 수 있겠다" 막연하게 생각하기도 했고요!

이제 변동 가능한 rank 대신 고정값인 id로 관리합니다!

이 문장 하나로는 택도 없었네요 ㅠ

역으로 생각하면 서버에서 내려주는 killingParts에는 rank가 순서대로 고정적으로 담겨있고,
나머지 정보가 변동된다고 해석할 수도 있을 것 같은데 뭐가 고정이고 뭐가 변동이라는 말인가요?

코난의 의문이 확실히 이해됩니다!
혹시 위 이유들을 읽어보시고 여전히 의문이 남으면 직접 이야기 해주세요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

리팩터링 전에는 page컴포넌트가 KillingRank 상태를 가지고 있었고, rank로 killingPart를 찾고,
이걸 코멘트 리스트에 id를 넘기는데 쓰이고 있었어요.
이 부분에서 기존에 있던 KillingRank state대신, nowPlayingTrack state에 담는 값을 id로 바꾸면서
하나로 통일하면 좋겠다고 생각했어요~

그렇군요 👍👍

const [isRepeat, setIsRepeat] = useState(false);
const { videoPlayer, playerState, seekTo, pause } = useVideoPlayerContext();

const timerRef = useRef<number | null>(null);

const toggleRepetition = () => {
setIsRepeat(!isRepeat);
};

useEffect(() => {
if (document.activeElement === videoPlayer.current?.getIframe()) {
setNowPlayingTrack(DEFAULT_PART_ID);
}
}, [videoPlayer, playerState]);

useEffect(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반복 재생 이펙트 이동 및 로직 개선

const part = killingParts.find((part) => part.id === nowPlayingTrack);
if (!part) return;

seekTo(part.start);

const interval = (part.end - part.start) * 1000;

if (isRepeat) {
timerRef.current = window.setInterval(() => seekTo(part.start), interval);
} else {
timerRef.current = window.setTimeout(() => {
pause();
setNowPlayingTrack(DEFAULT_PART_ID);
}, interval);
}

return () => {
if (!timerRef.current) return;
window.clearInterval(timerRef.current);
};
}, [isRepeat, nowPlayingTrack, seekTo, pause, killingParts]);

return (
<>
<FlexContainer>
<TitleWrapper aria-label="Top 3 킬링파트 듣기">
<ItalicTitle aria-hidden="true">Top 3</ItalicTitle>
<NormalTitle aria-hidden="true"> Killing part</NormalTitle>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<NormalTitle aria-hidden="true"> Killing part</NormalTitle>
<NormalTitle aria-hidden="true">Killing part</NormalTitle>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

다른 스타일의 텍스트 사이에 띄어쓰기가 필요해서 넣었습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하! 띄어쓰기로 간격을 주지말고 TitleWrapper를 flex로 변경하고 간격을 주면 어떨까요??

<TitleWrapper aria-label="Top 3 킬링파트 듣기">
  <ItalicTitle aria-hidden="true">Top 3</ItalicTitle>
  <Spacing direction="horizontal" size={10} />
  <NormalTitle aria-hidden="true">Killing part</NormalTitle>
</TitleWrapper>

</TitleWrapper>
<SwitchWrapper>
<SwitchLabel htmlFor="repetition">구간 반복</SwitchLabel>
<ToggleSwitch
id="repetition"
on={toggleRepetition}
off={toggleRepetition}
defaultToggle={isRepeat}
/>
</SwitchWrapper>
</FlexContainer>
<Spacing direction="vertical" size={16} />
<KillingPartTrackList
killingParts={killingParts}
nowPlayingTrack={nowPlayingTrack}
setNowPlayingTrack={setNowPlayingTrack}
/>
{nowPlayingTrack !== DEFAULT_PART_ID && (
<CommentList songId={songId} partId={nowPlayingTrack} />
)}
</>
);
};

export default KillingPartInterface;

const SwitchWrapper = styled.div`
display: flex;
column-gap: 8px;
`;

const SwitchLabel = styled.label`
font-size: 12px;
color: ${({ theme: { color } }) => color.white};
`;

const TitleWrapper = styled.div`
font-size: 22px;
font-weight: 700;
color: ${({ theme: { color } }) => color.white};

@media (max-width: ${({ theme }) => theme.breakPoints.md}) {
font-size: 18px;
}
`;

const ItalicTitle = styled.span`
font-style: italic;
color: ${({ theme: { color } }) => color.primary};
`;

const NormalTitle = styled.span`
color: ${({ theme: { color } }) => color.white};
`;

const FlexContainer = styled.div`
display: flex;
align-items: center;
justify-content: space-between;
`;
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useState } from 'react';
import { VideoPlayerProvider } from '@/features/youtube/components/VideoPlayerProvider';
import TimerProvider from '@/shared/components/Timer/TimerProvider';
import ToastProvider from '@/shared/components/Toast/ToastProvider';
import KillingPartTrack from './KillingPartTrack';
import type { KillingPart } from '@/shared/types/song';
Expand All @@ -13,7 +14,9 @@ const meta = {
return (
<ToastProvider>
<VideoPlayerProvider>
<Story />
<TimerProvider time={killingPart.end - killingPart.start}>
<Story />
</TimerProvider>
</VideoPlayerProvider>
</ToastProvider>
);
Expand All @@ -36,21 +39,15 @@ const killingPart: KillingPart = {
};

const KillingPartTrackWithHook = () => {
const [nowPlayingTrack, setNowPlayingTrack] = useState(-1);
const [nowPlayingTrack, setNowPlayingTrack] = useState<KillingPart['id']>(-1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

💬 ide에서는 똑같이 number로 읽더라고요. useState<KillingPart['id']>(-1);로 변경했을 때 어떤 점이 좋나요?!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저번에도 비슷한 질문을 해주신것 같은데요!
개발자의 타입 추론 목적으로 접근해 보는건 어떨까요??

해당 코드가 항상 killingPart의 id 만을 받아야 하는 상태인것을 아는 상황이라면 number로 두어도 이해가 되겠지만,
코드를 처음 보는 개발자 입장에서는 이해가 어려울 수도 있겠죠!
number는 조금 넓은 타입이니까요.

위와 같은 이유로 저는 객체 타입의 하위 타입? 이라면 조금더 좁혀서 명시해 주고 있습니다!
답변이 되었을까요?


const changePlayingTrack: React.ChangeEventHandler<HTMLInputElement> = ({ currentTarget }) => {
const newTrack = Number(currentTarget.value);

setNowPlayingTrack(newTrack);
};

const isPlaying = killingPart.rank === nowPlayingTrack;
const isNowPlayingTrack = killingPart.rank === nowPlayingTrack;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const isNowPlayingTrack = killingPart.rank === nowPlayingTrack;
const isNowPlayingTrack = killingPart.id === nowPlayingTrack;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이런일이! 수정하겠습니다!


return (
<KillingPartTrack
killingPart={killingPart}
isPlaying={isPlaying}
changePlayingTrack={changePlayingTrack}
isNowPlayingTrack={isNowPlayingTrack}
setNowPlayingTrack={setNowPlayingTrack}
/>
);
};
Expand Down
Loading