Skip to content

[정새론] 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

Open
wants to merge 16 commits into
base: React-정새론
Choose a base branch
from

Conversation

Squarecat-meow
Copy link
Collaborator

요구사항

기본

  • 중고마켓 페이지 주소는 "/items"입니다.
  • 페이지 주소가 "/items"일때 상단 네비게이션 바의 '중고마켓' 버튼의 색상은 "3692FF"입니다.
  • 상단 네비게이션 바는 이전 미션에서 구현한 랜딩 페이지와 동일한 스타일로 만들어 주세요.
  • 상품 데이터 정보는 https://panda-market-api.vercel.app/docs/#/ 에 명세된 GET 메소드 “/products” 를 사용해주세요.
  • '상품 등록하기' 버튼을 누르면 “/additem” 로 이동합니다. ( 빈 페이지 )
  • 전체 상품에서 드롭 다운으로 “최신 순” 또는 “좋아요 순”을 선택해서 정렬을 할 수 있습니다.

심화

  • 페이지네이션 기능을 구현합니다.

스크린샷

image
스크린샷 2025-05-26 15 33 36
스크린샷 2025-05-26 15 33 58

멘토에게

  • 뷰포트의 너비에 따라 fetch의 query parameter를 바꿀 수 있나요?
  • pagination 기능을 어찌저찌 완성했는데, 로직에 문제가 없는지, 그 밖의 방어코드가 필요할지 리뷰 부탁드리겠습니다.
  • 컴포넌트로 쪼개야 하는 단위는 적절한지, 아니면 더 잘게 쪼개서 로직을 분할해야 하는지 궁금합니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@Squarecat-meow Squarecat-meow added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label May 26, 2025
@addiescode-sj addiescode-sj self-requested a review May 27, 2025 00:17
Copy link
Collaborator

@addiescode-sj addiescode-sj left a 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}`)
Copy link
Collaborator

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 };
Copy link
Collaborator

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 }) {
Copy link
Collaborator

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만 남게 되어 유지보수에 용이하고,
스타일 규칙을 객체로 관리해주니 수정 및 확장에도 용이해지겠죠? :)

Copy link
Collaborator

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}번째 판매 물품 즐겨찾기 아이콘`} />
Copy link
Collaborator

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를 보내주는게 좋을것같아요.

예시를 들어드릴게요!

Copy link
Collaborator

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 }) {
Copy link
Collaborator

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}
/>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 개선하면, 아래와 같은 장점들이 생깁니다:

  • 유연성 향상: 단순히 이미지가 아닌 어떤 React 엘리먼트든 렌더링할 수 있습니다.
  • 컨트롤 위임: 렌더링할 내용을 완전히 부모 컴포넌트에서 제어할 수 있습니다.
  • 재사용성: Input 컴포넌트가 특정 UI 요소(이미지)에 종속되지 않게 됩니다.
  • 테스트 용이성: 렌더링 로직을 분리함으로써 테스트가 더 쉬워집니다.

Comment on lines +36 to +42
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));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

el 값에 따라 분기가 되지 않고있는데, 간단하게 이렇게 고쳐보면 어떨까요?

Suggested change
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을 사용해주시는게 더 깔끔할거예요!

참고

Comment on lines +88 to +98
<div className='best-card-section'>
{bestCardData ? (
<>
{bestCardData.map((el) => (
<Card key={el.id} element={el} />
))}
</>
) : (
<></>
)}
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<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>

Copy link
Collaborator

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 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서도 위에 드린 코멘트 참고해서 코드를 좀 더 간결하면서도 안정적이게 바꿔봅시다!

Comment on lines +14 to +23
const [validate, setValidate] = useState({
email: {
state: false,
message: '',
},
pw: {
state: false,
message: ''
}
});
Copy link
Collaborator

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: "",
  });

Comment on lines +13 to +26
const [validate, setValidate] = useState({
email: {
state: false,
message: '',
},
pw: {
state: false,
message: '',
},
nickname: {
state: false,
message: '',
}
});
Copy link
Collaborator

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: "",
  });

@addiescode-sj
Copy link
Collaborator

질문에 대한 답변

멘토에게

  • 뷰포트의 너비에 따라 fetch의 query parameter를 바꿀 수 있나요?

뷰포트의 너비를 계산 후 fetch할때 관련된 query parameter를 바꿔주면 되겠죠?
이런 로직이 프로그램 내에서 자주 사용된다면 코드 응집도와 재사용성을 위해 커스텀훅으로 분리하시는게 좋고요 :)

  • pagination 기능을 어찌저찌 완성했는데, 로직에 문제가 없는지, 그 밖의 방어코드가 필요할지 리뷰 부탁드리겠습니다.

읽어내려가면서 보이는 코드 퀄리티에는 문제가 없었는데,
더 안정적인 코드 흐름을 위해서 발생할 수 있는 예외사항을 꼼꼼히 처리해주는식으로 개선해보시면 좋을것같아요.

예를 들자면 코드를 더 안정적인 흐름으로 만들기위해 이런 일들을 추가로 해보면 좋겠죠?

  • 페이지 범위 계산할 때 음수 인덱스가 발생할 경우 관련해서 처리해주기
  • currentPage와 totalPage의 유효성 검사, callback 함수의 존재 여부를 확인
  • 컴포넌트로 쪼개야 하는 단위는 적절한지, 아니면 더 잘게 쪼개서 로직을 분할해야 하는지 궁금합니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

코드리뷰 채널에서 공유드린, 리액트에서 자주 쓰는 설계 방법을 깊게 공부해보시면 점점 좋아질거예요!
참고

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants