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

[#131] 대회 종료시 대시보드로 이동 기능 #150

Merged
merged 4 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion frontend/src/components/Submission/Connecting.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ interface Props {
}

export default function Connecting(props: Props) {
if (!props.isConnected) return null;
if (props.isConnected) return null;

return (
<div className={rowStyle}>
Expand All @@ -16,6 +16,7 @@ export default function Connecting(props: Props) {
</div>
);
}

const rowStyle = css({
display: 'flex',
gap: '0.5rem',
Expand Down
22 changes: 18 additions & 4 deletions frontend/src/components/Timer/index.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { css } from '@style/css';

import { useNavigate } from 'react-router-dom';

import Loading from '@/components/Common/Loading';
import useTimer from '@/hooks/timer/useTimer';
import { formatMilliSecond } from '@/utils/date';
Expand All @@ -8,14 +10,26 @@ import type { Socket } from '@/utils/socket';
interface Props {
socket: Socket;
endsAt: Date;
isConnected?: boolean;
isConnected: boolean;
[key: string]: unknown;
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 property는 어떤 용도인가요?

Copy link
Collaborator Author

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를 구현할 때 사용할 수 있게 했습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

였지만, 지금은 없습니다.

}

const DASHBOARD_URL = '/contest/dashboard';
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 상수로 뺀 거 정말 좋은 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이런것들도 .env에서 관리하고 싶은데 어떻게 생각하시나요

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 정말 좋은 방법이라고 생각해요! 기조님과 같이 한 번 얘기해보면 좋을 것 같네요:D

Copy link
Collaborator

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는 깃허브에 올라가지 않기 때문에 직접 동기화를 해야해서 번거로울 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

다만 하나의 상수 파일로 관리하는 것은 좋아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇네요. 환경 변수로 관리하기 보다 상수 파일로 관리하는 게 더 낫겠습니다


export default function Timer(props: Props) {
let { socket, endsAt, isConnected } = props;
const navigate = useNavigate();

let { socket, endsAt, isConnected, ...rest } = props;
// api 연결이 X endsAt 대신 임시로 만들어놓은 것.
endsAt = new Date('2023-11-29T13:10:10.000Z');
const { remainMiliSeconds } = useTimer({ socket, endsAt });
// min 1 => 60초 동안 돌아갑니다. 변경해서 쓰세요 일단은..
const min = 1;
endsAt = new Date(new Date().getTime() + min * 60 * 1000);

const onTimeoutHandler = () => {
navigate(`${DASHBOARD_URL}/${rest.competitionId}`);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

구지 rest로 사용하는 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

있었지만, 지금은 없습니다


const { remainMiliSeconds } = useTimer({ socket, endsAt, onTimeoutHandler });
Copy link
Collaborator

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를 수행하는게 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋습니다. 그렇게 하니 범용성 있어졌네요


if (isConnected && remainMiliSeconds !== -1) {
// 연결도 되어있고, 서버 시간도 도착해서 count down을 시작할 수 있을 때
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/hooks/timer/useTimer.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

개인적인 의견인데 useTimer에서 socket을 받고 사용하는게 어색하게 느껴져요. 아무래도 이름이 범용적이라 그런 것 같네요. socket을 사용하는 로직을 분리할 수 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Timer이름도 Timer인데 socket을 받는게 이상해서 SocketTimer로 변경했습니다.

Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ export default function useTimer({ socket, endsAt, onTimeoutHandler }: UseTimer)
}, []);

useEffect(() => {
// TODO time 0이면 대시보드로 이동하는 로직
// 해당 PR에서 해결할 문제는 아니라 PASS
// 초기 값인 -1 => 서버에서 시간이 오지 않았다.
if (remainMiliSeconds === -1) return;
if (Math.floor(remainMiliSeconds / 1000) <= 0) {
if (typeof onTimeoutHandler === 'function') onTimeoutHandler();
// 나가는 로직
Expand Down
13 changes: 8 additions & 5 deletions frontend/src/pages/ContestPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,25 @@ export default function ContestPage() {
submitSolution(form);
}


const { endsAt } = competition;

function handleOpenModal() {
modal.open();
}

const problems = problemList.map((problem) => problem.id);


return (
<main className={style}>
<CompetitionHeader crumbs={crumbs} id={competitionId} />
<section>
<section className={rowStyle}>
<span className={problemTitleStyle}>{problem.title}</span>
<Timer socket={socket.current} isConnected={isConnected} endsAt={new Date(endsAt)} />
<Timer
socket={socket.current}
isConnected={isConnected}
endsAt={new Date(endsAt)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 식으로 new Date를 안에 작성해줄 수 있군요:D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 endsAt가 스웨거에선 string으로 오더라구요. 바뀔수도 있겠지만 string으로 온다고 생각해서 Date로 변경해서 사용하고 있습니다.

competitionId={competitionId}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Timer는 competitionId를 받는 것은 다소 엉뚱해보여요. competitionId를 사용하는 로직을 분리할 수 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞습니다. 그냥 종료시에 어디로 이동할지가 필요해서 대회번호를 넘겨줬는데 타이머를 사용할 페이지에서 끝났을 시에 수행할 함수를 넘겨주는 쪽으로 바꿨습니다.

/>
</section>
<section className={rowListStyle}>
<ContestProblemSelector
Expand Down