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

Mission4 / 강하은 #30

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

kanghaeun
Copy link
Collaborator

1. 구현 모습

2. 해결 과정

image
초기 상태에서는 두 가지 상태 변수를 선언하였습니다.
첫 번째는 상품 데이터를 담을 빈 배열인 products, 두 번째는 현재 보여줄 상품의 종류를 나타내는 currentView 이며 초기값은 "All product"로 설정하였습니다.
useEffect 훅을 사용하여, currentView 상태가 변경될 때마다 해당 상품 데이터를 불러왔습니다.
axios를 통해 currentView가 "All product"일 경우 products.json을, "Sale product"일 경우 sale_products.json을 요청하게 하였습니다. 그리고 응답받은 데이터는 products 상태에 저장됩니다.
상품 데이터를 불러와 화면에 표시하는 부분은 map 함수를 사용하여 구현하였습니다. products 배열의 각 요소를 순회하며, 상품의 이미지, 이름, 가격 정보를 포함한 요소를 생성하여 화면에 렌더링합니다.
상품 전환 기능은 한 개의 버튼을 통해 제공됩니다. 이 버튼은 현재 currentView 상태에 따라 표시되는 텍스트가 달라지며, 클릭 시 currentView 상태를 "All product"와 "Sale product" 간에 전환합니다.
이를 통해 사용자는 버튼을 클릭함으로써 두 가지 상품 목록 중 하나를 선택해 볼 수 있습니다.

3. To 리뷰어에게

  • 간단하다고 생각해서 App.jsx에 코드를 다 넣었는데, 보통 App.jsx에 어떤 코드를 포함시키는지 궁금해요. 개인적으로는 App.jsx에는 렌더링할 컴포넌트들과 모든 컴포넌트가 공통으로 필요로 하는 상태 관리 코드만을 넣는 편이에요.
    가독성을 고려했을 때 이런 방식이 괜찮은지, 어느 정도가 적당한지 궁금합니다
  • 이 외에도 고려해야 할 점들이나 추가로 하면 괜찮을 것들 조언해주시면 감사하겠습니다!

@kanghaeun kanghaeun added the mission 미션 입니다! label Apr 9, 2024
@kanghaeun kanghaeun requested a review from vvvpiano April 9, 2024 12:27
@kanghaeun kanghaeun self-assigned this Apr 9, 2024
Copy link

vercel bot commented Apr 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
donut-study ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 9, 2024 0:27am

Copy link
Collaborator

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 배경사진이 꽤나 고급지네요 어디인가요? ㅋㅋ
코멘트에도 달았지만 민준님 PR(#29)에서 리뷰할 만한 내용과 겹치는 것들이 있어서 시간나실 때 같이 확인해보면 좋을 것 같습니다!

간단하다고 생각해서 App.jsx에 코드를 다 넣었는데, 보통 App.jsx에 어떤 코드를 포함시키는지 궁금해요. 개인적으로는 App.jsx에는 렌더링할 컴포넌트들과 모든 컴포넌트가 공통으로 필요로 하는 상태 관리 코드만을 넣는 편이에요. 가독성을 고려했을 때 이런 방식이 괜찮은지, 어느 정도가 적당한지 궁금합니다

(100% 개인의견입니다.)
"가독성" 보다는 역할과 책임에 따라 분리하는 것이라고 생각합니다.(물론 가독성도 챙기면 좋지만.. 역할과 책임에 따라 분리하다보면 대부분 가독성도 좋아집니다.) 제가 다른 리뷰에는 남겨놓았지는 모르겠지만, 컴포넌트는 UI로직에 대한 책임을 가진다고 생각합니다. 따라서 상태처리, API 처리 등은 custom hook에 위임하고, 컴포넌트에서는 사용하는 상태와 handler만 노출하는 식이죠.

그래서 결론은, 되도록 UI관련된 상태나 event handler만 노출하되 세부적인 로직등은 custom hook이나 다른 함수로 빼서 작성하는 편입니다. 설명을 너무 어렵게 한 것 같다면 다음 스터디 시간에 이야기 해보죠!

);
}}
>
{currentView === "All product" ? "All product" : "Sale product"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{currentView === "All product" ? "All product" : "Sale product"}
{currentView}

로 쓸수 있나요??

<ProductList>
{products.map((product) => (
<div key={product.id}>
<img src={product.src} style={{ width: "100%" }} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

styled-component를 사용하지 않고 inline style을 준 이유가 있나요?

Comment on lines +11 to +21
axios
.get(
`api/${
currentView === "All product" ? "products.json" : "sale_products.json"
}`
)
.then((res) => {
setProducts(res.data);
})
.catch((error) => {
console.error("상품 데이터를 불러오는 중 오류가 발생했습니다.", error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

#29 (comment)
(위와 다른 코멘트입니다.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mission 미션 입니다!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants