-
Notifications
You must be signed in to change notification settings - Fork 0
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
[#131] 대회 종료시 대시보드로 이동 기능 #150
The head ref may contain hidden characters: "131-\uB300\uD68C-\uC885\uB8CC\uC2DC-\uB300\uC2DC\uBCF4\uB4DC\uB85C-\uC774\uB3D9-\uAE30\uB2A5"
Conversation
✅ Deploy Preview for algo-with-me ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
<Timer | ||
socket={socket.current} | ||
isConnected={isConnected} | ||
endsAt={new Date(endsAt)} |
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.
이런 식으로 new Date를 안에 작성해줄 수 있군요:D
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.
넵 endsAt가 스웨거에선 string으로 오더라구요. 바뀔수도 있겠지만 string으로 온다고 생각해서 Date로 변경해서 사용하고 있습니다.
} | ||
|
||
const DASHBOARD_URL = '/contest/dashboard'; |
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.
이렇게 상수로 뺀 거 정말 좋은 것 같아요!
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.
이런것들도 .env에서 관리하고 싶은데 어떻게 생각하시나요
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.
저는 정말 좋은 방법이라고 생각해요! 기조님과 같이 한 번 얘기해보면 좋을 것 같네요:D
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.
음...저는 반대.
.env는 환경 변수가 들어가는 영역인데, DASHBOARD_URL은 비지니스 로직인 것 같아요.
그리고 페이지가 추가되거나 삭제됨에 따라 .env를 갱신해줘야 하는데 .env는 깃허브에 올라가지 않기 때문에 직접 동기화를 해야해서 번거로울 것 같습니다.
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.
다만 하나의 상수 파일로 관리하는 것은 좋아요.
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.
그렇네요. 환경 변수로 관리하기 보다 상수 파일로 관리하는 게 더 낫겠습니다
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:D!
navigate(`${DASHBOARD_URL}/${rest.competitionId}`); | ||
}; | ||
|
||
const { remainMiliSeconds } = useTimer({ socket, endsAt, onTimeoutHandler }); |
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.
onTimeoutHandler를 받지 말고, useTimer가 isTimeout을 반환하면 isTimeout을 useEffect에서 감지하고 onTimeoutHandler를 수행하는게 어떨까요?
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.
좋습니다. 그렇게 하니 범용성 있어졌네요
frontend/src/hooks/timer/useTimer.ts
Outdated
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.
개인적인 의견인데 useTimer에서 socket을 받고 사용하는게 어색하게 느껴져요. 아무래도 이름이 범용적이라 그런 것 같네요. socket을 사용하는 로직을 분리할 수 있을까요?
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.
Timer이름도 Timer인데 socket을 받는게 이상해서 SocketTimer로 변경했습니다.
@@ -8,14 +10,26 @@ import type { Socket } from '@/utils/socket'; | |||
interface Props { | |||
socket: Socket; | |||
endsAt: Date; | |||
isConnected?: boolean; | |||
isConnected: boolean; | |||
[key: string]: unknown; |
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.
이 property는 어떤 용도인가요?
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.
isConnected 인가요 아니면 [key:string]인가요
isConnected는 소켓 연결상태를 나타내고,
[key: string]: unknown;는 자유롭게 프로퍼티 받아서 onTimeoutHandler를 구현할 때 사용할 수 있게 했습니다.
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 onTimeoutHandler = () => { | ||
navigate(`${DASHBOARD_URL}/${rest.competitionId}`); | ||
}; |
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.
구지 rest로 사용하는 이유가 있나요?
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.
있었지만, 지금은 없습니다
frontend/src/pages/ContestPage.tsx
Outdated
socket={socket.current} | ||
isConnected={isConnected} | ||
endsAt={new Date(endsAt)} | ||
competitionId={competitionId} |
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.
Timer는 competitionId를 받는 것은 다소 엉뚱해보여요. competitionId를 사용하는 로직을 분리할 수 있을까요?
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.
맞습니다. 그냥 종료시에 어디로 이동할지가 필요해서 대회번호를 넘겨줬는데 타이머를 사용할 페이지에서 끝났을 시에 수행할 함수를 넘겨주는 쪽으로 바꿨습니다.
1 useTimer => useSocketTimer로 변경 Timer 사용하는 페이지에서 타이머_끝났을_때_실행할_함수를 구현하고 Timer에게 내려줌. <Timer 소켓, 연결 상태, 끝나는 시간, 타이머_끝났을_때_실행할_함수 /> Timer 구현부 만약 socket말고 http로 시간을 받아온다면 프로퍼티로 받는 socket이 있냐 없냐에 따라 갖고오는 커스텀 훅 변경이 필요. 일단은 Timer도 SocketTImer로 바꿨습니다. Timer인데 소켓을 받는 게 이상해서 |
const { remainMiliSeconds } = useTimer({ socket, endsAt }); | ||
// min 1 => 60초 동안 돌아갑니다. 변경해서 쓰세요 일단은.. | ||
const min = 120; | ||
endsAt = new Date(new Date().getTime() + min * 60 * 1000); |
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.
2시간을 본다고 가정하는거죠?
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.
네 정확합니다. 직접 데이터 받아오니 끝나는 시간이 항상 지금보다 전이라 대시보드로 바로 이동해서 개발용으로 열어뒀습니다.
}); | ||
|
||
useEffect(() => { | ||
if (isTimeout && typeof onTimeout === 'function') onTimeout(); |
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.
여담인데 isFunction 같은 유틸을 하나 만들어야겠네요. 제쪽에서 할게요
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.
👯
한일
작동 화면
2023-11-28.10.02.11.mov