Skip to content
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 #216

Merged

Conversation

purplenib
Copy link
Collaborator

@purplenib purplenib commented Jul 5, 2024

요구사항

  • Github에 PR(Pull Request)을 만들어서 미션을 제출합니다.
  • 피그마 디자인에 맞게 페이지를 만들어 주세요.
  • React를 사용합니다

상품 상세

  • 상품 상세 페이지 주소는 “/items/{productId}” 입니다.
  • 목록으로 돌아가기 버튼을 클릭하면 중고마켓 페이지 주소인 “/items” 으로 이동합니다
  • response 로 받은 아래의 데이터로 화면을 구현합니다- favoriteCount : 하트 개수
    images : 상품 이미지
    tags : 상품 태그
    name : 상품 이름
    description : 상품 설명

상품 문의 댓글

  • 문의하기에 내용을 입력하면 등록 버튼의 색상은 “3692FF”로 변합니다.
  • response 로 받은 아래의 데이터로 화면을 구현합니다
    image : 작성자 이미지
    nickname : 작성자 닉네임
    content : 작성자가 남긴 문구
    description : 상품 설명
    updatedAt : 문의글 마지막 업데이트 시간

주요 변경사항

design

  • Items 페이지, 아이템 요소의 기본 이미지 추가
  • ItemInfo 페이지, 피그마 시안의 디자인 적용

refactor

  • Items 페이지, util 함수 분리
  • constatns.jsx 생성, 데이터 초기 값 분리
  • 데이터 패칭 용도 useFetch 커스텀 훅 생성 및 적용
  • AddItemDetails 컴포넌트에 htmlFor 속성 추가
  • AddItemTags 컴포넌트에서 tag 배열 index 값 -> id 값 변경
  • ItemDetails, ItemTags 컴포넌트 분리

rename

  • mediaquery.css 파일 삭제, 미디어쿼리 디자인 각 페이지의 CSS 파일에 병합
  • 네이밍 App.jsx -> Router.jsx
  • 네이밍 ItemDetials, ItemTags -> AddItemDetails, ...

feat

  • ItemInfo 페이지, 기본 요구사항 구현
  • Items 페이지, 페이지네이션 추가

스크린샷

스크린샷 2024-07-05 205246


스크린샷 2024-07-05 205304


스크린샷 2024-07-05 205319


멘토에게

  • 멘토링 시간에 말씀하셨던 커스텀 훅을 만들어봤습니다. 개선점이 있을지 확인부탁드립니다. 🥳
  • CSS 파일이 많이 복잡한 상태입니다. 테일윈드 CSS를 다음 미션에 적용해보겠습니다. 🙏🏻

@purplenib purplenib added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Jul 5, 2024
@purplenib purplenib requested a review from wlgns2223 July 5, 2024 12:03
@wlgns2223
Copy link
Collaborator

주요 변경사항을 잘 적어주셔서 제가 파악하기가 쉬웠습니다 !
변경하신 파일중에 App.jsx는 관용적으로 App.jsx로 계속 사용을 합니다 ㅎㅎ 이 파일은 파일명을 변경안하는게 관습인거 같아요.
index.js도 마찬가지입니다 ! 추후에 App.jsx에 라우터 이외에도 여러 로직들이 추가됩니다 ~ ㅎㅎ

Comment on lines +6 to +11
const paramsString = JSON.stringify(requestParams);

useEffect(() => {
const fetchData = async () => {
try {
const parsedParams = JSON.parse(paramsString);
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 paramStringstringfy했다가 parse하는 이유가 무엇일까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

const { data: productData = INITIAL_PRODUCTID } = useFetch(
    // getProductId(),
    getProductApiCall(id),
    {
      productId,
    },
    INITIAL_PRODUCTID
  );


const fetchData = async () => {
      try {
        const parsedParams = JSON.parse(paramsString);
        // const result = await apiCall(parsedParams);
        const result = await apiCall();

        setData(result);
      } catch (error) {
        console.error("데이터 패칭 실패:", error);
      }
    };


// 함수를 리턴하는 함수
export const getProductApiCall = (id) => () => getProductId({ productId: id });

useFetch 커스텀훅을 작성하신 점 너무 잘하셨습니다 !
함수를 리턴하는 함수를 이용하시면 useFetch 내부에서 파라미터를 신경 쓸 필요없이 apiCall()만 해주면 될 것 같은 생각이 드는데, 혹시 함수를 리턴하는 함수 이해가 가실까요? ㅎㅎ
참고해보시면 좋을 것 같아요 !

Copy link
Collaborator Author

@purplenib purplenib Jul 8, 2024

Choose a reason for hiding this comment

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

여기서 paramString을 stringfy했다가 parse하는 이유가 무엇일까요?

원래는 객체 requestParams를 그대로 받아 패치했었는데 items 페이지를 확인해보니 무수히 많은 리퀘스트를 보내고 있었습니다. 객체를 참조할 때마다 리퀘스트를 보내는 문제라고 생각했고, 객체가 아닌 문자열을 의존성 배열에 넣어서 해결해야겠다고 생각했습니다. 그래서 저 방식대로 해봤더니 문제가 해결돼서 그렇게 했었습니다..!

그런데 지금 보니 parse 를 다시 해줄 필요는 없고 apiCall 함수에 객체 requestParams를 주면 됐을 것 같네요..?

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 +43 to +49
<ul className="tags">
{productData.tags.map((tag, index) => (
<li key={index} className="tag-info">
#{tag}
</li>
))}
</ul>
Copy link
Collaborator

Choose a reason for hiding this comment

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

순서가 없는 리스트를 ul태그로 쓰신 점 너무 좋습니다 !

</div>
</div>
</section>
<InquiryInput productId={productId} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

InquaryInput에 필요한 props인 productId만 전달 해주신 점 잘 하셨습니다.
컴포넌트에서 필요없는 데이터(props)는 웬만하면 받지 않는게 유지보수하기 쉬운 코드가 되더라구요 !

Comment on lines +26 to +33
<textarea
className="item-detail-input inquiry-input"
type="text"
id="inquiry-input"
value={inquiryInput}
placeholder="개인정보를 공유 및 요청하거나, 명예 훼손, 무단 광고, 불법 정보 유포시 모니터링 후 삭제될 수 있으며, 이에 대한 민형사상 책임은 게시자에게 있습니다."
onChange={handleInputChange}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

코멘트 UI에 input이 아니라 textarea를 써주신 점 잘 하셨습니다 !

Comment on lines +9 to +16
const { data: commentsData } = useFetch(
getProductComments,
{ productId, limit },
{ list: [INITIAL_COMMENTS] }
);
const navigate = useNavigate();

const comments = commentsData.list;
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 컴포넌트 내부에서 해결 못하는 부분( productId )를 props로 받고
데이터가 필요한 컴포넌트에서 데이터를 호출한 점이 관심사의 분리를 잘 지켜주신 점 같습니다.

comments fetch 로직을 알 필요없는 다른 컴포넌트는 comments 데이터의 존재를 알 수 없기 때문에 의존성도 약해지고 응집도는 높아진 좋은 코드라고 생각합니다 !

@wlgns2223
Copy link
Collaborator

comments의 데이터 Fetch부분을 comment 컴포넌트 내부에서 하고 필요한 props ( product id) 만 외부에서 받아온 점이 관심사의 분리를 잘 이해하고 적용하신거라고 생각이 들어요 ! 스프린트 미션 수고 하셨습니다 !

@wlgns2223 wlgns2223 merged commit c48399a into codeit-bootcamp-frontend:React-김영주 Jul 8, 2024
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