-
Notifications
You must be signed in to change notification settings - Fork 37
[윤혜림] Sprint7 #258
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
[윤혜림] Sprint7 #258
The head ref may contain hidden characters: "React-\uC724\uD61C\uB9BC-sprint7"
Conversation
1005hoon
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.
혜림님! 항상 코드리뷰 내용들을 잘 반영해주시는게 눈에 띄네요
초반 코드리뷰때보다 실력이 많이 좋아지고 있는게 확 보입니다!
그리고 되게 사소한거 디테일을 엄청 잘 잡아주시네요 ㅎㅎ 이것도 정말 혜림님만이 지니고있는 역량이라고 생각해요.
이제 조금만 더 욕심내어본다면, 기능 구현이 커질수록 역할과 책임이 무너지는 부분들이 계속 생기는데요, 이런 부분들 조금 차근차근 해나가보면 어떨까요? 물론 쉽진않겠지만 혜림님은 이 과정이 이제 얼마 남지 않았다는 생각이 드네요 ㅎㅎ
고생 많으셨습니다!
| "react-responsive": "^10.0.0", | ||
| "react-router-dom": "^6.28.0", | ||
| "react-scripts": "5.0.1", | ||
| "react-timeago": "^7.2.0", |
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.
오 기능 구현에 필요한 라이브러리 탐색하는거 좋습니다
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 디렉토리의 경우, 콤포넌트를 위한 디렉토리가 아닌것으로 생각되어요.
따라서 소문자로 경로 이름을 두면 어떨까요?
| const response = await fetch( | ||
| `https://panda-market-api.vercel.app/products?${query}` | ||
| ); | ||
| 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.
상수로 관리하는거 너무 좋습니당
| const BASE_URL = "https://panda-market-api.vercel.app"; | ||
|
|
||
| // 베스트/전체 상품 리스트 | ||
| async function getProductData(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.
default 값 주신거 너무 좋습니다 👍🏻
| } | ||
|
|
||
| // 디테일 상품 정보 | ||
| export async function getProductId(productId, setProductData) { |
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 함수들의 역할을 다시 한번만 생각해볼까요?
우선 API에 둔 함수의 역할은 다음과 같이 생각되는데요
- 전달받은 인자값 또는 데이터를 서버에 전송하기 좋게 가공하기
- 서버에 네트워크 통신 전송하기
- 결과에 따라 오류를 던져주거나 전달받은 데이터를 반환해주기
이 상황에서 api함수들은 딱 위 작업만 진행을 하고, 화면에 보여질 데이터를 업데이트 해주는 setState액션은 콤포넌트와 가장 가까운 곳에서 작업을 진행해주어야 할 것 같아요!
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.
| useEffect(() => { | ||
| const handleClickOutside = (event) => { | ||
| // 해당 이벤트가 영역 밖이라면 케밥 버튼 비활성화 | ||
| if (outRef.current && !outRef.current.contains(event.target)) { | ||
| setSelectBox(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener("click", handleClickOutside, true); | ||
| return () => { | ||
| document.removeEventListener("click", handleClickOutside, true); | ||
| }; | ||
| }, [handleOptionClick]); |
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.
이런 디테일 신경쓰시는거 너무 좋네요
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.
혜림님의 강점이라고 생각합니다
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 { useState, useEffect } from "react"; | ||
| import getProductData from "../../Api/api"; | ||
| import { useMediaQuery } from "react-responsive"; | ||
| import CalculatorMediaQuery from "../../utils/calculatormediaQuery"; |
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.
class 또는 Component가 아닌 이상은 소문자로 시작하도록 작성해주세요!
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.
이렇게 한번 3rd party module을 래핑해서 사용하신거 너무 좋습니다
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.
👍🏻

요구사항
스크린샷
멘토에게