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.

Sign up for GitHub

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

Jump to bottom

Feature/에러 메세지 상수화 #846 #855

Merged
merged 9 commits into from
Dec 11, 2023
Merged

Feature/에러 메세지 상수화 #846 #855

merged 9 commits into from
Dec 11, 2023

Conversation

mun-kyeong
Copy link
Collaborator

연관 이슈

작업 요약

  • api와 연관된 error 및 success 메세지들을 상수로 묶어서 적용하였습니다.

작업 상세 설명

  • 각 주제별로 error 와 success로 나누어 메세지를 분류하였습니다.

리뷰 요구사항

  • 예상 리뷰 소요시간 2분

@mun-kyeong mun-kyeong added the Refactor 리팩토링 label Dec 10, 2023
@mun-kyeong mun-kyeong self-assigned this Dec 10, 2023
publdaze
publdaze requested changes Dec 10, 2023
View reviewed changes
Copy link
Contributor

@publdaze publdaze left a comment

Choose a reason for hiding this comment

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

eslint 에러나는 거 확인 부탁드려요!
대부분 절대경로로 사용되는(@ 붙은거!) 것들이 상대 경로로 사용된 것 보다 위치가 밑에 있어서 뜨는 에러 같아요!

publdaze
publdaze reviewed Dec 10, 2023
View reviewed changes
src/constants/apiResponseMsg.ts Outdated
success: {},
error: {
noSubmissionsLeft: '남은 제출 횟수가 없습니다.',
invalidAttendanceWithCount: (min: number) => `출석코드가 틀렸습니다. (남은 제출횟수 ${min}회)` as const,
Copy link
Contributor

Choose a reason for hiding this comment

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

요기는 왜 as const가 붙은 걸까요?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

as const 가 없으면 함수의 출력 형식이 string 으로 잡히고 있으면 햅틱 형식에 맞춰서 잡힙니다.
아래는 참고용 이미지 입니다. 한가지 문제점은 vscode상에서는 햅틱 안의 한글이 깨져보입니다....

(min: number) => `출석코드가 틀렸습니다. (남은 제출횟수 ${min}회)`
스크린샷 2023-12-11 오후 7 45 22

(min: number) => `출석코드가 틀렸습니다. (남은 제출횟수 ${min}회)` as const
스크린샷 2023-12-11 오후 7 44 40

pipisebastian
pipisebastian reviewed Dec 10, 2023
View reviewed changes
src/constants/apiResponseMsg.ts Outdated
errror: {
existingStudentId: '이미 존재하는 학번입니다.',
},
};
Copy link
Member

Choose a reason for hiding this comment

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

폴더명 Msg 보다는 풀어서 Message는 어떨까용!

src/api/memberApi.ts Outdated
@@ -123,7 +124,7 @@ const useEditEmailMutation = () => {
const { handleError } = useApiError({
400: {
default: () => {
toast.error('현재 비밀번호가 일치하지 않습니다.');
toast.error(PASSWORD.error.passwordMismatch);
Copy link
Member

Choose a reason for hiding this comment

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

passwordMismatch보다 �mismatchPassword 는 어떨까용..? duplicateSeminarDate도 그렇고 동사 -> 명사 순인 것 같아서 통일하면 좋을 것 같아요!

src/constants/apiResponseMsg.ts Outdated

export const PASSWORD = {
success: {
changedSuccess: '비밀번호가 변경되었습니다.',
Copy link
Member

Choose a reason for hiding this comment

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

changedSuccess 보다 changedPassword는 어떨까합니다! PASSWORD.success.~ 로 이미 success인걸 바로 알 수 있어서용!

Copy link
Member

Choose a reason for hiding this comment

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

음음.. 아예 changedPassword 보다 changed 로도 충분히 이해되지 않을까.. 합니다...

src/constants/apiResponseMsg.ts Outdated

export const ID = {
success: {},
errror: {
Copy link
Member

Choose a reason for hiding this comment

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

errror! r이 중간에 3개에용!

pipisebastian
pipisebastian reviewed Dec 11, 2023
View reviewed changes
src/constants/apiResponseMsg.ts Outdated
changedSuccess: '비밀번호가 변경되었습니다.',
},
error: {
passwordMismatch: '현재 비밀번호가 일치하지 않습니다.',
Copy link
Member

@pipisebastian pipisebastian Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

이부분도 mismatch로 적어도 이해할 수 있을 것 같아요! PASSWORD.error.mismatch 로 들어가니까요!

publdaze
publdaze approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@publdaze publdaze left a comment

Choose a reason for hiding this comment

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

굿입니다!

jasper200207
jasper200207 approved these changes Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@jasper200207 jasper200207 left a comment

Choose a reason for hiding this comment

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

큰 코멘트는 없어서 Approve 하겠습니다. 👍

src/constants/apiResponseMessage.ts Outdated
},
} as const;

export const POST = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

큰 상관은 없을 듯 하긴 한데 POST 대신 BOARD는 어떨까요...?
REST API의 POST랑 용어가 겹쳐보입니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

오옹 저도 REST API의 POST가 살짝 떠올랐었는데..! ㅎㅎ

src/constants/apiResponseMessage.ts Outdated
},
} as const;

export const ID = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

API와 용어를 맞추기 위해 LOGIN_ID도 괜찮아 보입니다.

pipisebastian
pipisebastian approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@pipisebastian pipisebastian left a comment

Choose a reason for hiding this comment

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

굿이에용~👍

@mun-kyeong mun-kyeong merged commit 34c3880 into develop Dec 11, 2023
1 check passed
@mun-kyeong mun-kyeong deleted the feature/에러_메세지_상수화_#846 branch December 11, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@publdaze publdaze publdaze approved these changes

@pipisebastian pipisebastian pipisebastian approved these changes

@jasper200207 jasper200207 jasper200207 approved these changes

Assignees

@mun-kyeong mun-kyeong

Labels
Refactor 리팩토링
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mun-kyeong @jasper200207 @publdaze @pipisebastian