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

Feat/#479 킬링파트 등록 시 구간 선택을 토글 형식으로 변경 #482

Closed
wants to merge 5 commits into from

Conversation

ukkodeveloper
Copy link
Collaborator

@ukkodeveloper ukkodeveloper commented Sep 28, 2023

📝작업 내용

킬링파트 등록 시 구간 선택을 토글 형식으로 변경

  • interval input 컴포넌트 사용하지 않아서 삭제했습니다.
  • interval null 타입을 추가했습니다.
  • 아무것도 선택하지 않았을 때 구간반복을 하지 않도록 하였습니다.
  • 구간 변경 시에 구간 첫 시작점으로 이동하도록 하였습니다.

참고사항

(도밥의 조언으로 참고사항을 더 자세하게 적습니다! 감사해요 도밥)

  • 현재 작업사항은 킬링파트 구간 토글이 가능하는 것에 집중한 작업입니다.
  • 그 전에 쌓여있던 등록 페이지 개선에 대한 부분은 추후에 개선이 필요하다고 느꼈습니다.
    • 디자인이 변경될 수도 있고, 정책이 변경될 수도 있어서 적어도 논의가 이뤄질 필요가 있습니다.
  • 결론은 토글이 가능하게끔만 해놓은 것이고, 여전히 등록 플로우(사용성)은 좋지 않을 수 있음을 알려드립니다.

#️⃣연관된 이슈

close #479

@ukkodeveloper ukkodeveloper added [ 🌞 FE ] 프론트엔드 크루들의 빛나는 개발 이야기 하나둘셋 호! ✨ Feat 꼼꼼한 기능 구현 중요하죠 labels Sep 28, 2023
@ukkodeveloper ukkodeveloper self-assigned this Sep 28, 2023
@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Unit Test Results

  85 files    85 suites   13s ⏱️
321 tests 321 ✔️ 0 💤 0
324 runs  324 ✔️ 0 💤 0

Results for commit a1e960e.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Creative-Lee Creative-Lee left a 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을 사용하는 것을 제안합니다.

image

별도의 브랜치에서 테스트 해본 결과
기존 코드의 수정없이 아래 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]);

정리 하자면 기존 코드를 그대로 유지한 상태에서

  1. 비선택 시 사용할 0 상수 추가 및 상태값으로 사용(type 추가)
  2. 중복 선택시 상태 0(비선택)으로 변경
  3. 플레이어 반복 재생 effect return 처리

입니다.

정책 제안

현재로써는 토글을 해제 해야만 전체 듣기 및 반복 듣기 해제 가 가능한 상태에요
기본값으로 토글을 해제해 놓으면서 선택을 하도록 유도하는걸 어떨까요?

대신 이경우 버튼인지 인식하지 못하는 이슈가 있을 수 있어서,
인터페이스의 각 부분이 뭘 의미하는지 소제목으로 설명해주면 좋을 것 같아요.
image

ex 소제목)

  • interval 버튼들 -> 킬링파트 길이 or 구간 길이
  • 슬라이더 -> 구간 선택

확인 해주시고 코멘트 및 재요청 주세요!

Copy link
Collaborator

@cruelladevil cruelladevil left a 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')};
Copy link
Collaborator

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;
Copy link
Collaborator

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;
Copy link
Collaborator

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);
Copy link
Collaborator

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);
Copy link
Collaborator

@cruelladevil cruelladevil Sep 30, 2023

Choose a reason for hiding this comment

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

💬 partStartTimeTexttoMinSecText(partStartTime)로 고정적이면 좋을 것 같은데 어떻게 생각하시나용

Kapture 2023-10-01 at 01 31 36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[ 🌞 FE ] 프론트엔드 크루들의 빛나는 개발 이야기 하나둘셋 호! ✨ Feat 꼼꼼한 기능 구현 중요하죠
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEAT] 등록 페이지에서 초 등록 구간에서 토글이 가능하게 한다.
3 participants