-
Notifications
You must be signed in to change notification settings - Fork 39
[김성주] sprint5 #169
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
[김성주] sprint5 #169
The head ref may contain hidden characters: "React-\uAE40\uC131\uC8FC-sprint5"
Conversation
-login,signUp 나중에 추가하기
- 컴포넌트 및 css 모듈 나누기 - 공통로직 빼기 -> responsivePageSize.js, updateProductsByQuery.js - ProductsAllContext 추가 - 클래스 네이밍 변경 - 등등
-result.list 이상한 값 set되는 거 수정
+api 호출 쪽 에러 throw 확실하게
addiescode-sj
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.
수고하셨습니다!
첫 리액트 미션이다보니 변경사항이 너무 많아서 리뷰하는데 조금 힘들었지만, 열심히 해주셔서 보람있었어요 :) useValidate 훅 구조 관련해서 코멘트 드린거 먼저 봐주시고, 전체적으로 코드 스타일과 재사용성을 고려해 개선하는 방법 위주로 피드백 드렸으니 참고해서 리팩토링 진행해보시면 좋을것같아요 :)
주요 리뷰 포인트
- 불필요한 리렌더링 방지 및 코드 복잡도 개선 (useValidate 훅)
- 코드 스타일 관련 피드백
- 유지보수를 고려한 개발
- 디렉토리 구조
src/App.jsx
Outdated
| return ( | ||
| <BrowserRouter> | ||
| <Routes> | ||
| <Route element={<Header></Header>}> |
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.
| <Route element={<Header></Header>}> | |
| <Route element={<Header />}> |
Self closing으로 쓰셔도 됩니다!
src/components/Header/Header.jsx
Outdated
| import HeaderAuth from "./HeaderAuth"; | ||
|
|
||
| function Header() { | ||
| const [isLogined, setIsLogined] = useState(sessionStorage.getItem("logined")); |
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 [isLogined, setIsLogined] = useState(sessionStorage.getItem("logined")); | |
| const [isLoggedIn, setIsLoggedIn] = useState(sessionStorage.getItem("loggedIn")); |
문법상 isLoggedIn이 맞습니다.
간단한 단어 교정이나 오타 검증은 아래 extension 도움을 받아보세요! :)
src/context/ProductAllContext.jsx
Outdated
| @@ -0,0 +1,30 @@ | |||
| import { createContext, useState } from "react"; | |||
|
|
|||
| export const ProductAllContext = createContext(); | |||
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.
네이밍이 조금 모호하네요. api 요청에 의한 response data라면 ProductData정도가 적당할것같습니다 :)
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.
해당 context는 src 바로 밑에 있어서, 일반적으로 단일 페이지가 아닌 여러 페이지에서 공유되는 공용 목적으로 쓰입니다.
만약 특정 페이지에서만 쓰이는 context라면 사용하는 입장에서 찾기 쉽게 폴더 및 파일을 이동하시는게 좋을것같아요.
아래 아티클 참고해보시고, 성주님이 쓰기 좋은 방식으로 개선해보세요!
참고
src/hooks/useResizeInnerWidth.js
Outdated
| useEffect(() => { | ||
| if (innerWidth === null) return; | ||
|
|
||
| const newPageSize = reCalculatePageSize(type, innerWidth) | ||
| setPageSize(newPageSize) | ||
| }, [innerWidth]); |
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.
type이 의존성 배열에 포함되어있지않아 type이 변경되어도 pageSize가 재계산되지 않을 수 있다는 문제가 생길 수 있어요. 아마 eslint 사용하시면 기본 규칙에 포함되어있어서 관련해서 워닝이 표시될텐데 사용해보시는걸 추천드립니다! :)
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.
구조상, 이 커스텀훅을 사용하는 모든 컴포넌트에서 입력 필드가 변경될때마다 상태가 바뀌고, 해당 훅을 사용하는 모든 컴포넌트가 리렌더링되는 문제가 발생해요.
이 훅을 유지하면서도(구조를 크게 바꾸지않으면서도) 문제를 개선하기 위해서는, 각 입력 필드의 유효성 검사 결과를 선택적으로(필요할때만) 구독할 수 있도록 구현해주는게 좋을것같아요.
useReducer 훅을 사용해 개별적인 상태 업데이트 로직을 관리하고, 항상 모든 입력필드의 state 변경을 감지하지않고 특정 state가 바뀌지 않는 한 같은 결과를 재사용할수있도록 메모이제이션 해주면 어떨까요?
예시를 작성해드릴게요 :)
- useReducer를 사용해 상태 업데이트
const initialState = {
values: {},
errors: {},
errorMessages: {},
isValid: {},
};
function validationReducer(state, action) {
switch (action.type) {
case "SET_VALUE": {
const { name, value } = action.payload;
const validator = validRuleObj[name];
const isValid = validator.isValid(value, state.values["user-password"]);
const errorMessage = isValid ? "" : validator.getErrorMessage(value);
return {
...state,
values: { ...state.values, [name]: value },
errors: { ...state.errors, [name]: !isValid },
errorMessages: { ...state.errorMessages, [name]: errorMessage },
isValid: { ...state.isValid, [name]: isValid },
};
}
default:
return state;
}
}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.
그 다음 useValidate 훅에서 해당 reducer를 사용해 상태 업데이트를 관리하면서도, 특정 state가 바뀌지 않는 한 같은 결과를 재사용할수있도록 메모이제이션해주면:
export function useValidate() {
const [state, dispatch] = useReducer(validationReducer, initialState);
const setValue = useCallback((name, value) => {
dispatch({ type: "SET_VALUE", payload: { name, value } });
}, []);
const getFieldState = useCallback(
(name) => {
return {
value: state.values[name] || "",
error: state.errors[name] || false,
errorMessage: state.errorMessages[name] || "",
isValid: state.isValid[name] || false,
};
},
[state]
);
return {
setValue,
getFieldState,
};
}이런 장점이 생겨요.
- useReducer를 사용하여 상태 업데이트 로직을 더 예측 가능하게 (간소화) 만들 수 있습니다.
- 각 입력 필드의 상태를 선택적으로 구독할 수 있어 불필요한 리렌더링을 방지합니다.
- 사용하는 입장에서도, getFieldState 함수를 통해 각 필드의 상태를 쉽게 가져올 수 있습니다.
- 컴포넌트에서 필드별 유효성 검사를 수행할때마다 훅 인스턴스를 생성하지않아도되고 (복잡도를 떨어트리는것또한 준수한 코드 퀄리티를 유지하기위해 중요), 하나의 훅 실행으로 모든 상태를 관리하게 만들 수 있습니다.
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 validation = useValidate();
const emailState = validation.getFieldState("user-email");
// 입력 필드에서
<input
value={emailState.value}
onChange={(e) => validation.setValue("user-email", e.target.value)}
/>이런식으로 useValidate 훅을 한번만 실행하고, 상태 업데이트를 원하는 입력 필드만 선택적으로 연결해 사용하는게 가능해지죠? :)
src/pages/Auth/Login.jsx
Outdated
| const toItemsNavigation = useNavigate(); | ||
| const emailInput = useValidate(); | ||
| const passwordInput = useValidate(); |
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.
위에 useValidate 훅 구조 개선 관련해 코멘트 참고해보시고, 이런식으로 여러개의 훅 인스턴스를 생성하지않도록 해봅시다! :)
src/pages/Items/Items.jsx
Outdated
| <ProductsFavorite></ProductsFavorite> | ||
| <ProductAllContextProvider> | ||
| <ProductsAll></ProductsAll> | ||
| </ProductAllContextProvider> |
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.
| <ProductsFavorite></ProductsFavorite> | |
| <ProductAllContextProvider> | |
| <ProductsAll></ProductsAll> | |
| </ProductAllContextProvider> | |
| <ProductsFavorite /> | |
| <ProductAllContextProvider> | |
| <ProductsAll /> | |
| </ProductAllContextProvider> |
src/pages/Items/ProductsAll.jsx
Outdated
| <section className={styles.items__all}> | ||
| <div className={styles[`items__all-filter`]}> | ||
| <h2>전체 상품</h2> | ||
| <ProductsFilterBar></ProductsFilterBar> |
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.
요렇게 많이 쓰시네요 내부에 children이 없으면 <ProductsFilterBar /> 이렇게 쓰시는게 간편합니다!
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.
굳굳! 재사용 가능한 로직(페이지네이션 기능과 관련된 로직)은 커스텀훅으로 따로 또 분리해주면 재사용성을 극대화할수있어요 :)
나중에 새로운 형태의 Pagination UI가 추가될때 이 Pagination 컴포넌트에 props로 추가해 조건부 렌더링을 하기보다는 새 Pagination 컴포넌트를 만들어서 커스텀 훅을 사용해주면 용이하겠죠?
혹은, 기존 Pagination 컴포넌트에 합성 패턴을 사용해 UI도 재사용 가능한 단위로 분리하고 조합해 사용해주는 방법도 있고요 :)
-useReduce -> 로직 간소화, 불필요한 인스턴스생성 제거 -useCallback, React.memo -> 불필요한 인풋 리렌더링 방지 -Header -> logined x loggedIn으로 이름 수정
-useResizeInnerWidht 훅 내부 useEffect deps에 type 추가
-ProductsAllContextProvider -> ProductDataProvider로 이름 변경 -context폴더 삭제 -> ProductDataProvider폴더 pages/Items폴더 내부에 위치
https://panda-sprint5.netlify.app/
요구사항
기본
중고마켓 페이지 주소는 “/items” 입니다.
페이지 주소가 “/items” 일때 상단네비게이션바의 '중고마켓' 버튼의 색상은 “3692FF”입니다.
상단 네비게이션 바는 이전 미션에서 구현한 랜딩 페이지와 동일한 스타일로 만들어 주세요.
상품 데이터 정보는 https://panda-market-api.vercel.app/docs/#/ 에 명세된 GET 메소드 “/products” 를 사용해주세요.
'상품 등록하기' 버튼을 누르면 “/additem” 로 이동합니다. ( 빈 페이지 )
전체 상품에서 드롭 다운으로 “최신 순” 또는 “좋아요 순”을 선택해서 정렬을 할 수 있습니다.
중고마켓 반응형
베스트 상품
Desktop : 4개 보이기
Tablet : 2개 보이기
Mobile : 1개 보이기
전체 상품
Desktop : 10개 보이기
Tablet : 6개 보이기
Mobile : 4개 보이기
심화
주요 변경사항
스크린샷
header
PC
태블릿
모바일
정렬
페이지네이션
멘토에게
최대한 보기 좋게 다듬고 싶었는데 프롭으로 내려주는 게 많아지니까 너무 보기 힘들게 된 것 같아요.
로직도 최대한 안 꼬이게 하고 싶었는데 서로 영향 주는 걸 풀려고 해도 생각한 만큼 잘 되지 않았습니다.
보기 힘드실 것 같아서 죄송해요 ㅠㅠㅠㅠㅠ
셀프 코드 리뷰를 통해 질문 이어가겠습니다.