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.

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

Merged
merged 30 commits into from
Feb 23, 2024

Conversation

LJW25
Copy link
Collaborator

@LJW25 LJW25 commented Feb 3, 2024

📄 Summary

관리자� 멤버 관리 페이지, �카테고리 관리 페이지, 환율 관리 페이지와 각 페이지별 추가 & 수정 모달 구현

🙋🏻 More

다음은 인증/인가 🙃

- 관리자 멤버 관리 페이지 (/admin/member)

image

- 관리자 추가 모달

image

- 관리자 비밀번호 수정 모달

image

- 카테고리 관리 페이지 (/admin/category)

image

- 카테고리 추가 및 수정 모달

image image

- 환율 관리 페이지 (/admin/category)

image

환율 추가 및 수정 모달

image image

@LJW25 LJW25 self-assigned this Feb 3, 2024
@LJW25 LJW25 added ✨ Feature FE-Admin 행록 어드민 프론트엔드 labels Feb 3, 2024
@LJW25 LJW25 linked an issue Feb 3, 2024 that may be closed by this pull request
5 tasks
@ashleysyheo
Copy link
Collaborator

😮🙌👍

Copy link
Member

@dladncks1217 dladncks1217 left a 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 보내둘테니 코멘트 남긴것들이랑 경로 관련한 것들만 확인해주시죠 크크

멋집니다멋집니다👍👍

Comment on lines 1 to 5
import { axiosInstance } from '../axiosInstance';

import type { PassowrdPatchData } from '@/types/adminMember';

import { END_POINTS } from '@/constants/api';
Copy link
Member

Choose a reason for hiding this comment

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

컨벤션 있는데 그거 확인해보고 맞춰주면 좋을거같아용
노션에있습니다
이거 말고도 전체적으로 확인해주시면 좋을듯??

Copy link
Collaborator Author

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 });
Copy link
Member

Choose a reason for hiding this comment

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

useUpdatePasswordForm으로 바꾸는거 어떤가여 훅인데 앞자리 대문자 좀 이상하네요
마치 UseState 같은느낌

Copy link
Collaborator

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();
Copy link
Member

Choose a reason for hiding this comment

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

밑에서는 updateInputValue같이 썼는데 여긴 대문자인 이유가 있나요!?

Copy link
Collaborator Author

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(
Copy link
Member

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번 등장해서, 파일 여기저기 돌면서 코드 확인하는데 흐름쫒기가 쉽지 않더라구요🥲

Copy link
Collaborator

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 폴더 내부에 둔다.

Copy link
Collaborator Author

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 구조로 변경해보려는데 어떨까요?!

Copy link
Member

Choose a reason for hiding this comment

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

너무좋은데요!?

Comment on lines 63 to 65
// const checkCurrencyValidity = (currencyInformation: CurrencyFormData) => {
// return currencyKeys.some((key) => isInvalidCurrency(Number(currencyInformation[key])));
// };
Copy link
Member

Choose a reason for hiding this comment

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

주석이 남아있어용

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

으어억 이스터에그 발견! 당첨되셨습니다!

Copy link
Member

Choose a reason for hiding this comment

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

오 좋은데요??

Comment on lines 100 to 103
// if (checkCurrencyValidity(currencyInformation)) {
// setIsCurrencyError(true);
// return;
// }
Copy link
Member

Choose a reason for hiding this comment

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

여기도여

Comment on lines 5 to 10
export const containerStyling = () => {
return css({
width: '80vw',
padding: `${Theme.spacer.spacing6} ${Theme.spacer.spacing6} ${Theme.spacer.spacing0} ${Theme.spacer.spacing6}`,
});
};
Copy link
Member

Choose a reason for hiding this comment

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

꼭 함수여야 할 이유도 없는 것 같기도 하고...
함수로 다 통일하려고 쓰는거라면 return문이 필요할까 싶기도 하구요??
()=>css({ ~~~ 와 같이 사용할 수 있어요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ed14a01

파라미터 필요없는 것들은 일반 변수로 수정했습니다!

Comment on lines 25 to 27
const [isUsernameError, setIsUsernameError] = useState(false);
const [isPasswordError, setIsPasswordError] = useState(false);
const [isConfirmPasswordError, setIsConfirmPasswordError] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

각 에러에 대해 개별적으로 상태를 정의하는 대신, 위에 adminMemberInformation처럼 객체로 선언해도 괜찮을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

15a47cf

전체적으로 반영 완! 좋은 의견 감사합니당

Comment on lines 43 to 72
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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

위 코멘트처럼 에러를 객체로 관리한다면 에러 관련 처리 함수도 추상화 할 수 있을 것 같아여
updateInputValue 함수처럼요!

Comment on lines 74 to 78
{
username: adminMemberInformation.username,
adminType: adminMemberInformation.adminType,
password: adminMemberInformation.password,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{
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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

gcTime을 추가한 이유는 뭔가요??

Copy link
Collaborator Author

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(
Copy link
Collaborator

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 폴더 내부에 둔다.

Comment on lines 14 to 36
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>
</>
);
};
Copy link
Collaborator

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>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ebb0bb3
애슐리 말대로 고치니까 *pageSkeleton 자체가 필요없더라구요! 싹 수정했습니다 👍

Comment on lines 10 to 15
return input > 180 || input < -180;
};

export const isInvalidCategoryId = (input: number) => {
return input > 999 || input < 100;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 숫자들이 뭐를 의미하는지 잘 모르겠어요 상수화 해두면 좋을 것 같아여!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0c5be44

상수화 완! 사실 대부분 이미 상수로 선언은 해놓고 validator에만 적용을 안해놨더라구요..? 수정했습니다..ㅎㅎ

@jjongwa
Copy link
Member

jjongwa commented Feb 4, 2024

풀스택 개발자 이오의 도전을 응원합니다

@LJW25 LJW25 merged commit 0619d0f into develop Feb 23, 2024
1 check passed
hgo641 pushed a commit that referenced this pull request Apr 23, 2024
* 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 객체로 관리
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE-Admin 행록 어드민 프론트엔드 ✨ Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

관리자 페이지 구현
4 participants