-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/#479 킬링파트 등록 시 구간 선택을 토글 형식으로 변경 #482
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 많으셨습니다 우코
우선 PR에 코멘트가 없어서 조금 당황했을 수도 있을것 같아요.
전체적인 변경사항에 대한 코멘트를 여기에 정리하는게 좋겠다 싶어서 본문으로 남깁니다.
기존 기능이 올바르게 동작하지 않는 부분
- 토글 해제 시 시작시간 0:00 초 고정 이슈
이 부분은 VideoSlider 쪽 로직에 null 분기 넣으면서 발생한 것 같아요.
2.mp4
코드 제안
interval의 null type이 추가되면서, 여러 파일에서 null 처리 분기 코드가 많아졌더라고요
가독성과 추후 확장에 용이하지 않다고 생각했어요.
이 부분에서 NONE : 0 상수 추가와 함께 null 대신 0을 사용하는 것을 제안합니다.
별도의 브랜치에서 테스트 해본 결과
기존 코드의 수정없이 아래 2개의 코드만 추가하면 되었습니다.
또한 위에서 언급한 시작 시간 0:00 초 고정 이슈
도 해결됩니다.
const updateKillingPartInterval: React.MouseEventHandler<HTMLButtonElement> = (e) => {
const newInterval = Number(e.currentTarget.dataset['interval']) as KillingPartInterval;
if (newInterval === interval) { // 우코가 추가한 코드 - 버튼 한번 더 클릭시 비 선택 처리
setInterval(0);
return;
}
{...}
}
useEffect(() => {
if (interval === 0) return; // 우코가 추가한 코드 - 비 선택이면 노래 일반 재생(반복x)
const timer = window.setInterval(() => {
videoPlayer.current?.seekTo(partStartTime, true);
}, interval * 1000);
return () => window.clearInterval(timer);
}, [videoPlayer.current, partStartTime, interval]);
정리 하자면 기존 코드를 그대로 유지한 상태에서
- 비선택 시 사용할
0
상수 추가 및 상태값으로 사용(type 추가) - 중복 선택시 상태 0(비선택)으로 변경
- 플레이어 반복 재생 effect return 처리
입니다.
정책 제안
현재로써는 토글을 해제 해야만 전체 듣기 및 반복 듣기 해제
가 가능한 상태에요
기본값으로 토글을 해제해 놓으면서 선택을 하도록 유도하는걸 어떨까요?
대신 이경우 버튼인지 인식하지 못하는 이슈가 있을 수 있어서,
인터페이스의 각 부분이 뭘 의미하는지 소제목으로 설명해주면 좋을 것 같아요.
ex 소제목)
- interval 버튼들 -> 킬링파트 길이 or 구간 길이
- 슬라이더 -> 구간 선택
확인 해주시고 코멘트 및 재요청 주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 interval을 null 말고 0을 하면 어떨까싶어요!
작업하시는 김에 상수화 안된 부분도 변경해주시면 좋을 것 같아요!
그 외 사항은 코멘트로 남겼습니당! 고생하셨습니다👍
@@ -90,15 +96,16 @@ const RegisterTitle = styled.p` | |||
`; | |||
|
|||
const Register = styled.button` | |||
cursor: pointer; | |||
cursor: ${({ disabled }) => (disabled ? 'not-allowed' : 'pointer')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globalStyle로 가면 좋을 것 같네요!
button {
cursor: pointer;
background: none;
border: 0;
&:disabled {
cursor: not-allowed;
}
}
@@ -62,6 +67,7 @@ export const VoteInterfaceProvider = ({ | |||
}; | |||
|
|||
useEffect(() => { | |||
if (!interval) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 개행좀 부탁드려도 될까요?😭
const submitKillingPart = async () => { | ||
if (!interval) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 개행좀 부탁드려도 될까요?😭 22
@@ -25,19 +25,25 @@ export const VoteInterfaceProvider = ({ | |||
songId, | |||
songVideoId, | |||
}: PropsWithChildren<VoteInterfaceProviderProps>) => { | |||
const [interval, setInterval] = useState<KillingPartInterval>(10); | |||
const [interval, setInterval] = useState<KillingPartInterval | null>(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KILLING_PART_INTERVAL.TEN
작업하시는 김에 상수화 수정해주시면 감사합니당🙇♂️
const partEndTime = partStartTime + interval; | ||
const partStartTimeText = toMinSecText(partStartTime); | ||
const partEndTimeText = toMinSecText(partEndTime); | ||
const partStartTimeText = interval ? toMinSecText(partStartTime) : toMinSecText(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝작업 내용
킬링파트 등록 시 구간 선택을 토글 형식으로 변경
참고사항
(도밥의 조언으로 참고사항을 더 자세하게 적습니다! 감사해요 도밥)
#️⃣연관된 이슈
close #479