-
Notifications
You must be signed in to change notification settings - Fork 6
코드리뷰용 PR #312
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
base: code-review
Are you sure you want to change the base?
코드리뷰용 PR #312
Conversation
…n-182 [Feat] 로딩 페이지 및 스켈레톤 mixin 추가
…-modal-187 [Feat] 로그인, 구독 모달 추가
[Feat] number type 문자로 포맷
[Feat] 구독 api & 확인 모달 적용
…mobile-199 [Design] 랜딩 페이지 반응형 스타일 적용
…-203 [Design] footer 반응형 디자인 적용
…page-247 [Feat] 관리자 전략 페이지 완성
[Feat] 분석 탭에 데이터 없을 경우 ui 통일하여 추가
…n-check-304 [Feat] 로그인 아닐경우, 전략목록에서 리스트 클릭시 로그인체크 모달 띄우기
[Feat] 전략등록 페이지 퍼블리싱 및 API 연결
…age-162 [Fix] admin category에서 Suspense가 적용되지 않던 부분과 axiosInstance를 사용하지 않던 곳들을 찾아 교정
…-306 [Feat] % 수치 소수점 두자리까지 보여지도록 적용
…age-162 [Fix] build error
[Feat] API 작업 진행 상황 공유
dozukwang
left a comment
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.
깔끔하게 잘 작업해 주었으며 세세한 기능들도 많이 챙겨 작업해 주셨습니다.
전체적인 구성도 잘 잡았고 화면 구현도 거의 대부분 완성해주셔서 좋았습니다. 규모도, 필요했던 기술스택도 더 많고 복잡한 프로젝트로 쉽지 않았을 것으로 보입니다.
수고 많으셨습니다👏👏
코드리뷰는 동작확인이 가능한 코드를 중점으로 진행했습니다. 미완성 됐거나 실제 사용형태를 확인하기 어려운 코드는 눈에 띄는 부분에 대해서만 의견을 남겼습니다.
공통적으로는 아래 부분들을 추가로 확인해보면 좋을 것 같습니다.
- 실제 동작과 기능구현 측면에서는 미완성된 부분이 다수 존재합니다.
- 화면상의 구성은 갖춰줬기 때문에 동작이 되지 않는 경우 오류인지, 추가 개발이 필요한 부분인지 파악하기 어려운 부분이 있었습니다.
- 추가 개발이 필요한 경우라면 (onClick인 경우라면) 클릭 시 토스트창 또는 Alert 등으로 “기능 준비중입니다.” 라는 간단한 안내를 준다면 추후 기능 테스트를 진행할 때 팀원이나 사용자에게 빠른 동작 이해를 줄 수 있을 것 같습니다.
- 반응형 대응 페이지로 제작하지 않았으나
flex, length%를 통해 유동적인 대응을 함께 진행했지만min-width설정이 없어 메인 콘텐츠 확인이 어려운 부분이 있습니다.- 모바일 대응 아닌 PC 전용 페이지를 고려하고 있다면 콘텐츠의 크기를 고려한 예외처리를 추가하고, 가로 스크롤을 통해 전체적인 내용을 확인할 수 있게 한다면 콘텐츠의 안정적인 이용이 개선될 것 같습니다.
- 전체적으로 오류 대응 예외처리가 존재하지 않습니다.
- 서버의 동작 자체가 원활하지 않아 500번 오류가 나기도 하고, 조금씩 동작하지 않거나 실패하는 경우 등이 존재합니다. 500번 오류가 발생하는 경우에는 페이지 자체가 날라가는 경우가 많았스비다.
- 오류에 대한 처리를 확인해보면
console으로 자체적인 확인만 진행되고 있는 것으로 보이는데, 최소 동작 성공에 대한 안내는 없어도alert등으로 오류가 발생했으니 잠시 후 시도해주세요 같은 안내가 필요합니다. - 사용자는 개발자가 아니기 때문에 디버깅을 통한 오류대응을 진행할 수 없기 때문에
onError, catch등을 통한 적절한 대응까지가 동작의 완성이라고 생각하면 좋을 것 같습니다.
- 많은 데이터를 두었을 때 동작이 원활한지, 어떤 개선점이 있는지 등을 찾아보고 실제로 리팩토링을 하지는 않더라도 어떻게 개선하면 좋을지, 문제점은 무엇일지 등을 고민해보시면 좋을 것 같습니다.
- 여태까지 프로젝트보다 규모가 큰 만큼 확인할 것과 고민할 것이 많이 있기 때문에 살펴보시면 좋을 것 같습니다.
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 interface ImageDataModel { | ||
| id: number | ||
| imageUrl: string | ||
| title: string | ||
| } |
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으로 외부에서도 사용하는 타입이라면
shared > types으로 관리위치를 변경해도 좋을 것 같습니다. - 컴포넌트 고유의 타입만 내부에서 관리하는 방법으로 진행하고 있는 것으로 보이기 때문에 관리방법이 섞이지 않도록 유지해주는 것이 좋습니다.
| } | ||
| } | ||
|
|
||
| if (!data || !Array.isArray(data.content) || isLoading) return null |
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 (!Array.isArray(data?.content) || isLoading) return null으로 축약 가능합니다.- 옵셔널 체이닝을 통해 안전한 탐색을 하고 조건을 더 명확하게 보여줄 수 있습니다.
| </Button> | ||
| </div> | ||
| )} | ||
| {croppedImagesData && croppedImagesData.length !== 0 ? ( |
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.
- 마찬가지로 옵셔널 체이닝으로 연결하면 최종 컴포넌트 출력조건만 표시하고, 예외문 길이도 줄일 수 있습니다.
- 또
length !== 0의 조건은undefined에도 해당될 수 있으니length > 0으로 출력조건 범위를 더 세부적으로 줄이는 편이 좋을 것 같습니다.
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 ( | ||
| <Button | ||
| variant="filled" | ||
| onClick={() => mutate()} |
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 hasFooter = !pathname.includes('/signin') && !pathname.includes('/signup') | ||
| return ( | ||
| <> | ||
| <LogoHeader hasText={true} hasLinks={true} isLoggedIn={isAuthenticated ? true : 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.
isAuthenticated자체가boolean데이터기 때문에? true : false문은 불필요한 예외처리입니다.
|
|
||
| import { SIGN_UP_COOKIE, SignUpCookieValueType } from '../_constants/cookies' | ||
|
|
||
| const COOKIE_EXPIRATION_MINUTES = 30 |
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.
- cookie 관련하여 사용하고 있는 constants가 있으니 위치를 옮겨서 같이 관리해도 좋을 것 같습니다.
| export const getUserTypeCookie = () => { | ||
| const userType = getSignupCookie(SIGN_UP_COOKIE.USER_TYPE) | ||
| return userType ? (userType as UserType) : undefined | ||
| } | ||
|
|
||
| export const getIsAgreedTermsCookie = () => { | ||
| const isAgreedTerms = getSignupCookie(SIGN_UP_COOKIE.IS_AGREED_TERMS) | ||
| return isAgreedTerms === 'Y' | ||
| } | ||
|
|
||
| export const getNicknameCookie = () => { | ||
| const nickname = getSignupCookie(SIGN_UP_COOKIE.NICKNAME) | ||
| return nickname | ||
| } |
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.
/**
*
* @param target "userType" | "isAgreedTerms" | "nickname"
* - userType = 관리자 또는 일반사용자 권한 반환
* - isAggreedTerms = 동의여부 반환(boolean)
* - nickname = 닉네임 반환
*/
export const getInfoFromCookie = (target: "userType" | "isAgreedTerms" | "nickname") => {
let targetInfo = undefined
switch (target) {
case "userType":
targetInfo = getSignupCookie(SIGN_UP_COOKIE.USER_TYPE) as UserType
return targetInfo ? (targetInfo as UserType) : undefined
case "isAgreedTerms":
targetInfo = getSignupCookie(SIGN_UP_COOKIE.USER_TYPE)
return targetInfo === 'Y'
case "nickname":
targetInfo = getSignupCookie(SIGN_UP_COOKIE.USER_TYPE)
return targetInfo
}
}- 쿠키의
getter함수를 동일하게 사용하는 형태라면 하나의 함수를 두고 원하는 데이터를 입력 > 반환하게 해도 좋을 것 같습니다. - 이 경우 파라미터와 함수설명을 통해 쿠키를 통해 어떤 데이터를 다룰 수 있는지 한 눈에 확인할 수 있는 장점이 있습니다.
- 아래
setter용 함수들도 비슷하게 통합 가능합니다.
| useAuthStore.getState().setAuthState({ | ||
| isAuthenticated: false, | ||
| user: null, | ||
| }) |
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.
- 해당 코드와 26~29번째 줄은 동일하게 권한 등이 없는 유저의 현재 정보를 제거시키는 작업으로 동일합니다. = 로그아웃 처리
- 초기화 작업을 함수로 두고 호출하여, 명확하게 어떤 동작을 진행하는지 안내하면 좋을 것 같습니다.
const handleLogOut = () => {
const { setAuthState } = useAuthStore.getState();
// 사용자 정보 초기화
setAuthState({
isAuthenticated: false,
user: null,
});
// 기타 추가처리가 필요하다면 이곳에 추가하여 진행
};
...
if (isTokenExpired(accessToken)) {
handleLogOut()
return config
}
🚀 풀 리퀘스트 제안
🔍 작업 내용
🔧 변경 사항
📸 스크린샷 (권장)
🙏 리뷰 참고 (선택 사항)
📄 기타 (선택 사항)