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

[#67] 대회 생성 페이지 구현 #100

Merged
merged 26 commits into from
Nov 23, 2023
Merged

Conversation

dev2820
Copy link
Collaborator

@dev2820 dev2820 commented Nov 23, 2023

한 일

  • 대회 생성 페이지로의 라우터를 추가한다.
  • 문제를 선택할 수 있는 문제 리스트를 구현한다.
  • 문제 선택 여부는 토글할 수 있다.
  • 문제 이름, 시작 시간, 종료 시간을 입력할 수 있는 form이 있다.
    • 입력을 검증한다. (필요한데 없거나 잘못된 입력을 검사하고 대회 생성을 막는다.)
  • 대회 생성에 성공하면 리스트 페이지로 라우팅한다.

검증 로직

  • 대회 이름은 1글자 이상
  • 대회 최대 참여 인원은 1명 이상
  • 시작 시간은 현재 시간보다 5분 늦게 설정 가능
  • 종료 시간은 시작 시간보다 늦어야함
  • 문제 수는 1개 이상이어야함

스크린 샷

/contest/create 페이지에서 확인할 수 있습니다.

avaabba
form 검증 로직,
마지막에 contest/detail/id 화면으로 이동하도록 해두었는데, 아직 해당 페이지가 없어서 그렇게 나옴

- 문제를 선택할 수 있도록 기능을 추가함
- 대회 생성시 detail 페이지로 이동하도록 기능 추가
- 지정된 서버만 참조하는 api 인스턴스를 util로 분리함
- Response 타입과 데이터 타입을 분리해 정리
- CompetitionCreatePage -> CreateCompetitionPage (영문법에 맞춰 수정함)
- 시작 시작이 옳게 설정되면 에러가 나도록 하고 있었음
- 종료 시간도 마찬가지
@dev2820 dev2820 self-assigned this Nov 23, 2023
}

