MISSON4 / 이규민 #33

Klomachenko · 2024-04-09T19:21:31Z

1. 구현 모습

[mission] 4주차 미션입니다! #22

2. 해결 과정

배포 링크

클릭시 이동합니다

기능 요구사항

기본 화면에서 제품 보여주기
- 제품 이름
- 제품 가격
- 제품 사진
상품 조건별로 보여주기
- 전체 상품
- 세일중인
- 낮은가격순

파일 구조

파일 구조 보기

src
 ┣ assets
 ┣ components
 ┃ ┣ atom
 ┃ ┗ molecule
 ┃ ┗ organism
 ┣ Page
 ┣ hooks
 ┣ utils
 ┣ tests
 ┣ css
 ┣ App.css
 ┣ App.jsx
 ┣ index.css
 ┗ main.jsx

상태 관리

기존 상태의 경우 shopPage에서 관리해 주고 있었으나, hooks 분리를 통해 useProductManage에서 관리하고 있습니다.
fetch 함수를 통해 가져온 데이터를 state에 담아서, 관련 로직(handleOnSale, handlePrice)들을 통해 상태를 관리합니다.

상태 관련 질문점

훅 분리 전 커밋에서 ShopPage에서 너무 많은 로직을 수행하고 있었습니다.
- 상품 데이터를 받아오는 useEffect(fetch)훅
- 전체 상품을 가져오는 handleAllItem
- 할인 상품을 필터링하는 handleOnSale
- 가격에 따라 상품을 정렬하는 handlePrice
위 로직들의 경우엔 ‘비즈니스 로직’에 해당한다고 판단하였고, 이에 따라 useProductManage라는 훅으로 묶어 처리하였습니다.
혹 위의 처리가 맞는지, 아니면 handleAllItem의 경우에는 다른 훅으로 따로 분리를 해주는 편이 나았을지 궁금합니다!

3. To 리뷰어에게

위의 상태관리 관련 질문을 작성하였습니다(너무 길어 따로 분리하였습니다 번거롭게 해드려 죄송합니다.)
전체적으로 hooks의 분리가 적절히 이루어졌는지 궁금합니다!
아래는 리팩터링 할 목록입니다. 혹시 올바르게 찾았는지, 더 하면 좋겠다 싶은게 있는지 확인해주시면 매우 감사하겠습니다!
- 현재 api 내부의 sale_products.json을 사용하지 않고 있는데, 사용하지 않은 이유는, 상태관리를 통해 불필요한 통신 횟수를 줄이기 위해서 였습니다.
  하지만, 해당 데이터가 굉장히 클 경우엔 상태로 관리하는 것보다는 그냥 받아오는 것이 더 합리적일 것이라 판단하여 fetch함수를 한번 더 사용해 세일중인 물품을 가져오는 것이 좋아보입니다.
  - 혹시, 이러한 경우에 통신을 한번 더 하는지, 아니라면 상태로 관리해주는지 궁금합니다!
- 전체적으로, 상태의 관리가 단조롭다는 생각이 듭니다. 만약 장바구니가 추가된다거나 하는 경우를 가정해 상태를 ‘상태관리 라이브러리’로 관리해주거나, 더 상단으로 콜백을 통해 옮기거나, 아예 App.jsx로 끌어올려야 할 필요성이 있어 보입니다!

Description: fontSize, backgroundColor, color, borderColor를 미리 설정합니다

Description: 카드 컴포넌트를 생성합니다

Description: theme에서 중복되는 색상들을 삭제하고 하나로 통일하였습니다

Description: 가장 기본적인 형태의 Card Component를 구현하였습니다(최소기능)

…into mission

vercel · 2024-04-09T19:21:35Z

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 7:29pm

JUDONGHYEOK

