-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/role constansts로 빼기 #848 #852
Merged
The head ref may contain hidden characters: "feature/Role-constansts\uB85C-\uBE7C\uAE30-#848"
+85
−55
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
export const MEMBER_ROLE_PREFIX = 'ROLE_'; | ||
|
||
export const MEMBER_ROLE = { | ||
회장: `${MEMBER_ROLE_PREFIX}회장`, | ||
부회장: `${MEMBER_ROLE_PREFIX}부회장`, | ||
서기: `${MEMBER_ROLE_PREFIX}서기`, | ||
총무: `${MEMBER_ROLE_PREFIX}총무`, | ||
사서: `${MEMBER_ROLE_PREFIX}사서`, | ||
학술부장: `${MEMBER_ROLE_PREFIX}학술부장`, | ||
대외부장: `${MEMBER_ROLE_PREFIX}대외부장`, | ||
전산관리자: `${MEMBER_ROLE_PREFIX}전산관리자`, | ||
FRONT_전산관리자: `${MEMBER_ROLE_PREFIX}FRONT_전산관리자`, | ||
BACK_전산관리자: `${MEMBER_ROLE_PREFIX}BACK_전산관리자`, | ||
INFRA_전산관리자: `${MEMBER_ROLE_PREFIX}INFRA_전산관리자`, | ||
회원: `${MEMBER_ROLE_PREFIX}회원`, | ||
출제자: `${MEMBER_ROLE_PREFIX}출제자`, | ||
} as const; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { Typography } from '@mui/material'; | |
import List from '@mui/material/List'; | ||
import ListItem from '@mui/material/ListItem'; | ||
import { VscSearch } from 'react-icons/vsc'; | ||
import { MEMBER_ROLE } from '@constants/member'; | ||
import muiTheme from '@constants/muiTheme'; | ||
import { roleDutyListInfo, roles } from '@mocks/DutyManageApi'; | ||
|
||
|
@@ -50,13 +51,8 @@ const DutyProfileTooltip = ({ jobName }: DutyProfileTooltipProps) => { | |
const [tooltipOpen, setTooltipOpen] = useState(false); | ||
const [modalOpen, toggleModalOpen] = useReducer((prev) => !prev, false); | ||
|
||
let badgeImage; | ||
if (jobName.search('전산관리자') !== -1) { | ||
badgeImage = roles.find((role) => role.name === 'ROLE_전산관리자')?.img; | ||
} else { | ||
badgeImage = roles.find((role) => role.name === jobName)?.img; | ||
} | ||
|
||
const badgeImage = roles.find((role) => role.name === jobName)?.img; | ||
// NOTE jobName으로는 "ROLE_전산관리자" 내려오지만, roles에는 존재하지 않습니다! 세부적인 전산관리자(프론트, 백, 인프라)만 존재합니다 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오옹 이런 주석 좋은것같슴당 |
||
return ( | ||
<div className="relative z-10"> | ||
<DutyProfileButton | ||
|
@@ -65,7 +61,7 @@ const DutyProfileTooltip = ({ jobName }: DutyProfileTooltipProps) => { | |
setTooltipOpen={setTooltipOpen} | ||
toggleModalOpen={toggleModalOpen} | ||
/> | ||
{jobName !== 'ROLE_전산관리자' ? ( | ||
{jobName !== MEMBER_ROLE.전산관리자 && ( | ||
Comment on lines
-68
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 굿!! |
||
<> | ||
<DescriptionRoleDutyTooltip | ||
title={tooltipContent(jobName)} | ||
|
@@ -85,7 +81,7 @@ const DutyProfileTooltip = ({ jobName }: DutyProfileTooltipProps) => { | |
badgeImage={badgeImage} | ||
/> | ||
</> | ||
) : null} | ||
)} | ||
</div> | ||
); | ||
}; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
MEMBER_ROLE
상수를 선언하는 방식이 3가지인데, 어느게 좋을지 고민이 됩니다..ROLE_회장
과 같이 바로 텍스트로 넣어주기 -> 해당 방식이 간단하지만,ROLE_
부분이 중복되는데 이걸 바로 박아버리기 애매했습니다.백틱
안에 같이 넣어주기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.
2222
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.
저는 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.
1.
ROLE_
이라는 prefix를 백엔드와 공통적으로 사용하고 있는데, 직책을 어떻게 표기할 지에 따라서 충분히 변경 가능한 부분으로 보입니다. 1번으로 가게되면 변경될 경우 수동으로 업데이트 해주어야 하는 게 번거로울 것 같아요!3. 사실 이게 제일 깔끔해보이지는 한데, 단순한 텍스트에 함수 실행문이 들어가는 게 조금 과한 것 같다는 생각도 들더라구요!
그래서 최종적으로 prefix 변수를 사용하지만 템플릿 리터럴로 표현된 2번으로 선택하였습니다! 하지만 prefix로 사용되는
ROLE_
이 크게 변동이 많을 것 같은 부분은 아니라서 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.
저도 2번이 좋아보입니다.
Prefix는 공통이라 추후에 바뀔때 처리도 쉬울듯 하고,
실제 사용하는 입장에서는 1번과 2번 모두 똑같이 보여서
코드쪽 처리가 쉬운 2번이 좋을거 같습니다.
밑에는 참고 이미지 입니다!
1번
2번