Feature/role constansts로 빼기 #848 #852

pipisebastian · 2023-12-10T09:40:46Z

연관 이슈

Close Role constansts로 빼기 #848

작업 요약

�member role(회장, 부회장, 사서 등등) 상수로 빼줬습니다.

To 리뷰어... 💌

예상 리뷰 소요 시간 5분
코멘트 봐주시면 감사하겠습니다!

#848

pipisebastian · 2023-12-10T10:03:09Z

src/constants/member.ts

+  BACK_전산관리자: 'ROLE_BACK_전산관리자',
+  INFRA_전산관리자: 'ROLE_INFRA_전산관리자',
+  회원: 'ROLE_회원',
+  출제자: 'ROLE_출제자',


상수를 선언할 때, chairman 이 아닌 한글로 회장으로 선언해줬습니다! 영어로 하면 오히려 코드읽기가 어려울 것 같아서 입니다..! 그런데 한글의 경우, FRONT_전산관리자같은 경우 어떻게 선언해줘야할지(카멜케이스, _ 등등) 고민입니다..

한글 섞여있는 경우 _ 좋은 것 같아요!

저도 영어랑 한글이 섞여있다면 언더바로 구분하는게 좋은 것 같습니다~!!

pipisebastian · 2023-12-10T10:06:43Z

src/constants/member.ts

+export const MEMBER_ROLE3 = {
+  회장: prefix('회장'),
+  부회장: prefix('부회장'),
+} as const;


MEMBER_ROLE 상수를 선언하는 방식이 3가지인데, 어느게 좋을지 고민이 됩니다..

ROLE_회장과 같이 바로 텍스트로 넣어주기 -> 해당 방식이 간단하지만, ROLE_ 부분이 중복되는데 이걸 바로 박아버리기 애매했습니다.

prefix 변수 백틱안에 같이 넣어주기

prefix 함수 만들어서 사용하기

왜죠!!!!!

저는 1번도 괜찮을 것 같긴 합니다!

이유

에러 메시지를 상수로 바꾸는 과정에서는 (중복되더라도) 그냥 텍스트로 사용합니다.

prefix() 가 어떤 형태인지 알고 있다면 이해할 수 있겠지만, 모른다면 해당 함수를 타고 들어가서 확인을 해야할 것 같습니다

근데 공통되는걸 묶으려면 백틱을 사용하는게 조금 더 전달력이 좋을 것 같습니당

왜죠!!!!!

1. ROLE_이라는 prefix를 백엔드와 공통적으로 사용하고 있는데, 직책을 어떻게 표기할 지에 따라서 충분히 변경 가능한 부분으로 보입니다. 1번으로 가게되면 변경될 경우 수동으로 업데이트 해주어야 하는 게 번거로울 것 같아요!

3. 사실 이게 제일 깔끔해보이지는 한데, 단순한 텍스트에 함수 실행문이 들어가는 게 조금 과한 것 같다는 생각도 들더라구요!

그래서 최종적으로 prefix 변수를 사용하지만 템플릿 리터럴로 표현된 2번으로 선택하였습니다! 하지만 prefix로 사용되는 ROLE_이 크게 변동이 많을 것 같은 부분은 아니라서 1번도 괜찮을 것 같아요!

저도 2번이 좋아보입니다.
Prefix는 공통이라 추후에 바뀔때 처리도 쉬울듯 하고,
실제 사용하는 입장에서는 1번과 2번 모두 똑같이 보여서
코드쪽 처리가 쉬운 2번이 좋을거 같습니다.
밑에는 참고 이미지 입니다!

1번

2번

publdaze · 2023-12-10T10:03:11Z

src/pages/admin/DutyManage/Tooltip/DutyProfileTooltip.tsx