안녕하세요 규민님! 리뷰가 늦어져서 죄송해요. 요새 정신이 없네요. 규민님도 중간고사 기간이셨나요? 학업과 미션을 같이 병행하시느라 너무 고생이 많으세요. 지금 상황에서는 미흡한 모습들이 보이지만 지금 이대로 꾸준히 하시면 꼭 좋은 개발자가 되시리라고 믿습니다! 같이 화이팅 해보아요~~ 👍

우선 미션에 대한 전체적인 피드백을 드리자면은 아직은 조금 javascript나 react에 대한 경험이 많이 부족하시지만 그래도 그 안에서 많은 고민을 했다는 것이 느껴져서 좋았어요. custom hook을 분리한 부분이라든지 나름대로 구조를 가져가려고 하시는 부분들에 있어서 열정이 느껴져서 좋았습니다.ㅎㅎ

앱관점에서 body의 margin이라던지 반응형 대응이 잘 되지 않고 있어서 조금 아쉬웠어요. 시간이 남으신다면 이런것도 챙겨주시면 사용성 측면에서 훨씬 더 좋은 제품이 나올 것 같습니다!

질문답변

혹 위의 처리가 맞는지, 아니면 handleAllItem의 경우에는 다른 훅으로 따로 분리를 해주는 편이 나았을지 궁금합니다!
전체적으로 hooks의 분리가 적절히 이루어졌는지 궁금합니다!

굳이 handleAllItem을 다른 훅으로 돌릴 필요는 없어보여요. 아마 API로직 때문에 그렇게 느끼셨나 생각이 들었는데 지금 상황에서는 hook의 복잡도가 크게 높아보이지 않아서 분리하지 않아도 괜찮다고 느꼈습니다. 굳이 분리를 하자면 API Layer를 별도로 형성할 수도 있을 것 같은데 API 로직이 하나밖에 없어서 안해도 될거 같아요ㅎㅎ
다만 그외에 훅에서 아쉬웠던 점은 상태관리에 대한 관점이 조금 아쉬웠어요. filter된 데이터나 sort한 데이터를 어떻게 처리해야할지 어떻게 하면 더 효율적인 네트워크 페칭을 할 것인지에 대한 고민이 조금 더 필요해보입니다!
hook의 분리는 잘이뤄졌다고 생각해요. 코드 스타일의 차이지만 저는 custom hook을 사용하는 것을 좋아하기 때문에 분리 자체는 잘하셨다고 생각합니다.

하지만, 해당 데이터가 굉장히 클 경우엔 상태로 관리하는 것보다는 그냥 받아오는 것이 더 합리적일 것이라 판단하여 fetch함수를 한번 더 사용해 세일중인 물품을 가져오는 것이 좋아보입니다.
혹시, 이러한 경우에 통신을 한번 더 하는지, 아니라면 상태로 관리해주는지 궁금합니다!

아하 sale_products.json이 왜 있나 생각했는데 이런 이유 때문이군요. 결과는 규민님 의견에 동의를 하지만 이유는 조금 다른 것 같아요. 보통 이런 목록을 보여주는 API는 페이징 처리가 되어있습니다. 따라서 모든 목록을 불러오지 않고 범위로 불러오게 되죠. 그러면 프론트에서는 당연히 해당 데이터만을 가지고선 sorting이나 filtering이 불가하기 때문에 API에 의존하는 경향이 큽니다.
따라서 모든 목록을 불러오는 지금 상황에서는 굳이 filter나 sort 로직에서 추가 API는 필요하지 않아보여요. 하지만 규민님이 작성해주신 현상황에서도 보다 효율적인 네트워크 페칭은 생각해볼수도 있을 것 같아요.

전체적으로, 상태의 관리가 단조롭다는 생각이 듭니다. 만약 장바구니가 추가된다거나 하는 경우를 가정해 상태를 ‘상태관리 라이브러리’로 관리해주거나, 더 상단으로 콜백을 통해 옮기거나, 아예 App.jsx로 끌어올려야 할 필요성이 있어 보입니다!

