-
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
[ Fix ] 오늘 나의 몰입시간(accumulate Time) 2배로 증가하는 버그 해결 #193
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에 자세히 작성해 주신 덕분에 수정 로직을 이해하는 데 아주 큰 도움이 되었어요! ㅎ.ㅎ
네이밍 수정까지 같이 이루어져 코드 직관성과 간결성도 개선된 것 같습니다! 수고 많으셨어요 :)
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.
버그 해결 후 코드가 훨씬 간결하고 가독성이 좋아졌네요! 너무 좋습니다 👍🏻
@@ -63,7 +66,7 @@ const Timer = ({ | |||
<InnerCircleIcon className="absolute" /> | |||
<div className="absolute flex h-[22rem] w-[27.1rem] flex-col items-center justify-center"> | |||
<div className="flex flex-col items-center justify-center"> | |||
<AccumulatedTime isPlaying={isPlaying} totalTimeOfToday={accumulatedTime} /> | |||
<AccumulatedTime accumulatedTime={accumulatedTime} /> | |||
<TaskTime isPlaying={isPlaying} timer={timerTime} /> |
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.
로직 수정 후 AccumulateTime과 TaskTime은 props로 받은 내용을 표시하는 UI 컴포넌트적 역할만 하고 있네요!
UI 구조가 단순하고 재사용되지 않는 부분인 것 같아 별도의 컴포넌트로 분리하지 않아도 될 것 같은데, 건휘님께서 해당 컴포넌트들을 분리하신 기준이 궁금해요!
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.
별도의 컴포넌트로 분리하지 않아도
TaskTime과 AccumulatedTime이 단순한 UI 표시 역할만 하더라도, 이를 분리해 두면 더 명확하게 각 컴포넌트가 맡은 역할을 구분하기 용이하다고 생각했습니다. 뿐만 아니라, 각 컴포넌트가 특정 작업만 담당하므로 컴포넌트의 복잡도를 낮춰 가독성 측면에서도 유리하다고 판단하였습니다!
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.
LGTM
- 원인
- 기존
AccumulatedTime.tsx
내부에서useTimerCount
훅을 선언하여 사용했는데, 이 훅의 초기 값으로 서버에서 받아온totalTimeOfToday
값을 사용하였다. - 타이머를 멈추고 시작하는
onClick
함수에서 멈추는 동작이 있을 때increasedTime
을 reset해줘야 했지만, 해당 함수는AccumulatedTime.tsx
상위에서 정의되었기 떄문에,AccumulatedTime.tsx
내부에서 선언된useTimerCount의 increasedTime
을 초기화 수 없었다.
(리액트의 단방향 데이터 흐름)
- 기존
- 해결
AccumulatedTime.tsx
에서 쓰이는useTimerCount
를 상위(TimerPage.tsx
)에서 선언해, 타이머를 멈추고 실행하는 과정에서increasedTime
을 reset 할수 있게하여 해결
위 내용과 같이 이해했는데 맞을까요?!
잘 작동하는것 확인했습니다 고생하셨어요~ pr 자세히 남겨주셔서 이해하는데 도움이되었습니다.
정확합니다!! 제가 더 잘 풀어서 설명 했어야하는데 감사합니다. |
🔥 Related Issues
✅ 작업 리스트
🔧 작업 내용
컴포넌트 구조
TImerPage(최상위)=> Timer => AccumulatedTime
기존 AccumulatedTime 컴포넌트(총 몰입 시간을 보여주는 컴포넌트)
기존에는 useEffect를 사용하여 isPlaying(타이머 재생 여부)이 바뀔 때 마다 resetIncreasedTime()을 호출하는 방식으로 increasedTime이 누적되는 것을 방지하였는데 useEffect를 사용하지 않고 해결하는 방법을 시도해보았습니다.
1. TimerPage(최상단 페이지)
totalTimeOfToday(오늘 총 몰입 시간)을 타이머가 작동할 때 +1씩 증겨시켜주는 useTimerCount훅을 사용한 로직을 최상단 컴포넌트인 TimerPage로 옮기고
timer(총 몰입 시간)
와resetIncreasedTime(증가한 시간 초기화)
을 추출하여 props로 넘겨주었습니다.2. Timer.tsx
play버튼을 클릭하여 타이머가 정지되었을 때 호출하는 api의
onSucess
조건에resetAccumulatedIncreasedTime()
을 추가해주었습니다.onSucess
조건에도resetAccumulatedIncreasedTime()
을 추가해주어 올바르게 increasedTime이 reset될 수 있게 해주었습니다.🧐 새로 알게된 점
useEffect 사용을 최소화하여 예기치 못한 사이드 이팩트를 방지하자!
🤔 궁금한 점
더 좋은 방향이 있으면 피드백 부탁드립니다!
📸 스크린샷 / GIF / Link