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 #222

Conversation

ojm51
Copy link
Collaborator

@ojm51 ojm51 commented Jul 5, 2024

요구사항

기본

상품 상세

  • 상품 상세 페이지 주소는 “/items/{productId}” 입니다.
  • response 로 받은 데이터로 화면을 구현합니다.
  • 목록으로 돌아가기 버튼을 클릭하면 중고마켓 페이지 주소인 “/items” 으로 이동합니다

상품 문의 댓글

  • 문의하기에 내용을 입력하면 등록 버튼의 색상은 “3692FF”로 변합니다.
  • response 로 받은 데이터로 화면을 구현합니다

주요 변경사항

  • 반응형을 제외한 기본 요구사항 반영
  • 상품 이미지가 존재하지 않을 때 보여질 기본 이미지 추가
  • jsx를 리턴하는 컴포넌트의 확장자를 js -> jsx로 변경

스크린샷

image
image
image

멘토에게

  • 화면을 구성하는 과정에서 div 태그로 속성을 조정하는 것이 편해, 자꾸 모든 내용을 div로만 감싸게 됩니다. 아래와 같은 화면이 있을 때 어떤 태그로 구조와 계층을 만드는 것이 좋은지 궁금합니다.
500,000원
  • 배포 링크입니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@ojm51 ojm51 requested a review from Taero-Kim July 5, 2024 14:41
@ojm51 ojm51 added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Jul 5, 2024
? "add-inquiry-button active"
: "add-inquiry-button";

const placeholder =
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
요렇게 변하지 않는 placeholder는 컴포넌트 밖에 상수로 선언해도 괜찮을 것 같아요!

placeholder={placeholder}
onChange={handleValueChange}
></textarea>
<button className={buttonClassName} disabled={!isInputFill}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳! isInputFill 상태에 따른 disabled 처리를 잘하셨네요!

</div>

<div className="inquiry-wrapper">
{comments.length !== 0 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
요런 경우에 comment.length가 0인지 아닌지에 따라 boolean으로 형변환을 하여 아래와 같이 써도 괜찮을 것 같아요!

Suggested change
{comments.length !== 0 ? (
{!!comments.length ? (

src={inquiryEmptyImage}
alt="아직 문의가 없습니다."
width="200px"
height="200px"
Copy link
Collaborator

Choose a reason for hiding this comment

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

브라우저가 이미지 영역을 계산하여 미리 해당 공간을 비워둘 수 있도록 width, height 속성을 잘 부여하셨네요!
참고로 이 부분은 css로 정의해도 무방합니다!

width="486px"
height="486px"
onError={onErrorImg}
></img>
Copy link
Collaborator

Choose a reason for hiding this comment

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

p5;
셀프 클로징 태그를 사용해도 괜찮을 것 같아요!

Suggested change
></img>
/>

alt={name}
width="486px"
height="486px"
onError={onErrorImg}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이미지 로드가 제대로 안되는 경우 target.src에 대체 이미지를 할당하는 처리 좋습니다!


function ProductListItem({ item, className }) {
const { id, name, price, image, favoriteCount } = item;
const classNames = `image ${className}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p5;
className을 props로 받아서, ProductListItem 컴포넌트를 실제로 사용하는 곳에서
필요한 경우 추가적인 className을 받아 추가 스타일을 적용하도록 잘 하신 것 같아요!

다만, 이 classNames는 product 이미지에 대한 클래스명이기 때문에,
그에 맞게 props로 받는 className과 classNames의 변수명이 그에 맞게 변경되면 좋을 것 같아요!

Suggested change
const classNames = `image ${className}`;
const productImageClassNames = `image ${productImageClassName}`;

setProductComments(data);
}
getProductComment();
}, [productId]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
깔끔하게 해당 페이지에 필요한 상품정보와 상품 댓글을 잘 불러오셨네요!
그런데 이렇게 fetching하여 상태를 업데이트하는 로직은 커스텀 훅으로 분리하여 사용해도 깔끔할 것 같아요!

커스텀훅을 구성하는 방법은 다양합니다! 아래 예시와 같은 방법도 있고,
useProdcutDetail과 useProductComment 등으로 댓글과 상품정보를 각각 다른 커스텀훅에서 불러오는 방법도 있을 것 같습니다.
한 번 여러가지 방법을 생각해보세요!

예시) useProductDetails()

export const useProductDetails = ({ productId }) => {
  const [productDetails, setProductDetails] = useState([]);
  const [productComments, setProductComments] = useState([]);

  useEffect(() => {
    async function getProductDetail() {
      const data = await fetchProductDataById(productId);
      setProductDetails(data);
    }
    getProductDetail();

    async function getProductComment() {
      const data = await fetchProductComment(productId);
      setProductComments(data);
    }
    getProductComment();
  }, [productId]);

  return {
    productDetails,
    productComments
  };
}; 

ProductDetailPage 에서 사용시

const ProductDetailPage = () => {
  const { productId } = useParams();
  
  const {productDetails, productComments} = useProductDetails({ productId });

  return (
    <div>...</div>
  )
}

}

export async function fetchProductComment(productId) {
const url = `${BASE_URL}/products/${productId}/comments?limit=10`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
뒤에 붙는 limit 쿼리 스트링은 나중에 여러가지 숫자를 전달하여, 페이지네이션 등에 사용할 수도 있으니
10으로 하드코딩 해놓는 것 보다는 함수에서 limit 인자를 받아서 사용하는 것도 괜찮을 것 같아요!

export async function fetchProductComment(productId, limit) {
  const url = `${BASE_URL}/products/${productId}/comments?limit=${limit}`;
  ...
}

@Taero-Kim
Copy link
Collaborator

정민님 고생하셨어요!
미션을 거듭할수록 코드가 전반적으로 깔끔해지고 가독성이 좋아지는 것 같아요!
너무 잘하고 계십니다!

앞으로는 커스텀훅을 적극 활용하여
UI를 보여주는 컴포넌트에 포함된 다양한 로직들을 분리시켜
컴포넌트를 가볍게 만드는 것을 연습해보는 것을 권장 드립니다!

+) 컴포넌트 파일의 확장자를 jsx로 변경하신 것도 아주 잘하셨습니다!

@Taero-Kim
Copy link
Collaborator

화면을 구성하는 과정에서 div 태그로 속성을 조정하는 것이 편해, 자꾸 모든 내용을 div로만 감싸게 됩니다. 아래와 같은 화면이 있을 때 어떤 태그로 구조와 계층을 만드는 것이 좋은지 궁금합니다.

말씀처럼 div로만 구성하기 보다는
적절하게 시멘틱 태그들을 이용하여, html의 구조를 작성하면 문서 구조의 가독성 측면에도 좋고
검색 엔진 측면에서도 유리합니다!

정답이 있는 건 아니지만, 예시를 보여드리자면 아래와 같습니다.

    <div class="container">
      <article class="product">
        <section class="product-main">
          <h2 class="product-title">아이패드 미니 팔아요</h2>
          <p class="product-price">500,000원</p>
        </section>

        <section class="product-description">
          <h3>상품 소개</h3>
          <p>액정에 잔기스랑 ..</p>
        </section>

        <section class="product-tags">
          <h3>상품 태그</h3>
          <ul>
            <li>#아이패드 미니</li>
            <li>#애플</li>
            <li>#가성비</li>
          </ul>
        </section>
      </article>
    </div>

@Taero-Kim Taero-Kim merged commit cf5a0d8 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