export function Input({ id, label, children, ...props }: Props) {
const child = Children.only(children);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

입력된 Children이 1개 뿐임을 검증하는 로직입니다


Input.TextField = forwardRef(
({ className, ...props }: TextFieldProps, ref: ForwardedRef<HTMLInputElement>) => {
return <input className={cx(inputStyle, className)} type="text" ref={ref} {...props}></input>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cx는 두 클래스 이름을 합성한다고 보시면 됩니다.

cx('a','b') // 'a b'

Comment on lines +30 to +31
const [startsAt, setStartsAt] = useState<string>(initialForm.startsAt ?? currentDateStr);
const [endsAt, setEndsAt] = useState<string>(initialForm.endsAt ?? currentDateStr);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

기본적으로 현재 시간으로 form이 셋업되도록 합니다.

Comment on lines +17 to +23
const $target = e.target as HTMLElement;
if ($target.tagName !== 'BUTTON') return;

const $li = $target.closest('li');
if (!$li) return;

const problemId = Number($li.dataset['problemId']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

여긴 이벤트 델리게이션 로직입니다.
이벤트 델리게이션이란?

리액트도 자체 델리게이션 로직이 있는걸로 아는데 잘 몰라서 일단 구현 했어요

@dev2820 dev2820 marked this pull request as ready for review November 23, 2023 02:22
@dev2820 dev2820 requested review from dmdmdkdkr and mahwin November 23, 2023 02:22
Comment on lines 40 to 42
</span>
{getSelectButton({ isPicked: pickedProblemIds.includes(id) })}
</li>
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.

아이고 여기 좀 변경했는데 깜빡하고 안올렸네요

message?: string;
};

const FIVE_MIN_BY_MS = 5 * 60 * 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 300000 대신 5 * 60 * 1000 을 쓰신 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어떤 값인지 구분하기 편하게 하려고요. 1000 = 1초

1000 = 1초
1000 * 60 = 1분
1000 * 60 * 5 = 5분

이렇게 가독성을 높이는 겁니다 그리고 이렇게 해두면 나중에 분을 변경할 때도 편해서요

Comment on lines +14 to +20
const VALIDATION_MESSAGE = {
needLongName: '이름은 1글자 이상이어야합니다',
needMoreParticipants: '최대 참여 인원은 1명 이상이어야 합니다',
tooEarlyStartTime: '대회 시작 시간은 현재보다 5분 늦은 시간부터 가능합니다',
tooEarlyEndTime: '대회 종료 시간은 대회 시작시간보다 늦게 끝나야합니다',
needMoreProblems: '대회 문제는 1개 이상이어야합니다',
};
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

Choose a reason for hiding this comment

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

아직 api배포가 되진 않았지만, 대회 전체 조회api응답을 받도록 작성할 예정인데, 그것도 이런 식으로 분리할 수 있겠다는 생각이 드네요!
기조님은 대회 전체 조회 도 이런 식으로 분리하는 게 좋다고 생각하시나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 그렇게 생각합니다. 외부 데이터를 받아오는 영역은 분리해서 관리해주는게 좋아요. Model로직과 ViewModel 로직이 섞이지 않게 분리하는거죠

@dev2820 dev2820 requested a review from dmdmdkdkr November 23, 2023 03:27
Copy link
Collaborator

@dmdmdkdkr dmdmdkdkr left a comment

Choose a reason for hiding this comment

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

Lgtm🧸

Comment on lines +3 to +5
const api = axios.create({
baseURL: import.meta.env.VITE_API_URL,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게도 할 수 있네요 👍🏻

Comment on lines +1 to +2
const ONE_SEC_BY_MS = 1_000;
const ONE_MIN_BY_MS = 60 * ONE_SEC_BY_MS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

변수 네이밍 좋네요.

Comment on lines +3 to +5
export const isNil = (type: unknown): type is Nil => {
if (Object.is(type, null)) return true;
if (Object.is(type, undefined)) return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

type이 unknown은 알겠는데, return이 boolean이 아니라 type is Nil로 타이핑하신 이유가 있나요? ts playground에선 오류가 없어 보이는데 궁금합니다.

Copy link
Collaborator Author

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.

저렇게 하면 type으로 들어온 값이 Nil이 거나 Nil이 아님을 보장할 수 있습니다.

const value:number|undefined = ...; // undefined이거나 number임

if(isNil(value)) {
  // 여기서 value는 Nil이 된다.
}
else {
  // 여기서 value는 number가 된다.
}


import type { CompetitionForm, CompetitionInfo, CreateCompetitionResponse } from './types';

export * from './types';
Copy link
Collaborator

Choose a reason for hiding this comment

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

추가적인 타입이 없어도 이렇게 export 하시나요?
다른 곳에서 타입이 필요하다면, api/competitions/types에서 직접 가져오는 게 더 명확해 보이는데, 궁금합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

추가적인 타입이 없다는 말이 무슨말인지 이해 못했어요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 아닙니다!! 제가 착각했습니다.

Comment on lines +14 to +25
export type CompetitionInfo = {
id: CompetitionId;
name: string;
detail: string;
maxParticipants: number;
startsAt: string;
endsAt: string;
createdAt: string;
updatedAt: string;
};

export type CreateCompetitionResponse = CompetitionInfo;
Copy link
Collaborator

Choose a reason for hiding this comment

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

타입 구조가 완전 같아도 네이밍을 정확히 해주는게 정말 좋아보입니다.

title: string;
};

export type FetchProblemListResponse = ProblemInfo[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

서버 응답 데이터 타입에는 항상 Response를 붙이시네요👍🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 좀 구분하는 편이에요. Response로 들어오는 데이터는 지금은 그냥 데이터이지만 규칙을 정하기에 따라 message나 statusCode를 같이 반환하는 경우도 생기는데 그 때 개념을 분리하기 위함입니다.

Comment on lines +32 to +34
interface TextFieldProps extends Omit<InputHTMLAttributes<HTMLInputElement>, 'type'> {
error?: boolean;
}
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.

ㅋㅋㅋㅋㅋㅋ

Omit = 특정 타입을 제외
InputHTMLAttributes = Input 태그가 가질 수 있는 컴포넌트 attributes 타입입니다.
즉 type을 제외한 모든 Input에 넣을 수 있는 속성을 가리키는 인터페이스에요.

error는 잘못 넣은겁니다. 없다고 봐주세요.

이 부분은 음...시간 되면 잠깐 설명드릴게요

Copy link
Collaborator

Choose a reason for hiding this comment

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

주말에 천천히 읽어보겠습니다. 좋은 코드 너무 감사합니다. 아직은 이해하기 어렵네요.


function togglePickedProblem(problemId: ProblemId) {
if (problems.includes(problemId)) {
setProblems((ids) => ids.filter((id) => id !== problemId).sort());
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

@mahwin mahwin left a comment

Choose a reason for hiding this comment

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

좋은 코드 감사합니다. LGTM 😎

@dev2820 dev2820 merged commit 7ed363f into fe-dev Nov 23, 2023
@dev2820 dev2820 deleted the 67-대회-생성-페이지-구현 branch November 23, 2023 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants