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.
깔끔하게 잘 작업해 주었으며 세세한 기능들도 많이 챙겨 작업해 주셨습니다.
전체적인 구성도 잘 잡았고 화면 구현도 거의 대부분 완성해주셔서 좋았습니다. 규모도, 필요했던 기술스택도 더 많고 복잡한 프로젝트로 쉽지 않았을 것으로 보입니다.
수고 많으셨습니다👏👏
코드리뷰는 동작확인이 가능한 코드를 중점으로 진행했습니다. 미완성 됐거나 실제 사용형태를 확인하기 어려운 코드는 눈에 띄는 부분에 대해서만 의견을 남겼습니다.
공통적으로는 아래 부분들을 추가로 확인해보면 좋을 것 같습니다.
- 실제 동작과 기능구현 측면에서는 미완성된 부분이 다수 존재합니다.
- 화면상의 구성은 갖춰줬기 때문에 동작이 되지 않는 경우 오류인지, 추가 개발이 필요한 부분인지 파악하기 어려운 부분이 있었습니다.
- 추가 개발이 필요한 경우라면 (onClick인 경우라면) 클릭 시 토스트창 또는 Alert 등으로 “기능 준비중입니다.” 라는 간단한 안내를 준다면 추후 기능 테스트를 진행할 때 팀원이나 사용자에게 빠른 동작 이해를 줄 수 있을 것 같습니다.
- 반응형 대응 페이지로 제작하지 않았으나
flex, length%를 통해 유동적인 대응을 함께 진행했지만min-width설정이 없어 메인 콘텐츠 확인이 어려운 부분이 있습니다.- 모바일 대응 아닌 PC 전용 페이지를 고려하고 있다면 콘텐츠의 크기를 고려한 예외처리를 추가하고, 가로 스크롤을 통해 전체적인 내용을 확인할 수 있게 한다면 콘텐츠의 안정적인 이용이 개선될 것 같습니다.
- 전체적으로 오류 대응 예외처리가 존재하지 않습니다.
- 서버의 동작 자체가 원활하지 않아 500번 오류가 나기도 하고, 조금씩 동작하지 않거나 실패하는 경우 등이 존재합니다. 500번 오류가 발생하는 경우에는 페이지 자체가 날라가는 경우가 많았스비다.
- 오류에 대한 처리를 확인해보면
console으로 자체적인 확인만 진행되고 있는 것으로 보이는데, 최소 동작 성공에 대한 안내는 없어도alert등으로 오류가 발생했으니 잠시 후 시도해주세요 같은 안내가 필요합니다. - 사용자는 개발자가 아니기 때문에 디버깅을 통한 오류대응을 진행할 수 없기 때문에
onError, catch등을 통한 적절한 대응까지가 동작의 완성이라고 생각하면 좋을 것 같습니다.
- 많은 데이터를 두었을 때 동작이 원활한지, 어떤 개선점이 있는지 등을 찾아보고 실제로 리팩토링을 하지는 않더라도 어떻게 개선하면 좋을지, 문제점은 무엇일지 등을 고민해보시면 좋을 것 같습니다.
- 여태까지 프로젝트보다 규모가 큰 만큼 확인할 것과 고민할 것이 많이 있기 때문에 살펴보시면 좋을 것 같습니다.
There was a problem hiding this comment.
변수명도 특성에 맞춰 다양하게 제어해줬고, 파일명이나 이벤트 핸들러 제어까지 진행해주셨네요.
코드 일관성과 협업 규칙을 지키기 위한 작업을 고민하신게 느껴집니다.
실제로 개발할 땐 어떠셨나요? 좋았던 것이나 생각보다 불편했던 규칙 등이 있다면 어떻게 개선하면 좋을지 고민해보시면 좋을 것 같습니다.
| export interface ImageDataModel { | ||
| id: number | ||
| imageUrl: string | ||
| title: string | ||
| } |
There was a problem hiding this comment.
- export으로 외부에서도 사용하는 타입이라면
shared > types으로 관리위치를 변경해도 좋을 것 같습니다. - 컴포넌트 고유의 타입만 내부에서 관리하는 방법으로 진행하고 있는 것으로 보이기 때문에 관리방법이 섞이지 않도록 유지해주는 것이 좋습니다.
| } | ||
| } | ||
|
|
||
| if (!data || !Array.isArray(data.content) || isLoading) return null |
There was a problem hiding this comment.
if (!Array.isArray(data?.content) || isLoading) return null으로 축약 가능합니다.- 옵셔널 체이닝을 통해 안전한 탐색을 하고 조건을 더 명확하게 보여줄 수 있습니다.
| </Button> | ||
| </div> | ||
| )} | ||
| {croppedImagesData && croppedImagesData.length !== 0 ? ( |
There was a problem hiding this comment.
- 마찬가지로 옵셔널 체이닝으로 연결하면 최종 컴포넌트 출력조건만 표시하고, 예외문 길이도 줄일 수 있습니다.
- 또
length !== 0의 조건은undefined에도 해당될 수 있으니length > 0으로 출력조건 범위를 더 세부적으로 줄이는 편이 좋을 것 같습니다.
| return ( | ||
| <Button | ||
| variant="filled" | ||
| onClick={() => mutate()} |
There was a problem hiding this comment.
- 탈퇴진행 전 안정장치가 필요합니다! 다른 민감동작 보다도 특히 회원탈퇴는 이용자와 서비스 제공자 모두에게 중요한 가치손실로 이어질 수 있기 때문에 컨펌모달을 확실한 재의사 검증 단계를 추가해야 합니다.
| 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.
isAuthenticated자체가boolean데이터기 때문에? true : false문은 불필요한 예외처리입니다.
|
|
||
| import { SIGN_UP_COOKIE, SignUpCookieValueType } from '../_constants/cookies' | ||
|
|
||
| const COOKIE_EXPIRATION_MINUTES = 30 |
There was a problem hiding this comment.
- 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.
/**
*
* @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.
- 해당 코드와 26~29번째 줄은 동일하게 권한 등이 없는 유저의 현재 정보를 제거시키는 작업으로 동일합니다. = 로그아웃 처리
- 초기화 작업을 함수로 두고 호출하여, 명확하게 어떤 동작을 진행하는지 안내하면 좋을 것 같습니다.
const handleLogOut = () => {
const { setAuthState } = useAuthStore.getState();
// 사용자 정보 초기화
setAuthState({
isAuthenticated: false,
user: null,
});
// 기타 추가처리가 필요하다면 이곳에 추가하여 진행
};
...
if (isTokenExpired(accessToken)) {
handleLogOut()
return config
}
🚀 풀 리퀘스트 제안
🔍 작업 내용
🔧 변경 사항
📸 스크린샷 (권장)
🙏 리뷰 참고 (선택 사항)
📄 기타 (선택 사항)