-
Notifications
You must be signed in to change notification settings - Fork 31
[김수연] sprint11 #178
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
[김수연] sprint11 #178
The head ref may contain hidden characters: "Next-\uAE40\uC218\uC5F0"
Conversation
dongqui
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.
수연님 이번 미션도 고생 많으셨습니다 :) 👍 👍
프로젝트도 화이팅입니다!!
질문 주신 부분은 리뷰 확인해 주세요 :)
| const [isLoginState, setIsLoginState] = useState(false) | ||
| const router = useRouter() | ||
| const { mutate: signin, isPending } = useSigninMutation() | ||
| const [signinFormError, setSigninFormError] = 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.
로그인 / 홈페이지 react-hook-form이용했는데 왠지 더 복잡한 느낌이 있습니다., 이게 맞을까요?
->
음.. 커밋이 덜 푸쉬된 것일까요..!? 현재 코드는 react-hook-form 안 쓰고 있습니다~! 🤣
https://react-hook-form.com/
| setEmailError(emailValidation) | ||
| setPasswordError(passwordValidation) | ||
| // 필드별 유효성 검사 함수 | ||
| function validateField(field: keyof SigninForm, value: 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.
react-hook-form을 쓰셨을 때 제거 될 수 있는 부분들입니다~!
| } | ||
| } | ||
| // 로그인 버튼 활성화 상태 관리 | ||
| useEffect(() => { |
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.
이부분도 유효성 검사에 대한 상태를 react-hook-form이 제공해주기 때문에 제거될 거 같네요..!
나중에 다시 한 번 적용해 보시죠~! :)
| } | ||
|
|
||
| const ProductListItems = () => { | ||
| const [itemsDisplay, setItemsDisplay] = useState(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.
` 왜 두번 불러오는걸까요? 한번만 불러오려면 어떻게 해야할까요?
->
itemsDisplay 초기 값이 1일 때 요청 한 번,
그리고 렌더링 후에 아래 useEffect에서 handleResize 실행되고 setItemsDisplay되면서 요청이 한 번 더 가고 있네요~!
초기값이 1이 아니라 windowSize에 맞춰 정해주고 handleResize은 윈도우 사이즈가 변할때만 실행하게 하면 될 거 같습니다 :)
| ))} | ||
| </div> | ||
| </div> | ||
| <div className={styles['pagination']}> |
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.
현재 검색한 단어에따라 totalcount가 달라져서 페이지네이션 디자인도 바뀝니다. 디자인을 바꾸지 않고 무조건 1~5까지 보이게 할 수 있지만 쿠팡 들어가서 보니 실제로 검색량에 따라 밑 페이지네이션 버튼도 이에 맞게 바뀌는 것 같더라고요. 근데 이 페이지 버튼을 검색량에 따라 바뀌게 하는게 유저 입장에서 뭔가 불편하지 않을까 라는 생각을 합니다. 제 생각이 틀릴 수도 있는데 혹시 실무에서도 쿠팡처럼 검색량에 따라 밑 페이지네이션도 동적으로 바뀌는 것을 자주 이용하나요? 아니면 또 다르게 처리하는 방법이 있나요?
-> 음.. 검색량에 따라 바뀌게 하는 게 자연스럽지 않을까요?
만약 2페이지 가량의 검색량이 있는데 5까지 보여주면 유저는 검색량이 5페이지가 있다고 인식하게 될 거에요! 이런 경우 3~5페이지를 누르면 어떻게 처리를 할 지 애매할듯 합니다 🤔
| name: string | ||
| } | ||
|
|
||
| const ProductListItems = () => { |
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.
+) items 페이지를 리펙토링 했는데 혹시 더 고쳐야 할 부분이나 아쉬운 부분이 있을까요?
-> 기능적으로는 충분히 잘 동작하고 코드도 나쁘지 않습니다. 여기서 만약 더 발전 시킨다면 컴포넌트, 커스텀 훅을 통해 좀 더 추상화 시켜볼 수 있을 거 같아요.
좋은 코드를 쓴다는 것은 우선 쉽게 파악이 가능하고 쉽게 수정이 되는 코드죠!
따라서 같은 관심사를 가진 코드끼리 모아두고, 읽기 쉽도록 적절하게 함수나 컴포넌트, 훅으로 추상화 합니다.
에를 들면,
공통으로 사용되는 상태 값만 상단에 두고,
정렬, 검색 관련된 코드 따로,
productListAll 관련된 코드 따로,
페이지네이션 관련된 코드 따로 모아 추상화를 한다면 아래와 같이 작성이 가능할 거에요.
type SelectOption = {
value: string
name: string
}
const selectList: SelectOption[] = [
{ value: 'recent', name: '최신순' },
{ value: 'favorite', name: '좋아요순' },
]
const ProductListItems = () => {
const itemsDisplay = useItemsDisplay()
const [selectedOption, setSelectedOption] = useState(selectList[0].value)
const [currentPage, setCurrentPage] = useState(1)
const [searchTerm, setSearchTerm] = useState('')
const [debouncedSearchTerm] = useDebounce(searchTerm, 300)
// 상품리스트 가져오는 useQuery 훅
const { productListAll } = useItemsList({
page: currentPage,
itemsDisplay,
orderBy: selectedOption,
keyword: debouncedSearchTerm.trim(),
})
return (
<>
<ProductItemListFilter
searchTerm={searchTerm}
setSearchTerm={setSearchTerm}
selectedOption={selectedOption}
setSelectedOption={setSelectedOption}
/>
<ProductItemList productListAll={productListAll} />
<Pagenation
totalCount={productListAll.data?.totalCount}
currentPage={currentPage}
setCurrentPage={setCurrentPage}
/>
</>
)
}이렇게 정리하면, 원하는 코드를 쉽게 파악하고 찾아갈 수 있습니다 :)
여기서 좀 더 나아가면,
currentPage, selectedOption 등을 상태 값으로 관리하는 것이 아니라 쿼리 스트링으로 관리하는 방법도 있습니다. 쿼리스트링으로 관리하면 useSearchParams를 통해 컴포넌트간 상태 공유가 가능하게 되고 유저 경험도 좋아질 수 있습니다. 예를 들면 지금은 새로 고침을 하거나 뒤로가기르 누르면 페이지, 검색어 등이 모두 날라가게 되죠. 이럴 경우 링크를 공유하는 것도 어려울 거에요.
혹은 상태 관리 라이브러리를 사용할 수도 있겠죠.
쿼리스트링 혹은 상태관리 라이브러리로 상태를 관리하면 상태 값을 굳이 상단이 두지 않아도 되고, 아래와 같이 정리할 수 있습니다.
const ProductListItems = () => {
return (
<>
<ProductItemListFilter />
<ProductItemList />
<Pagenation />
</>
)
} 각각의 필요한 로직을 컴포넌트 내부에 정의하는 거죠!
이렇게 까지 추상화를 하는 것이 물론 늘 정답은 아닙니다.
요점은 코드의 관심사를 분리하는 것은 분명 클린 코드에 도움이 되는 경우가 많습니다! ㅎ
| }) | ||
|
|
||
| // 페이지네이션 관련 변수 설정 | ||
| const totalPages = Math.ceil(productListAll.data?.totalCount / itemsPerPage) |
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.
위 말씀드린 관점으로 본다면
특히 이부분 - totalPages, pages- 은 페이지네이션 컴포넌트로 따로 들어가는 것이 맞겠죠..! 단순히 페이지 네이션의 UI 구성만 하는 함수들이니까요 🤔 다른 로직들과 섞일 이유가 없습니다!
| /^(?=.*[A-Za-z])(?=.*\d)(?=.*[!@#$%^&*])[A-Za-z\d!@#$%^&*]{8,}$/, | ||
| '비밀번호는 영문, 숫자, 특수문자를 포함해야 합니다.' | ||
| ), | ||
| passwordConfirmation: z |
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.
zod refine 기능을 사용해 보세요~! :)
요구사항
기본
유효한 정보를 입력하고 스웨거 명세된 “/auth/signUp”으로 POST 요청해서 성공 응답을 받으면 회원가입이 완료됩니다.
회원가입이 완료되면 “/login”로 이동합니다.
회원가입 페이지에 접근시 로컬 스토리지에 accessToken이 있는 경우 ‘/’ 페이지로 이동합니다.
회원가입을 성공한 정보를 입력하고 스웨거 명세된 “/auth/signIp”으로 POST 요청을 하면 로그인이 완료됩니다.
로그인이 완료되면 로컬 스토리지에 accessToken을 저장하고 “/” 로 이동합니다.
로그인/회원가입 페이지에 접근시 로컬 스토리지에 accessToken이 있는 경우 ‘/’ 페이지로 이동합니다
로컬 스토리지에 accessToken이 있는 경우 상단바 ‘로그인’ 버튼이 판다 이미지로 바뀝니다.
심화
주요 변경사항
스크린샷
멘토에게
<문제 1>

` 왜 두번 불러오는걸까요? 한번만 불러오려면 어떻게 해야할까요?
<문제 2>
현재 검색한 단어에따라 totalcount가 달라져서 페이지네이션 디자인도 바뀝니다. 디자인을 바꾸지 않고 무조건 1~5까지 보이게 할 수 있지만 쿠팡 들어가서 보니 실제로 검색량에 따라 밑 페이지네이션 버튼도 이에 맞게 바뀌는 것 같더라고요. 근데 이 페이지 버튼을 검색량에 따라 바뀌게 하는게 유저 입장에서 뭔가 불편하지 않을까 라는 생각을 합니다. 제 생각이 틀릴 수도 있는데 혹시 실무에서도 쿠팡처럼 검색량에 따라 밑 페이지네이션도 동적으로 바뀌는 것을 자주 이용하나요? 아니면 또 다르게 처리하는 방법이 있나요?

로그인 / 홈페이지 react-hook-form이용했는데 왠지 더 복잡한 느낌이 있습니다., 이게 맞을까요?
+) items 페이지를 리펙토링 했는데 혹시 더 고쳐야 할 부분이나 아쉬운 부분이 있을까요?
(제가 보기엔 뭔가 아쉬운데.. 그 부분이 뭔지 잘 모르겠습니다)