[김수민] Sprint5 #176
[김수민] Sprint5 #176
Conversation
[Fix] delete merged branch github action
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
수민님 5주차 미션 하시느라 고생 많이 하셨습니다!
기존에 작업하셨던 코드들도 마이그레이션 하시느라 더 시간이 걸리셨을 것 같은데 여러가지를 고민해보시고 시도해보신 것들이 느껴졌어요!
저도 최대한 자세히 코멘트 드리려고 했으니 도움이 되면 좋겠습니다.
There was a problem hiding this comment.
💊 제안
재사용되는 값들을 객체로 관리하시는 것 좋습니다!
다만 id, type, labelText처럼 변경 가능성이 낮은 값은 사용하는 곳에서 바로 정의하시는 것을 추천드리고 placeholder, errorMessage 처럼 반복 사용되는 문자열 위주로 상수화해두면 코드 중복을 줄이고 유지보수가 더 수월해질 것 같습니다.
There was a problem hiding this comment.
❗️ 수정요청
기존에 작업하신 것들을 React로 변환하신 것 좋습니다. 이 과정에서 구조를 잡으시다보니 해당 파일을 생성하신 것 같아요.
다만 아직 사용되지 않는 빈 파일은 유지보수 측면에서 불필요한 파일로 남을 가능성이 큽니다.
비어 있는 파일이 많아지면 프로젝트가 불필요하게 복잡해지고 나중에 어떤 파일이 실제로 사용되는지 관리하기 어려워질 수 있습니다.
실제로 필요할 때 추가하는 것이 더 좋으니 비어 있는 파일을 삭제하는 것을 추천드립니다~
There was a problem hiding this comment.
💬 여담
React Router를 사용하고 계신데 향후 라우트가 많아지면 Route들을 별도의 컴포넌트로 분리하면 App 컴포넌트가 더 깔끔해지고 라우트 관리도 쉬워집니다.
| import './App.css'; | ||
| import './styles/reset.css'; | ||
| import './styles/layout.css'; | ||
| import './styles/common.css'; |
There was a problem hiding this comment.
💊 제안
현재 App.css, reset.css, layout.css, common.css를 모두 직접 import하는 방식은 파일이 늘어날수록 관리하기 어려울 수 있습니다. 이러한 스타일을 하나의 파일에 모아서 import하는 방식으로 변경하시면 더 깔끔하고, 스타일 파일 추가시 해당 파일만 변경하면 되므로 이러한 방식도 고려해보세요!
| const AuthStateContext = createContext(); | ||
| const AuthDispatchContext = createContext(); | ||
| export { AuthStateContext, AuthDispatchContext }; |
There was a problem hiding this comment.
💊 제안
AuthStateContext와 AuthDispatchContext를 App.jsx에 정의하기보다 별도의 파일로 분리하시는 것을 추천드립니다~
이렇게 코드나 구조를 최대한 명확하게 유지하시는 것이 가독성과 유지보수에 좋습니다!
| import ProductListItem from './ProductListItem'; | ||
| import styles from './styles/ProductList.module.css'; | ||
|
|
||
| const ProductList = ({listType, products = []}) => { |
There was a problem hiding this comment.
💊 제안
재사용성을 높이기 위해 베스트 상품과 전체 상품 목록을 하나의 컴포넌트로 묶으신 것은 좋은 시도입니다~ 다만 내부 디자인이 달라지면서 코드 복잡도가 높아질 수 있으니, 공통 디자인 요소만 별도의 하위 컴포넌트로 분리하고, BestProducts.jsx와 Products.jsx처럼 역할에 따라 컴포넌트를 나누시면 더 좋을 것 같아요!
There was a problem hiding this comment.
💊 제안
Pagination이 더 일반적으로 사용되는 컴포넌트 명이라 해당 이름으로 변경하시는 것을 추천드려요!
There was a problem hiding this comment.
💊 제안
캐러셀과 페이지네이션과 같은 컴포넌트의 경우 prevButton, nextButton 이 특정 조건에서는 disabled 되는 것이 일반적입니다.
이를 추가해보시면 더 좋을 것 같아요.
|
|
||
|
|
||
|
|
||
| const handleOptionClick = (option, apiValue) => { |
There was a problem hiding this comment.
❗️ 수정요청
| const handleOptionClick = (option, apiValue) => { | |
| const handleOptionClick = (option, apiValue) => { |
There was a problem hiding this comment.
💊 제안
화면단과 로직을 분리하신 점 좋습니다! 다만 현재 useProducts 훅 안에 베스트 상품 조회, 전체 상품 조회, 미디어 쿼리, 페이지 네비게이션 등 모든 로직이 한데 모여 있어 파악하기 복잡해 보입니다~ 기능별로 커스텀 훅이나 유틸 함수로 분리하시면 더 좋을 것 같아요.
4 participants
https://dduminipandamarket.netlify.app
요구사항
기본
중고마켓
중고마켓 반응형
심화
주요 변경사항
스크린샷
PC
M
멘토에게