-
Notifications
You must be signed in to change notification settings - Fork 16
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
관리자 페이지 구현 - 관리자멤버, 카테고리, 환율 관리 페이지 구현 #790
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.
"프론트엔드" 가 되어버린 리더 이오의 코드 잘 봤습니다
사실 file changes가 86개라서 빠르게 훑는 느낌으로 보긴 했는데, 전체적으로 import관련한 부분을 수정해야 할 게 많이 보이더라구요!
alias 적용했으니 이왕이면 쓰는게 좋겠죠!?
'@/constants/api'
같은 경로들은 tsconfig 확인해보고 @constants/api
처럼 수정하는게 좋을 것 같아요.
근데 생각해보면 vite.config.js파일에서도 추가해줘야하려나??
제대로 적용되어있는것들 잘 돌아가는거보면 문제 없어보이기도 하고...
제가 프로젝트 세팅한게 아니다보니 좀 더 봐야 알겠네요 ㅋㅋㅋ
그나저나 이제와서 co-location으로 구조 변경하기에는 좀 늦었겠죠?
리뷰하려는데 파일이 여기저기 막 돌아다니다보니 읽기가 쉽지 않군요😅
일단 request changes 보내둘테니 코멘트 남긴것들이랑 경로 관련한 것들만 확인해주시죠 크크
멋집니다멋집니다👍👍
import { axiosInstance } from '../axiosInstance'; | ||
|
||
import type { PassowrdPatchData } from '@/types/adminMember'; | ||
|
||
import { END_POINTS } from '@/constants/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.
@trivago/prettier-plugin-sort-imports 라이브러리로 자동정렬 적용했습니다. 이게 그냥 알아서 적용되는게 아니더군요?
**Note: There may be an issue with some package managers, such as pnpm. You can solve it by providing additional configuration option in prettier config file.
공식문서에서 이렇다는데 하라는대로 따라하니까 자동정렬 따라란 성공! 순서는 노션 참고했슴다
disableConfirmPasswordError, | ||
updateInputValue, | ||
handleSubmit, | ||
} = UseUpdatePasswordForm({ adminMemberId: adminMemberId, onSuccess: onClose }); |
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.
useUpdatePasswordForm
으로 바꾸는거 어떤가여 훅인데 앞자리 대문자 좀 이상하네요
마치 UseState
같은느낌
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.
일반적으로 js에서 함수명은 camelCase를 사용해요
타입, 클래스, 컴포넌트는 PascalCase
onSuccess, | ||
onError, | ||
}: UseUpdatePasswordFormParams) => { | ||
const UpdatePasswordMutaion = useUpdateAdminMemberPasswordMutation(); |
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.
밑에서는 updateInputValue
같이 썼는데 여긴 대문자인 이유가 있나요!?
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 [isEngNameError, setIsEngNameError] = useState(false); | ||
const [isKorNameError, setIsKorNameError] = useState(false); | ||
|
||
const updateInputValue = useCallback( |
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.
updateInputValue
가 여기저기 퍼져있군요?
뭔가 하고 보니, 기존에 있던 데이터중 일부만 수정하는 코드 같더라구요. 도메인에따라 네이밍을 조금 수정하는게 어떨까요?
디렉토리 구조가 co-location이 아닌데 PR 하나에 updateInputValue
가 86번 등장해서, 파일 여기저기 돌면서 코드 확인하는데 흐름쫒기가 쉽지 않더라구요🥲
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.
co-location으로 구조 변경하는 것도 나쁘지 않을 것 같아요!
공통적으로 사용되는 hook은 최상위로 빼고 관련된 것들은 ex. city 폴더 내부에 둔다.
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.
확실히 저도 개발하면서 co-location 구조를 썼으면 더 편했겠다는 생각이 많이 들었던것 같아요! 처음이라 기존 행록 구조를 따라하면서 익숙해지려 했는데, 이제 구조를 좀 익혔으니 더 늦기전에 구조를 바꿔볼까 합니다.
이번 PR에서 바꾸면 여차했을 때 롤백이 어려울것같아 기능상 문제가 없는 상태에서 머지하고 바로 새 PR파서 co-location 구조로 변경해보려는데 어떨까요?!
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 checkCurrencyValidity = (currencyInformation: CurrencyFormData) => { | ||
// return currencyKeys.some((key) => isInvalidCurrency(Number(currencyInformation[key]))); | ||
// }; |
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.
으어억 이스터에그 발견! 당첨되셨습니다!
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 (checkCurrencyValidity(currencyInformation)) { | ||
// setIsCurrencyError(true); | ||
// 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.
여기도여
export const containerStyling = () => { | ||
return css({ | ||
width: '80vw', | ||
padding: `${Theme.spacer.spacing6} ${Theme.spacer.spacing6} ${Theme.spacer.spacing0} ${Theme.spacer.spacing6}`, | ||
}); | ||
}; |
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.
꼭 함수여야 할 이유도 없는 것 같기도 하고...
함수로 다 통일하려고 쓰는거라면 return문이 필요할까 싶기도 하구요??
()=>css({ ~~~
와 같이 사용할 수 있어요!
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 [isUsernameError, setIsUsernameError] = useState(false); | ||
const [isPasswordError, setIsPasswordError] = useState(false); | ||
const [isConfirmPasswordError, setIsConfirmPasswordError] = useState(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.
각 에러에 대해 개별적으로 상태를 정의하는 대신, 위에 adminMemberInformation처럼 객체로 선언해도 괜찮을 것 같아요!
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 disableUsernameError = useCallback(() => { | ||
setIsUsernameError(false); | ||
}, []); | ||
|
||
const disablePasswordError = useCallback(() => { | ||
setIsPasswordError(false); | ||
}, []); | ||
|
||
const disableConfirmPasswordError = useCallback(() => { | ||
setIsConfirmPasswordError(false); | ||
}, []); | ||
|
||
const handleSubmit = (event: FormEvent<HTMLFormElement>) => { | ||
event.preventDefault(); | ||
|
||
if (isEmptyString(adminMemberInformation.username.trim())) { | ||
setIsUsernameError(true); | ||
return; | ||
} | ||
|
||
if (!isValidPassword(adminMemberInformation.password.trim())) { | ||
setIsPasswordError(true); | ||
return; | ||
} | ||
|
||
if (adminMemberInformation.password != adminMemberInformation.confirmPassword) { | ||
setIsConfirmPasswordError(true); | ||
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.
위 코멘트처럼 에러를 객체로 관리한다면 에러 관련 처리 함수도 추상화 할 수 있을 것 같아여
updateInputValue
함수처럼요!
{ | ||
username: adminMemberInformation.username, | ||
adminType: adminMemberInformation.adminType, | ||
password: adminMemberInformation.password, | ||
}, |
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.
{ | |
username: adminMemberInformation.username, | |
adminType: adminMemberInformation.adminType, | |
password: adminMemberInformation.password, | |
}, | |
{ | |
...adminMemberInformation | |
}, |
const { data: adminMemberData } = useSuspenseQuery<AdminMemberData[], AxiosError>({ | ||
queryKey: ['adminMember'], | ||
queryFn: getAdminMember, | ||
gcTime: 24 * 60 * 60 * 60 * 1000, |
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.
gcTime을 추가한 이유는 뭔가요??
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.
gcTime이 캐시가 삭제되지 않게 유지하는 시간으로 알고있는데 맞을까요?!
관리자 데이터의 경우 수정이 빈번하게 일어나는 데이터는 아니라고 생각해서 넣었습니다!
const [isEngNameError, setIsEngNameError] = useState(false); | ||
const [isKorNameError, setIsKorNameError] = useState(false); | ||
|
||
const updateInputValue = useCallback( |
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.
co-location으로 구조 변경하는 것도 나쁘지 않을 것 같아요!
공통적으로 사용되는 hook은 최상위로 빼고 관련된 것들은 ex. city 폴더 내부에 둔다.
const AdminMemberPageSkeleton = () => { | ||
return ( | ||
<> | ||
<Flex> | ||
<SidebarNavigation /> | ||
<Flex styles={{ direction: 'column', align: 'center' }} css={containerStyling}> | ||
<Heading size="large" css={titleStyling}> | ||
관리자 멤버 관리 | ||
</Heading> | ||
<Button variant="primary" css={addButtonStyling}> | ||
추가하기 | ||
</Button> | ||
<section css={tableStyling}> | ||
<AdminMemberTableSkeleton length={10} /> | ||
</section> | ||
<div css={pagenationSkeletonStyling}> | ||
<Skeleton /> | ||
</div> | ||
</Flex> | ||
</Flex> | ||
</> | ||
); | ||
}; |
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.
여기서 관리자 멤버 관리
, 추가하기
코드 같은 경우에는 AdminMemberPage
에 있는 코드를 그대로 가져오고 있는데, 이렇게 반복 하는 대신 Suspense
를 감싸는 범위를 좁혀봐도 좋을 것 같아요!
<Suspense fallback={<FallbackComponent />}>
<section css={tableStyling}>
<AdminMemberTable adminMembers={currentPageData} />
</section>
<PageNavigation
pages={pageIndexDatas}
selected={page}
maxPage={lastPageIndex}
onChangeNavigate={handleSetPage}
/>
</Suspense>
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.
ebb0bb3
애슐리 말대로 고치니까 *pageSkeleton 자체가 필요없더라구요! 싹 수정했습니다 👍
return input > 180 || input < -180; | ||
}; | ||
|
||
export const isInvalidCategoryId = (input: number) => { | ||
return input > 999 || input < 100; | ||
}; |
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.
상수화 완! 사실 대부분 이미 상수로 선언은 해놓고 validator에만 적용을 안해놨더라구요..? 수정했습니다..ㅎㅎ
풀스택 개발자 이오의 도전을 응원합니다 |
* feat: mock 데이터 추가 * feat: msw api 구현 * feat: category api 구현 * feat: currency api 구현 * feat: adminMember api 구현 * feat: CityPageSkeleton 구현 * feat: 카테고리 조회 페이지 구현 * feat: 카테고리 추가 모달 구현 * feat: 카테고리 수정 기능 구현 * feat: 카테고리 스켈레톤 페이지 구현 * feat: 환율 api hook 구현 * feat: 관리자 멤버 api hook 구현 * feat: 환율 조회 페이지 구현 * feat: 환율 정보 추가 모달 구현 * reafator: 컴포넌트 이름 변경 * feat: 환율 정보 수정 버튼 구현 * feat: 환율 skeleton 페이지 구현 * feat: 관리자 멤버 조회 페이지 구현 * feat: 관리자 추가 모달 구현 * feat: 비밀번호 수정 모달 구현 * refactor: style 수정 * feat: 관리자멤버 스켈레톤 페이지 구현 * refactor: import path 변경 * refactor: import 순서 리팩토링 * refactor: camelCase 적용 * refactor: pageSkeleton 제거 * refactor: style 선언 수정 * refactor: import 수정 * refactor: 매직넘버 상수화 * refactor: error 객체로 관리
📄 Summary
🙋🏻 More
- 관리자 멤버 관리 페이지 (/admin/member)
- 관리자 추가 모달
- 관리자 비밀번호 수정 모달
- 카테고리 관리 페이지 (/admin/category)
- 카테고리 추가 및 수정 모달
- 환율 관리 페이지 (/admin/category)
환율 추가 및 수정 모달