Feat/#499 등록 ui 개선 및 등록 페이지 리팩터링 #508
Conversation
| const submitKillingPart = async () => { | ||
| video.pause(); | ||
| await createKillingPart(songId, { startSecond: partStartTime, length: interval }); | ||
| openModal(); | ||
| navigate(-1); |
There was a problem hiding this comment.
💬 -1 은 뒤로가기의 의미이니 '듣기 페이지로 이동' 이라는 의미를 조금 더 확실히 해주면 어떨까요?
💬 추가로 -1은 어떤 페이지에서 접속하든 이전 히스토리로 이동하기 때문에, 이에대한 이슈도 있을 것 같아요.
songId도 사용할 수 있으니 명시해도 좋을 것 같아요.
There was a problem hiding this comment.
장르는 ALL 때릴생각을 하고 이야기했었는데, 우코의 생각이 더 좋은것 같아요! ㅎㅎ
대화를 통해 해결했습니다~
There was a problem hiding this comment.
[대화 정리]
- 특정 장르에서 노래를 듣다가 내 파트 등록 페이지로 갑니다.
- 등록 완료 후에는 이전에 사용하던 맥락으로 돌아가야하기 때문에 history 기록을 pop합니다.
| return ( | ||
| <> | ||
| <RegisterButton type="submit" onClick={submitKillingPart}> | ||
| <RegisterButton type="submit" onClick={openModal}> |
There was a problem hiding this comment.
이제 이 버튼을 누르면 비디오 퍼즈 + 모달이 열려야 자연스러울듯 해요.
현재는 -1로 이동하기전 노래를 끄는 함수가 실행됩니당
| const SoundWave = ({ length }: SoundWaveProps) => { | ||
| return Array.from({ length }, (_, index) => { | ||
| return ( | ||
| // eslint-disable-next-line react/display-name |
There was a problem hiding this comment.
💬 SoundWave.displayName =SoundWave
추가하면 warning이 사라지지 않던가용?
There was a problem hiding this comment.
SoundWave.displayName = 'SoundWave'; 적용완료
| const stretchWaveHeight = | ||
| (activeHeight: string, inactiveHeight: string) => (dom: HTMLDivElement | null) => { | ||
| if (!dom || !boxRef || typeof boxRef === 'function') return; | ||
| if (boxRef.current?.scrollLeft) { |
There was a problem hiding this comment.
💬 제약되는 상황이 아니라면 얼리리턴으로 댑스를 줄여도 좋겠네요!
| onWheel={wheelStartTime} | ||
| onTouchStart={playVideo} | ||
| $progressWidth={progressWidth} | ||
| ref={waveScrubberRef} |
There was a problem hiding this comment.
💬 ref는 중요한 정보라고 생각해서 앞으로 빼는 편이에요
맨앞이 아니더라도 event ,스타일 속성, 기타 속성들을 한곳에 모아두는게 더 읽기 편할 것 같아요.
지금은 event 사이에 스타일 속성 ref가 있어서 정돈된 느낌이 덜한것 같네요
There was a problem hiding this comment.
props가 많아서 정리해서 보여주니 훨씬 읽기 좋아진 것 같아요
| const deletePin = () => { | ||
| if (activePinIndex >= 0) { | ||
| setPinList(pinList.filter((_, index) => index !== activePinIndex)); | ||
| } else { | ||
| setPinList(pinList.slice(1)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
💬 나중에 set함수 분리 해주세요~
There was a problem hiding this comment.
추후에 어떻게 분리해야할 지 고민해봐야겠네요. 훅 정리하면서 시멘틱하게 이름 지어야할 것 같아요. 현재 usePin에 선언되어있는 함수가 많아서 고민입니다. 리팩터링할 때 적용해볼게요
ukkodeveloper
force-pushed
the
feat/#499
branch
from
c3d8dbb to
f2baad4
Compare
📝작업 내용
작업 내용이 좀 많습니다. 기존 제 PR에서 기본적인 것들을 지키지 않은 것 같아서, 이번에 좀 하나씩 확인해보면서 적용해봤습니다.
작업 내용은 크게 두 가지입니다.
WaveScrubber
구간의 길이, 구간 시작점, 전체 듣기 등 모든 제어에 다시 시작점으로 이동합니다.
페이지 리팩터링
이 작업을 시작한 계기는 이번에 구현을 하면서 스타일링을 변경하는 게 쉽지 않았었는데요.
페이지 코드 단에서 구조가 한 눈에 보이지 않더라고요.
아래는 등록 페이지 JSX 코드인데요. 파란색과 보라색 컴포넌트는 서로 동일한 계층에 존재합니다.
하지만 실제 렌더링된 UI를 보면 그렇지 않죠.
즉, 계층별로 컴포넌트의 추상화 레벨이 맞지 않기 때문에 머릿 속에 구조가 잘 들어오지도 않습니다.
이에 더해서 VoteInterface는 여러 UI가 복합적으로 뭉쳐져있기 때문에 외우고 있지 않는 한 어떤 ui가 담겨져 있는 지 알 수 없습니다. 해당 컴포넌트 파일을 봐야만 어떤 ui가 있는 지 알 수 있는 것입니다.
JSX 구조를 보여준다는 점뿐만 아니라 실제로 CSS flex를 통해 반응형을 구현하는 과정에서 flex를 새로 감싸는 작업에서도 힘든 상황이 있었습니다. 각 컴포넌트 구분이 명확하지 않다보니, 내가 추가한 flex 속성이 어느 ui에 영향을 미치는 건지 확인하는 것도 쉽지 않았습니다. 매번 질서가 잡히지 않은 중첩된 컴포넌트를 왔다갔다 하면서 CSS 속성을 확인해야 했습니다.
개선 후에는 JSX코드가 이렇게 됩니다.
계층을 잘 나누려고 노력을 했는데요. 이렇게 구조를 변경하니 페이지가 어떻게 구성되어 있는 지 쉽게 예측할 수 있을 뿐만 아니라 관심사 분리도 어부지리로 되었다는 느낌도 받았습니다. 만약 youtube 조작 ui를 변경하고 싶다면 해당 코드만 보고도 바로 VideoController에서 변경해야겠다는 판단이 들 것입니다.
이 작업을 통해 "실(loss)"이 존재합니다. context provider을 필요한 부분에만 적용하지 않고 전체를 감쌌다는 것인데요. 저는 실보다는 득이 훨씬 크다고 생각해서 변경했습니다. 다른 분들의 의견도 궁금하네요.
이것 외에는 commit 참고 부탁드립니당.
💬리뷰 참고사항
같이 고민했으면 좋겠는 부분
Context를 사용해서 모든 하위 컴포넌트에서 context를 사용해서 상태관리를 하고 있습니다. 그러다보니 비디오를 조작하는 모든 ui가 재사용이 불가능한데요. 일단, 재사용할 일이 없을 것 같아서 context를 주입했는데, 이 부분에 대해서 props로 받는 게 나을 지 여러분의 의견이 궁금해요.
이름 변경
Vote 들어간 컴포넌트나 훅의 이름을 Collect로 변경했어요. 현재 기획 변경으로 "수집"이라는 단어도 사용하지 않지만, "collect"도 내 파트 등록을 어느정도 포괄할 수 있는 단어이기도 하고, 현재 페이지 경로 세그먼트도 'collect'로 되어 있어서 통일했습니다. ("vote"는 의미가 많이 다른 것 같아서요.)
기타
리팩터링에서 api 호출 관련해서는 굳이 개선하지 않았습니다. axios 도입되면 그때 추가로 개선해야할 것 같습니다.
🔴 QA 테스트 환경 🔴
PartCollectingPart페이지를 감싸고 있는<AuthLayout>를 제거한다.스토리북을 사용할까 생각도 해봤는데, 어차피 페이지 단위 렌더링이 필요한 작업 + 이미 다른 PR에서 도밥이 작업한 것들을 중복으로 할...
대충 여러모로 시간이 꽤나 걸릴 것 같아서 스토리북은 조금 생략했습니다. 😢 이번만 양해 부탁드릴게요
#️⃣연관된 이슈
close #499