-
Notifications
You must be signed in to change notification settings - Fork 42
[정새론] sprint5 #175
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: React-정새론
Are you sure you want to change the base?
The head ref may contain hidden characters: "React-\uC815\uC0C8\uB860-sprint5"
[정새론] sprint5 #175
Conversation
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.
수고하셨습니다!
전체적으로 코드 깔끔하게 잘 짜시네요 ㅎㅎ
주요 리뷰 포인트
- 쿼리 스트링 조합 시 URLSearchParams 객체 이용하기
- 스타일링 관련 props 없애고 variant 기반으로 바꿔주기
- svg 컴포넌트화해서 사용자에게 피드백 주기
- render delegation 패턴의 장점 이해하기
- response 상태에 따른 분기 처리 더 안정적이고 깔끔하게 작성하기
- 객체 형태의 상태 업데이트 사용 시 주의점
*/ | ||
export default async function fetchLists(quantity, page, orderBy) { | ||
try { | ||
const res = await fetch(`${process.env.REACT_APP_BASE_URL}/products?page=${page}&pageSize=${quantity}&orderBy=${orderBy}`) |
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.
URLSearchParams 객체 사용해서 여러개의 파라미터를 관리해보는 방법으로 바꾸면, 쿼리 스트링의 파싱, 조작, 인코딩 등의 작업을 간편하게 처리할 수 있으면서도 실수를 줄일 수 있겠죠? :)
아래 아티클 참고해보세요!
참고
try { | ||
const res = await fetch(`${process.env.REACT_APP_BASE_URL}/products?page=${page}&pageSize=${quantity}&orderBy=${orderBy}`) | ||
const data = await res.json(); | ||
return { totalCount: data.totalCount, list: data.list }; |
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.
리턴값의 경우도 response가 수정되거나 확장될 가능성을 생각해 따로 객체 만들지말고 그대로 리턴해줍시다 :)
import { Link } from 'react-router-dom'; | ||
import './button.css'; | ||
|
||
function Button({ children, disabled, link, radius, size }) { |
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.
지금 구조로 봤을때, Button은 요구사항 변경에 따라 관리해야하는 props 개수를 점점 늘리게될것같아요.
스타일링 관련 props를 variant 패턴으로 리팩토링하면 어떨까요?
이런식으로 variant 객체를 컴포넌트 외부에 추가하면, Button의 스타일과 관련된 정보를 한곳에서 관리할 수 있어 수정 및 확장에 용이해요.
const VARIANTS = {
small: {
width: ...,
borderRadius: ...,
},
medium: {
width: 486,
borderRadius: 16,
},
disabled: {
...
}
};
그리고 단순 스타일링과 관련된 props를 제거해주고, 의미가 명확한 props만 유지하도록 바꿔보면:
function Button ({ children, disabled, link }) => {
const variantStyle = VARIANTS[variant] || VARIANTS.medium;
...
}
미리 정의된 디자인 규칙 안에서만 사용하므로 디자인 시스템의 일관성을 유지할 수 있게되고,
의미가 명확한 props만 남게 되어 유지보수에 용이하고,
스타일 규칙을 객체로 관리해주니 수정 및 확장에도 용이해지겠죠? :)
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.
왜 props의 증가가 안좋은지는, 아래 아티클 참고해보시면 좋을것같아요 :)
<h2>{element.name}</h2> | ||
<h3>{element.price.toLocaleString()}원</h3> | ||
<div className='card-favorite-container'> | ||
<img src={Heart} alt={`${element.id}번째 판매 물품 즐겨찾기 아이콘`} /> |
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.
해당 즐겨찾기 아이콘 클릭하면 사용자에게 피드백을 주기위해 색이 채워지거나 border 두께가 두꺼워지거나 색깔이 진해지지않을까요? ㅎㅎ 이런 인터렉션을 구현하려면 png형식보다는 svg로 export해서, svg를 컴포넌트화하여 props를 보내주는게 좋을것같아요.
예시를 들어드릴게요!
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.
- HeartIcon 컴포넌트 만들기 (예시)
import React from 'react';
function HeartIcon({ isActive = false }) {
return (
<svg
width="24"
height="24"
viewBox="0 0 24 24"
fill={isActive ? "#FF0000" : "none"}
stroke={isActive ? "#FF0000" : "#000000"}
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
>
<path d="M20.84 4.61a5.5 5.5 0 0 0-7.78 0L12 5.67l-1.06-1.06a5.5 5.5 0 0 0-7.78 7.78l1.06 1.06L12 21.23l7.78-7.78 1.06-1.06a5.5 5.5 0 0 0 0-7.78z" />
</svg>
);
}
export default HeartIcon;
- 사용할때
<HeartIcon isActive={element.isFavorite} />
@@ -0,0 +1,19 @@ | |||
import './input.css'; | |||
|
|||
function Input({ slot, slotDirection, slotAlt, onChange, onBlur, type, name, placeholder }) { |
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.
해당 컴포넌트를 설계 의도에 맞게 더 유연하게 사용할수있도록 개선해볼까요?
현재의 코드는 slot prop을 통해 이미지를 받고있지만 단순히 props를 통해 데이터만 전달하고있고, 내부적으로 조건부 렌더링을 수행하고 있습니다. 따라서 render delegation 패턴을 제대로 활용하려면 자식 컴포넌트의 렌더링 로직을 부모 컴포넌트에 위임하고 children을 통해 렌더링될 내용을 전달하는 방식으로 개선하는것이 유연한 방식입니다.
이렇게 컴포넌트간 역할을 정의해볼까요?
- 렌더링할 내용을 children prop으로 받아 더 유연하게 사용하기
- 부모 컴포넌트가 어떤 내용을 렌더링할지 결정
- Input 컴포넌트는 구조와 스타일링만 담당
import "./input.css";
function Input({
renderSlot,
slotDirection = "right",
onChange,
onBlur,
type,
name,
placeholder,
}) {
return (
<div className="input-container">
{slotDirection === "left" && renderSlot && (
<div className="slot-left">{renderSlot()}</div>
)}
<input
className={slotDirection === "left" ? "input-slanted" : "input"}
onChange={onChange}
type={type}
name={name}
onBlur={onBlur}
placeholder={placeholder}
/>
{slotDirection === "right" && renderSlot && (
<button className="input-button">{renderSlot()}</button>
)}
</div>
);
}
export default Input;
- 실제 사용
<Input
renderSlot={() => (
<img
src={Visibility}
alt='비밀번호 보이기 토글 버튼'
/>
)}
onChange={handleChange}
type='password'
name='pw'
onBlur={handleBlur}
/>
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 엘리먼트든 렌더링할 수 있습니다.
- 컨트롤 위임: 렌더링할 내용을 완전히 부모 컴포넌트에서 제어할 수 있습니다.
- 재사용성: Input 컴포넌트가 특정 UI 요소(이미지)에 종속되지 않게 됩니다.
- 테스트 용이성: 렌더링 로직을 분리함으로써 테스트가 더 쉬워집니다.
if (el === 'favorite') { | ||
setSort('favorite'); | ||
fetchLists(ITEMS_PER_PAGE, 1, el).then((res) => setCardData(res.list)); | ||
} else if (el === 'recent') { | ||
setSort('recent'); | ||
fetchLists(ITEMS_PER_PAGE, 1, el).then((res) => setCardData(res.list)); | ||
} |
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.
el 값에 따라 분기가 되지 않고있는데, 간단하게 이렇게 고쳐보면 어떨까요?
if (el === 'favorite') { | |
setSort('favorite'); | |
fetchLists(ITEMS_PER_PAGE, 1, el).then((res) => setCardData(res.list)); | |
} else if (el === 'recent') { | |
setSort('recent'); | |
fetchLists(ITEMS_PER_PAGE, 1, el).then((res) => setCardData(res.list)); | |
} | |
setSort(el); | |
fetchLists(ITEMS_PER_PAGE, 1, el).then((res) => setCardData(res.list)); |
만약 여기서 해당 로직 실행을 걸러줘야하는 조건 분기가 생긴다면, early return을 사용해주시는게 더 깔끔할거예요!
<div className='best-card-section'> | ||
{bestCardData ? ( | ||
<> | ||
{bestCardData.map((el) => ( | ||
<Card key={el.id} element={el} /> | ||
))} | ||
</> | ||
) : ( | ||
<></> | ||
)} | ||
</div> |
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.
<div className='best-card-section'> | |
{bestCardData ? ( | |
<> | |
{bestCardData.map((el) => ( | |
<Card key={el.id} element={el} /> | |
))} | |
</> | |
) : ( | |
<></> | |
)} | |
</div> | |
<div className="best-card-section"> | |
{bestCardData && | |
bestCardData.map((el) => <Card key={el.id} element={el} />)} | |
</div> |
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.
bestCardData &&
보다는 bestCardData.length !== 0 &&
이런식으로 좀 더 직접적으로 비교해주셔도되고,
더 깔끔하게는 optional chaining을 활용해 bestCardData?.map
으로 쓰시는 방법도 있어요 :)
</div> | ||
</div> | ||
<div className='all-card-section'> | ||
{cardData ? ( |
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 [validate, setValidate] = useState({ | ||
email: { | ||
state: false, | ||
message: '', | ||
}, | ||
pw: { | ||
state: false, | ||
message: '' | ||
} | ||
}); |
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.
현재 구조에서는 validate 상태가 하나의 객체로 관리되고 있어서,
비밀번호나 이메일 중 하나만 업데이트되어도 전체 validate 객체가 새로운 레퍼런스로 업데이트되어 불필요한 리렌더링이 발생할 수 있습니다.
이 문제를 개선해보려면 아래 예시와 같이 각각의 유효성 검사 상태를 분리하여 관리하는 것이 좋겠죠? :)
const [emailValidate, setEmailValidate] = useState({
state: false,
message: "",
});
const [pwValidate, setPwValidate] = useState({
state: false,
message: "",
});
const [validate, setValidate] = useState({ | ||
email: { | ||
state: false, | ||
message: '', | ||
}, | ||
pw: { | ||
state: false, | ||
message: '', | ||
}, | ||
nickname: { | ||
state: false, | ||
message: '', | ||
} | ||
}); |
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.
여기에서도 login 페이지와 같은 문제가 발생할수있으니 따로 분리해봅시다!
const [emailValidate, setEmailValidate] = useState({
state: false,
message: "",
});
const [nicknameValidate, setNicknameValidate] = useState({
state: false,
message: "",
});
const [pwValidate, setPwValidate] = useState({
state: false,
message: "",
});
질문에 대한 답변
뷰포트의 너비를 계산 후 fetch할때 관련된 query parameter를 바꿔주면 되겠죠?
읽어내려가면서 보이는 코드 퀄리티에는 문제가 없었는데, 예를 들자면 코드를 더 안정적인 흐름으로 만들기위해 이런 일들을 추가로 해보면 좋겠죠?
코드리뷰 채널에서 공유드린, 리액트에서 자주 쓰는 설계 방법을 깊게 공부해보시면 점점 좋아질거예요! |
요구사항
기본
심화
스크린샷
멘토에게