-
Notifications
You must be signed in to change notification settings - Fork 4
[feat] 공고 등록 페이지 구현 #71
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
Conversation
✅ Deploy Preview for thejulge1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cozy-ito
left a comment
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.
고생하셨습니다! 👍
|
|
||
| if (isSubmitting) return; | ||
|
|
||
| const requiredFields: Array<keyof FormType> = [ |
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 requiredFields: Array<keyof FormType> = [
"hourlyPay",
"startsAt",
"workhour",
];
export default function NoticeRegisterPage() {
...
const handleSubmit = async () => { ... };
...
}이 변수는 컴포넌트 외부로 위치시켜도 좋을 것 같아요 👍
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.
다른 페이지들에서처럼 missingField를 다루는 조건문 위에서 requiredFields를 다루는 게 코드를 보기에 이해하기 좋을 거 같다고 생각했는데, 혹시 공고 등록 페이지에서만 특히 requiredFields를 컴포넌트 밖으로 이동하는 게 좋다고 느낀 이유가 있으실까요..?!
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.
매 렌더링 시에 배열이 재생성되긴 하지만 재생 비용이 미미하고 문맥 가독성을 챙기는 게 우선이라구 생각합니다..! 일단 그대로 진행해보겠습니다! 꼭 필요하다고 생각하신다면 팀회의 때 논의해본 후에 버그 픽스 과정 중에 바꿔도 될 것 같아요 🤓
jeonghwanJay
left a comment
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.
수고하셨습니다 !!
| hourlyPay: string; | ||
| startsAt: Date | null; | ||
| workhour: string; | ||
| description: string; |
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.
description 속성은 requiredFields에 포함되지 않아서 옵셔널로 주어도 좋을 것 같습니다.!
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.
하단에서 description을 빈 문자열 ""로 초기화하여서 타입도 string으로 맞췄습니다!
만약 description을 옵셔널(description?: string) 로 만들게 되면 undefined일 가능성이 생기고, form.description.trim()과 같은 코드에서 에러가 발생하게 되더라구요. description이 옵셔널이 된다면 매번 form.description?.trim() ?? ""와 같은 처리가 필요해서 불필요하게 복잡해진다고 생각합니당
입력은 선택이지만, 항상 빈 문자열("")을 기본값으로 가진다는 가정하에 사용하는 느낌입니다!
| alert("유효한 시급을 입력해 주세요."); | ||
| return; | ||
| } | ||
|
|
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.
description이 옵셔널이므로 requiredFields에 포함이 안되는 건 맞지만, 만약 description이 입력되었을 때 "최대 길이 500자" 같은 유효성 검사 로직을 고려해봐도 좋을 것 같습니다 ~
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.
우선 TextArea 자체에 maxLength={500}을 두는 방식으로 구현해보았습니다. 추후에 UX 개선 과정에서 라벨 자체에 '공고 설명 (최대 500자)' 이런 식으로 설정하는 방식을 논의해보면 좋을 것 같습니다!!
#️⃣연관된 이슈
📝 PR 유형
📝작업 내용
스크린샷 (선택)
💬리뷰 요구사항(선택)