-      {jobName !== 'ROLE_전산관리자' ? (
+      {jobName !== MEMBER_ROLE.전산관리자 && (


publdaze

1,2,3 선택된 걸로 반영되면 approve 하겠습니다!

…nt-R2 into feature/Role-constansts로-빼기-#848 # Conflicts: # src/components/Layout/Sidebar/Sidebar.tsx

jasper200207

👍

jasper200207 · 2023-12-11T10:39:53Z

src/constants/member.ts

+export const MEMBER_ROLE3 = {
+  회장: prefix('회장'),
+  부회장: prefix('부회장'),
+} as const;


저도 2번이 좋아보입니다.
Prefix는 공통이라 추후에 바뀔때 처리도 쉬울듯 하고,
실제 사용하는 입장에서는 1번과 2번 모두 똑같이 보여서
코드쪽 처리가 쉬운 2번이 좋을거 같습니다.
밑에는 참고 이미지 입니다!

1번

2번

mun-kyeong

좋습니다~!! 질문에 답해주셔서 감사해용 ㅎㅎ 수고하셨습니다!!

mun-kyeong · 2023-12-11T11:03:10Z

src/pages/admin/DutyManage/Tooltip/DutyProfileTooltip.tsx

-  }
-
+  const badgeImage = roles.find((role) => role.name === jobName)?.img;
+  // NOTE jobName으로는 "ROLE_전산관리자" 내려오지만, roles에는 존재하지 않습니다! 세부적인 전산관리자(프론트, 백, 인프라)만 존재합니다


오옹 이런 주석 좋은것같슴당

mun-kyeong · 2023-12-11T11:05:17Z

src/constants/member.ts

+  BACK_전산관리자: 'ROLE_BACK_전산관리자',
+  INFRA_전산관리자: 'ROLE_INFRA_전산관리자',
+  회원: 'ROLE_회원',
+  출제자: 'ROLE_출제자',


저도 영어랑 한글이 섞여있다면 언더바로 구분하는게 좋은 것 같습니다~!!

#848

publdaze

굿굿!

pipisebastian added 2 commits December 10, 2023 18:10

feat : member role 상수 추가

d7d38a5

#848

refactor : 상수 적용

50041ff

#848

pipisebastian requested a review from publdaze December 10, 2023 09:40

pipisebastian commented Dec 10, 2023

View reviewed changes

publdaze reviewed Dec 10, 2023

View reviewed changes

refactor : 전산관리자 관련 주석 추가

d76f444

publdaze requested changes Dec 10, 2023

View reviewed changes

pipisebastian requested review from jasper200207 and mun-kyeong December 10, 2023 10:42

pipisebastian self-assigned this Dec 10, 2023

pipisebastian added the Refactor 리팩토링 label Dec 10, 2023

Merge branch 'develop' of https://github.com/KEEPER31337/Homepage-Fro…

64e36bf

…nt-R2 into feature/Role-constansts로-빼기-#848 # Conflicts: # src/components/Layout/Sidebar/Sidebar.tsx

jasper200207 approved these changes Dec 11, 2023

View reviewed changes

mun-kyeong approved these changes Dec 11, 2023

View reviewed changes

refactor : prefix 사용하여 상수 선언

d61376b

#848

publdaze approved these changes Dec 11, 2023

View reviewed changes

pipisebastian merged commit 3cf2d4a into develop Dec 11, 2023
1 check passed

pipisebastian deleted the feature/Role-constansts로-빼기-#848 branch December 11, 2023 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/role constansts로 빼기 #848 #852

Feature/role constansts로 빼기 #848 #852

pipisebastian commented Dec 10, 2023 •

edited

Loading

pipisebastian Dec 10, 2023

publdaze Dec 10, 2023

mun-kyeong Dec 11, 2023

pipisebastian Dec 10, 2023

publdaze Dec 10, 2023

pipisebastian Dec 10, 2023

mun-kyeong Dec 10, 2023

publdaze Dec 11, 2023 •

edited

Loading

jasper200207 Dec 11, 2023

publdaze Dec 10, 2023

publdaze left a comment

jasper200207 left a comment

jasper200207 Dec 11, 2023

mun-kyeong left a comment

mun-kyeong Dec 11, 2023

mun-kyeong Dec 11, 2023

publdaze left a comment

		{jobName !== 'ROLE_전산관리자' ? (
		{jobName !== MEMBER_ROLE.전산관리자 && (

Feature/role constansts로 빼기 #848 #852

Feature/role constansts로 빼기 #848 #852

Conversation

pipisebastian commented Dec 10, 2023 • edited Loading

연관 이슈

작업 요약

To 리뷰어... 💌

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

publdaze Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

publdaze left a comment

Choose a reason for hiding this comment

jasper200207 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mun-kyeong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

publdaze left a comment

Choose a reason for hiding this comment

pipisebastian commented Dec 10, 2023 •

edited

Loading

publdaze Dec 11, 2023 •

edited

Loading