현재 상황에서는 고민할 필요 없는 문제라고 생각이 됩니다. 어떻게 미션이 주어질지 모르는 상황에서 그렇다고 가정해서 개발하는 것은 흔히들 YAGNI라고 해서 지양해야된다고 생각해요. 말씀하신대로 장바구니가 추가된다면 같이 고민해보면 좋을 것 같습니다.

JUDONGHYEOK · 2024-04-14T07:33:03Z

package.json

-    "react-dom": "^18.2.0"
+    "react-dom": "^18.2.0",
+    "react-router-dom": "^6.22.3",
+    "react-youtube": "^10.1.0"


오 새로운 라이브러리를 사용하셨군요. 현상황에서는 단순하게 iframe으로 유튜브 영상을 삽입할 수도 있을 것 같은데 해당 라이브러리를 채택한 이유가 따로 있을까요??

JUDONGHYEOK · 2024-04-14T07:33:39Z

src/hooks/useProductManage.jsx

jsx문법이 사용되지 않고 있어요. js로 바꾸는 것이 좋을 것 같아요

JUDONGHYEOK · 2024-04-14T07:39:03Z

src/styles/theme.js

해당 파일을 사용하는 곳이 있을까요??

JUDONGHYEOK · 2024-04-14T07:39:45Z

vite.config.js

+  resolve: {
+    alias: [
+      { find: "@", replacement: "/src" },
+      { find: "@components", replacement: "/src/components" },
+      { find: "@css", replacement: "/src/css" },
+      { find: "@image", replacement: "/src/assets/Images" },
+    ]
+  }


절대경로를 등록하려고 하신 것 같은데 사용하지 않으신 이유가 있을까요?

ts가 아닌 js도 이런것이 되군요 대박

JUDONGHYEOK · 2024-04-14T07:40:14Z

src/components/atom/DefaultButton.jsx

+function DefaultButton({ buttonText, onClick }) {
+  return (
+    <button className={styles.button} onClick={onClick}>
+      {buttonText}
+    </button>
+  );
+}


이런 상황에서는 children을 활용하는 것이 더욱 잘 읽힐 것 같은데 어떻게 생각하세요? TabButton도 마찬가지 이구요

JUDONGHYEOK · 2024-04-14T07:51:18Z

src/components/organism/Header.jsx

+      </div>
+      <div className={styles.userBox}>
+        <button className={styles.userButton}>
+          <img src={user} alt='cart icon' />


src는 user인데 alt는 cart icon이네요!

JUDONGHYEOK · 2024-04-14T07:53:12Z

src/hooks/useProductManage.jsx

