-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: 회원가입 시 유저명과 프로필 이모지를 입력받도록 폼 수정 #880
Conversation
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:
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/api/join.ts
Outdated
export const postJoin = ({ | ||
emoji, | ||
email, | ||
password, | ||
userName, | ||
}: JoinParams): Promise<AxiosResponse> => { | ||
return api.post('/members', { emoji, email, password, userName }); | ||
}; |
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.
export const postJoin = ({ | |
emoji, | |
email, | |
password, | |
userName, | |
}: JoinParams): Promise<AxiosResponse> => { | |
return api.post('/members', { emoji, email, password, userName }); | |
}; | |
export const postJoin = (params: JoinParams): Promise<AxiosResponse> => { | |
return api.post('/members', params); | |
}; |
이런 식으로 작성하는건 어떨까요? 지금과 같은 방식이면 추후 Interface가 변경됐을 때 매번 arguments와 parameter에 변경되는 필드를 명시해야 해서 불편할거 같아서용
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.
네, params를 직접 넘겨주는 경우라면 굳이 destructuring하지 않아도 되겠네요
피드백 감사합니다 :)
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.
8736f41 에서 반영하였습니다.
frontend/src/constants/message.ts
Outdated
VALID_USERNAME: '유효한 이름입니다.', | ||
INVALID_USERNAME: '특수문자는 _ . , ! ? 만 허용됩니다.', |
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.
VALID_USERNAME
은 value와 매칭이 잘 되는데 INVALID_USERNAME
은 value와 매칭이 잘 안 되는거 같아용
NOW_ALLOWED_ CHARACTER
같은 이름은 어떨까요?
(이것도 적고 보니 썩 좋아보이진 않는거 같기도 하고...)
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.
해당 네이밍은 기존의 INVALID_ORGANIZATION
과 컨벤션을 맞추었다고 생각해주시면 좋을 것 같습니다. 저는 나쁘지 않다고 생각하는데 어떠신가요?
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.
아하 그렇군요 이대로 가도 좋을거 같네용 👍 👍 👍
): UseQueryResult<TData, AxiosError> => | ||
useQuery(['getEmojiList'], getEmojiList, { | ||
...options, | ||
refetchOnWindowFocus: false, |
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.
해당 쿼리에 refetchOnWindowFocus
를 false
로 주신 이유가 있을까용?
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.
이모지 리스트는 실시간으로 데이터를 업데이트할 필요가 없다고 생각해서 반복적으로 fetch해오는 상황을 막아주고 싶었습니다.
|
||
onSubmit({ email, organization }); | ||
onError: (error: AxiosError<ErrorResponse>) => { | ||
setUserNameMessage(error.response?.data.message ?? ''); |
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.
이건 소소한 부분이긴한데 response에 message가 담겨오지 않을 때는 default로 안내 문구를 띄워주면 좋을거 같네용
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.
c355196 에서 반영했습니다
- 비구조화 할당하지 않고 파라미터를 전달하도록 수정
- 이메일 중복 확인 및 이름 중복 확인 API 요청 후 알 수 없는 오류가 발생했을 때의 메시지를 보여주도록 수정
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.
👍👍👍
구현 기능
공유하고 싶은 내용
Close #879