-
Notifications
You must be signed in to change notification settings - Fork 42
React 이유진 sprint5 #188
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-\uC774\uC720\uC9C4-sprint5"
React 이유진 sprint5 #188
Conversation
[Fix] delete merged branch github action
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.
수고하셨습니다!
전체적으로 코드 어렵지않게 잘 작성하시고, 퀄리티도 좋네요.
리팩토링하면서 로직과 UI의 분리를 신경써서 하면 어떤 장점이 있는지 코멘트 참고해보시고, 우선적으로 개선해보시면 좋을것같아요!
주요 리뷰 포인트
- 로직과 UI 결합도 낮추기
- 유지보수를 고려한 개발
- 네이밍
@@ -0,0 +1,10 @@ | |||
import axios from "axios"; | |||
|
|||
const BASE_URL = 'https://panda-market-api.vercel.app'; |
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.
base url의 경우 외부 노출을 막기 위해 보통 환경 변수로 관리한답니다! 아래 아티클 참고해보시고 환경 변수로 바꿔주세요 :)
import styles from "../pages/ItemsPage.module.css"; | ||
import Pagination from "./Pagination"; | ||
|
||
function AllItemsSectoin({ bp }) { |
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.
bp와 같은 지나친 축약은 코드를 이해하기 어렵게 만듭니다.
breakpoint
와 같이 풀네임으로 바꿔보세요!
const pageSizeMap = { mobile: 4, tablet: 6, desktop: 10 }; | ||
const pageSize = pageSizeMap[bp]; |
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.
pageSizeMap의 경우 렌더링 이후 재계산 될 필요가 없으니 컴포넌트 바깥으로 빼두시고, bp와 같은 props 값에 따른 계산이 필요한 pageSize 정도만 내부에 남기시면 좋겠죠? :)
import { useState, useEffect } from "react"; | ||
import fetchItems from "../../api/itemApi"; | ||
|
||
function useItem({ |
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.
네이밍이 조금 명확하지않은것같아요.
useRequest, useAsync와 같이 비동기 데이터 fetching을 처리하는 의미로 바꿔줄까요?
page = 1, | ||
pageSize = 10, | ||
orderBy = "recent", | ||
name = "", |
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.
해당 요청에 대한 params가 항상 이 형식으로 고정되어있는게 아니니까, 형태를 이렇게 바꿔보는게 실수를 줄이면서도 유연하게 사용될 것 같습니다.
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.
import { useEffect, useState } from "react";
import fetchItems from "../../api/itemApi";
function useItem(params = {}) {
const [items, setItems] = useState([]);
const [totalCount, setTotalCount] = useState(0);
const [loading, setLoading] = useState(false);
const [error, setError] = useState(null);
useEffect(() => {
const loadItems = async () => {
setLoading(true);
setError(null);
try {
const data = await fetchItems(params);
setItems(data.list);
setTotalCount(data.totalCount);
} catch (err) {
console.error("상품 데이터 불러오기 실패:", err);
setError(err);
} finally {
setLoading(false);
}
};
loadItems();
}, [params]);
return { items, totalCount, loading, error };
}
export default useItem;
|
||
|
||
|
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.
공백은 한칸이면 충분합니다!
function getLinkStyle({ isActive}) { | ||
return{ | ||
color: isActive ? 'var(--blue)' : 'var(--secondary-color-gray600)' | ||
} | ||
} |
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.
굳굳! NavLink의 isActive를 잘 활용하셨네요 👍
다만 해당 함수가 Navbar 컴포넌트 바깥에 있어서 리액트 렌더링 주기에 맞춰서 사용될 수 없어요. 컴포넌트 안쪽으로 옮기거나, 인라인으로 작성해볼까요?
아래와 같이, 조건부 스타일링에 따른 클래스이름 조합이 복잡하거나, 보다 일관적인 방식으로 관리하기위해 도움 받을 수 있는 라이브러리를 쓰셔도 좋습니다 :)
const blockSize = 5; | ||
const totalPages = Math.ceil(totalCount / pageSize); | ||
const currentBlock = Math.ceil(page / blockSize); | ||
const startPage = (currentBlock - 1) * blockSize + 1; | ||
const endPage = Math.min(startPage + blockSize - 1, totalPages); | ||
|
||
const handlePrevBlock = () => { | ||
const prevBlockPage = Math.max(startPage - blockSize, 1); | ||
onPageChange(prevBlockPage); | ||
}; | ||
|
||
const handleNextBlock = () => { | ||
const nextBlockPage = Math.min(startPage + blockSize, totalPages); | ||
onPageChange(nextBlockPage); | ||
}; |
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.
페이지네이션 관련 로직과 UI의 결합도를 낮춰볼까요?
다른 분 PR에서 드린 코멘트인데, 참고해보시고 이렇게 결합도를 낮춰주면 어떤 장점이 생기고, 어떤 절차로 리팩토링 시도할수있는지 알아가시면 좋을것같아요! :)
질문에 대한 답변
아뇨, 전혀 문제될 것 없습니다. 오히려 커스텀 훅으로 빼두지않았다면 화면이 바뀌어 현재 화면의 사이즈를 알아내야하는 로직이 필요한 컴포넌트에서 코드를 반복 작성하게 되니까 좋지 않겠죠? :)
우선 현재 앱은 SPA이기때문에 SEO 점수를 최적화하지못한다는 단점이 있습니다.
CRA로 앱을 시작하기위해 필요한 파일들은 전부 제거해주시는게 좋습니다.
항상 고정된 방식의 워크플로를 고수하기보다는 상황에 맞춰 유연하게 작업하는 태도를 가지시면 좋습니다.
규칙이나 사례가 있다기보다는, 본문 내에서 피드백 드린것처럼 단일 책임 원칙에 따라 컴포넌트가 항상 명확한 책임을 가지도록하고, 상태 업데이트 로직이 분산되어있거나 복잡하게 꼬이지 않도록 신경쓰고, 공용으로 쓰이는 훅이나 컴포넌트, 유틸 함수의 경우 유지보수와 변경에 용이하도록 구조화하는등 너무 다양해서 지속적으로 미션 제출하면서 여러가지 시도를 해보시면 좋을것같아요! 그렇군요! 실제 코드와 개발자도구를 사용해 실행되는 환경에서 로그를 찍어본다던지 network탭에서 요청이 어떻게 들어가고있는지 분석해보면서 실제로 어떤 문제가 발생하고있는지 디버깅해보시면 됩니다. 관련한 코드 베이스를 파악해보는것부터 시작해봐요!
그다음 실제 실행 환경에서 개발자도구 > Network 탭에서 요청과 응답 본문을 확인해보세요! |
요구사항
기본
중고마켓
Desktop : 4개 보이기
Tablet : 2개 보이기
Mobile : 1개 보이기
Desktop : 12개 보이기
Tablet : 6개 보이기
Mobile : 4개 보이기
심화
주요 변경사항
추가 변경 예정
스크린샷
배포
배포사이트
멘토에게
useBreakpoint hook 사용 관련
반응형 구현을 위해 useBreakpoint라는 hook을 만들어 사용했습니다.
이런 방식이 현업에서도 자주 쓰이는지, 아니면 문제가 있어 권장되지 않는 방식인지 궁금합니다.
커스텀 드롭다운 vs. 기본
select
정렬 변경 버튼을 만들면서 반응형을 위해
select
대신button
과ul
로 커스텀 드롭다운을 구현했습니다.이 방식이 적절한지, 아니면 접근성이나 유지보수 측면에서 기본
select
사용이 더 좋은지 궁금합니다.CRA 기반 파일 정리 관련
프로젝트 폴더에 Create React App(CRA) 생성 당시의 파일들이 남아있습니다 (logo.svg, serviceWorker 등).
단순히 삭제해도 괜찮은지, 아니면 주의할 점이 있는지 알고 싶습니다.
코드 작성 흐름 조언
처음 아무것도 없는 상태에서 프로젝트를 시작하려니 막막함을 많이 느꼈습니다.
앞으로는 챗지피티 도움 없이 더 스스로 해보고 싶은데,
로직을 먼저 짜고 대략적인 UI를 붙이고 구체화하는 흐름이 맞는지,
혹은 더 좋은 작성 흐름이 있는지 조언을 부탁드립니다.
컴포넌트 분리·추상화 관련
아직 컴포넌트 분리와 추상화가 익숙하지 않습니다.
이 부분에서 연습할 때 도움이 될 만한 작은 팁이나 참고할 사례가 있으면 알려주시면 감사하겠습니다.
페이지 로드될 때 API 요청에서 pageSize 값이 제가 의도한 대로 넘어가지 않는 것 같습니다.

문제를 더 파악해보겠지만, 혹시 조언 주실 수 있다면 부탁드립니다.