Skip to content

Conversation

@gomsbft
Copy link
Collaborator

@gomsbft gomsbft commented May 29, 2025

개요

회원가입 폼 작업 했습니다

20250529 과제 제출합니다
- 회원가입 폼 구현
@gomsbft gomsbft requested a review from univdev May 29, 2025 11:31
<meta charset="UTF-8" />
<link rel="icon" type="image/svg+xml" href="/vite.svg" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Vite + React + TS</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 부분은 별로 중요하진 않은데, 웹사이트 초기화 이후 수정해주시는 습관을 들여주시면 좋을 것 같아요.

Copy link
Contributor

Choose a reason for hiding this comment

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

사용하지 않는 코드는 제거 필요합니다.

console.log('SignUp values:', values);
};
return (
<Typography variant="h1" component="div">
Copy link
Contributor

Choose a reason for hiding this comment

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

SignUpForm 컴포넌트가 Typography 컴포넌트에 들어가는 구조는 자연스럽지 못해요.
텍스트를 표현하는 태그이기 때문에 만약 div 태그를 표현하고 싶으셨다면 태그에 대해서 검색 해보시면 좋습니다.

password?: string;
username?: string;
phoneNumber?: string;
onSubmit?: (values: {
Copy link
Contributor

Choose a reason for hiding this comment

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

onSubmit 타입에 정의 된 values 매개변수의 구조가 복잡해 보여요.
코드 가독성을 위해 아래와 같이 변경하면 좋을 것 같습니다.

type OnSubmitArgs = {
  // ... 기존 구조체
}

const [username, setUsername] = useState(initUsername ?? '');
const [phoneNumber, setPhoneNumber] = useState(initPhoneNumber ?? '');

const handleSubmit = (event: React.FormEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

매개변수에 따로 타입을 지정하시는게 아니라 아래와 같은 방식을 쓰시면 코드 가독성에 도움이 됩니다.

  const handleSubmit: FormEventHandler<HTMLFormElement> = (event) => {
    event.preventDefault();
    onSubmit?.({ id, password, username, phoneNumber });
  };

noValidate
autoComplete="off"
>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

div는 현재 역할이 없는 것 같은데 페이지 구조의 복잡도를 줄이기 위해 제거가 필요해 보입니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

추가로, 코드의 통일성을 위해 <Box> 컴포넌트 활용 해주시면 좋을 것 같습니다 :)

return (
<form onSubmit={handleSubmit}>
<Box
component="form"
Copy link
Contributor

Choose a reason for hiding this comment

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

<Box> 태그가 현재 component 속성에 의해 form 태그로 렌더링이 되고 있어요.
그런데 바깥에 form 태그가 이미 존재하기 때문에 중첩 form 구조가 되고 있습니다.
둘 중 하나 제거가 필요해 보여요.

개인적인 추천으로는 바깥 form을 남겨두시고 Box form을 제거하시는게 가독성 측면에서 좋아보입니다.

/>
</div>
</Box>
<Stack spacing={2} direction="row">
Copy link
Contributor

Choose a reason for hiding this comment

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

Stack은 하위에 연속적으로 존재하는 컴포넌트를 배치할 때 사용합니다.
이 경우에는 하위에 Button이 하나밖에 없기 때문에 Box 로 영역을 구분하시는게 직관적일 것 같습니다.

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