@@ -0,0 +1,38 @@
+import { useState } from 'react';
+
+function useProductManage(initialProducts) {


초기화를 빈배열로만 하는 것 같은데 prop을 뚫어줄 필요가 따로 있었을까요?

JUDONGHYEOK · 2024-04-15T02:01:03Z

src/page/ShopPage.jsx

+  return (
+    <div className={styles.container}>
+      <div className={styles.buttonBox}>
+        <DefaultButton buttonText='All' onClick={handleAllItem} />


이렇게 구성한다면 All버튼을 누를때마다 네트워크 페칭을 하게 될 것 같아요 다른 방식은 없을까요??

JUDONGHYEOK · 2024-04-15T02:01:36Z

src/hooks/useProductManage.jsx

+    handleOnSale,
+    handlePrice,
+    handleAllItem,


handle이라는 이름이 개인적으로 조금은 어색한데요. event를 다룰 것처럼 네이밍인데 단순 fetch나 sort라서요. 다른 네이밍을 고려해볼수 있을까요?

JUDONGHYEOK · 2024-04-15T02:02:44Z

src/page/ShopPage.jsx

@@ -0,0 +1,29 @@
+import { useState, useEffect } from 'react';


사용하지 않는 훅은 제외해주세요. 기본적인 lint나 prettier를 적용하면 참 좋을텐데 말이죠

geongyu09 and others added 27 commits March 12, 2024 22:41

첫 주차 미션

d0af73d

style: theme.js 설정

fd7bc7d

Description: fontSize, backgroundColor, color, borderColor를 미리 설정합니다

style: fontWeight 추가

c11a40b

feat: create Card Component

1986e32

Description: 카드 컴포넌트를 생성합니다

chore: 절대경로 설정

c1afc08

feat: theme 중복 삭제

c10dc18

Description: theme에서 중복되는 색상들을 삭제하고 하나로 통일하였습니다

feat: Card Component 구현

e35acfa

Description: 가장 기본적인 형태의 Card Component를 구현하였습니다(최소기능)

feat: 4주차 미션을 위한 환경 설정

5f29f23

Merge branch 'mission' of https://github.com/Klomachenko/donut-study …

a8f4daa

…into mission

feat: Header 구현

b1204c2

feat: TabButton 구현

00f0417

feat: MainPage 구현

98709bd

feat: react-router-dom 적용

15e974b

feat: Item 구현

8b8dec3

feat: ItemList 구현 및 fetch를 통한 item get

6f6b310

feat: DefaultButton 구현

c8153e2

feat: 상품 state 위치 변경

c573874

feat: onSale 세일중인 상품만 받아오기 구현

c412111

feat: 가격 오름차순 구현

aa8013f

feat: All 전체상품 불러오기 구현

cc7179f

feat: 데이터 수정

9006616

delete: Card 컴포넌트 삭제

ae62b6c

feat: DefaultButton props 수정

73f33d0

style: 스타일 수정

03d6d9b

chore: Header 컴포넌트 위치 수정에 따른 경로 변경

9208497

chore: 의존성 설치 및 gitignore 설정

fd3c89e

feat: hooks 분리

f84f144

Klomachenko added the mission 미션 입니다! label Apr 9, 2024

Klomachenko self-assigned this Apr 9, 2024

vercel bot deployed to Preview April 9, 2024 19:21 View deployment

chore: packageplock.json 제거

aae09c6

vercel bot deployed to Preview April 9, 2024 19:29 View deployment

Klomachenko changed the title ~~Mission4/klomachenko~~ MISSON4 / 이규민 Apr 10, 2024

Klomachenko requested a review from JUDONGHYEOK April 10, 2024 17:00

JUDONGHYEOK requested changes Apr 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MISSON4 / 이규민 #33

MISSON4 / 이규민 #33

Klomachenko commented Apr 9, 2024

vercel bot commented Apr 9, 2024 •

edited

Loading

JUDONGHYEOK left a comment

JUDONGHYEOK Apr 14, 2024

JUDONGHYEOK Apr 14, 2024

JUDONGHYEOK Apr 14, 2024

JUDONGHYEOK Apr 14, 2024

2yunseong Apr 30, 2024

JUDONGHYEOK Apr 14, 2024

JUDONGHYEOK Apr 14, 2024

JUDONGHYEOK Apr 14, 2024

JUDONGHYEOK Apr 15, 2024

JUDONGHYEOK Apr 15, 2024

JUDONGHYEOK Apr 15, 2024

		@@ -0,0 +1,38 @@
		import { useState } from 'react';

		function useProductManage(initialProducts) {

		@@ -0,0 +1,29 @@
		import { useState, useEffect } from 'react';

MISSON4 / 이규민 #33

Are you sure you want to change the base?

MISSON4 / 이규민 #33

Conversation

Klomachenko commented Apr 9, 2024

1. 구현 모습

2. 해결 과정

배포 링크

기능 요구사항

파일 구조

상태 관리

상태 관련 질문점

3. To 리뷰어에게

vercel bot commented Apr 9, 2024 • edited Loading

JUDONGHYEOK left a comment

Choose a reason for hiding this comment

질문답변

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Apr 9, 2024 •

edited

Loading