Skip to content

[4,5단계미션] 홍의민 미션 제출합니다.#95

Open
EM-H20 wants to merge 39 commits into
cho-log:em-h20from
EM-H20:em-h20
Open

[4,5단계미션] 홍의민 미션 제출합니다.#95
EM-H20 wants to merge 39 commits into
cho-log:em-h20from
EM-H20:em-h20

Conversation

@EM-H20
Copy link
Copy Markdown

@EM-H20 EM-H20 commented May 4, 2026

안녕하세요, 임규영 리뷰어님! 프론트엔드 4기 홍의민입니다.

이전 미션에서 한 번 뵌 적이 있어 다시 뵙게 되어 반갑습니다!

이번 미션 진행하면서 설계문서를 what? how? 초점에 맞춰서 작성해습니다.


지난 미션에서 useState로 상태를 다루는 흐름은 어느 정도 익숙해졌는데, 이번 4~5단계를 진행하면서 컴포넌트 간에 상태를 어떻게 전이시키는지, 그리고 렌더링 외부와 어떻게 동기화하는지(useEffect) 에 대해 알아갈 수 있었습니다.(여전히 헷갈리기는 하지만 ㅎㅎ) 특히 controlled/uncontrolled 개념이나 children props로 컴포넌트를 재사용 가능하게 만드는 부분, useEffect로 사이드 이펙트를 분리하는 부분이 처음에는 헷갈렸지만 직접 손으로 구현하면서 왜 이렇게 쓰는지가 조금씩 이해되었습니다.

제 생각에는 아래와 같이 이해하고 있는데 이게 맞을까요?

uncontrolled : 필요할 때만 꺼내봄
controlled : 실시간으로 추적

이번 PR에서는 4단계와 5단계를 함께 구현했습니다.

4단계: 음식점 추가 폼 (controlled vs uncontrolled)

  • Header 의 추가 버튼에 onAddClick prop 을 추가하여, App 의 isAddModalOpen 상태를 변경해 모달이 열리도록 했습니다.
  • App 에서 기존에 정적으로 import 하던 restaurantsuseState 로 끌어올려 추가/갱신이 가능한 상태로 만들었고, addRestaurant 함수를 만들어 AddRestaurantModal 에 prop 으로 전달했습니다.
  • 폼 값은 uncontrolled 방식 (FormData) 으로 수집했습니다. 매 입력마다 state 를 갱신하는 controlled 방식보다, 제출 시점에 한 번만 값을 모으면 되는 이번 케이스에 더 잘 맞는다고 판단했습니다.
  • 추가 후에는 모달이 자동으로 닫히고, backdrop 클릭으로도 닫히도록 처리했습니다.
  • 조건부 렌더링은 && 대신 삼항 연산자(isOpen ? <Modal /> : null)로 통일했습니다. 0 같은 falsy 값이 의도치 않게 화면에 노출되는 사고를 미리 막기 위해서 저런 방법을 채택했습니다. 여러 리뷰를 받았을 때 공통으로 받은 내용이라 이번에 적용해봤습니다!

5단계: API 연동 (useEffect)

  • constants/restaurants.js 의 정적 데이터 의존을 제거하고, useState 의 초기값을 빈 배열로 바꿨습니다.
  • useEffect 안에서 GET 요청을 호출하여, 컴포넌트가 마운트될 때 한 번 음식점 목록을 불러옵니다.
  • 추가 시에는 POST 요청을 보낸 뒤, GET 함수를 다시 호출(refetch)해서 목록을 최신 상태로 동기화했습니다.
  • GET/POST 함수를 각각 분리해두어, 추가 후 refetch 가 자연스럽게 재사용되도록 했습니다.
  • API URL 은 BASE_URL 상수로 분리했습니다.

기타 변경사항

  • 모달 state 이름이 모호했던 isModalOpenisDetailModalOpen 으로 변경하여, 새로 추가한 isAddModalOpen 과 짝이 맞도록 정리했습니다.
  • 4단계 진행을 위해 임시로 <div className="modal"> 로 닫혀 있던 AddRestaurantModal 을 modal modal--open 으로 다시 열었습니다. 이제는 App 에서 조건부 렌더링으로 열림/닫힘을 제어합니다.

진행하면서 마주친 부분

  • <option value="..."> 가 있는데 왜 uncontrolled 인지 헷갈렸습니다: <select> 안의 option 마다 value 속성이 있길래 "이미 controlled 아닌가?" 생각이 들었습니다. option 의 value 는 HTML 표준 속성으로 "이 옵션이 선택되면 select 가 가질 값" 을 정의하는 것이고, controlled 여부는 <select> 자체에 value + onChange 가 붙었느냐로 판단한다고 들었는데, 같은 이름이라 그런지 솔직히 아직도 머릿속에서 깔끔하게 정리되지는 않습니다. 이 부분은 리뷰어님 피드백을 받아보고 싶습니다.
  • useEffect 의존성 배열 [] 의 의미: 처음에는 왜 빈 배열을 넘기는지 의아했는데, 이게 "마운트 시 딱 한 번만 실행" 이라는 신호라는 걸 알게 되었습니다. 의존성 배열을 아예 생략하면 매 렌더마다 effect 가 다시 실행되고, 배열 안에 값을 넣으면 그 값이 바뀔 때마다 다시 실행된다는 차이도 같이 정리되었습니다. GET 요청처럼 한 번만 실행하면 되는 작업에는 [] 를 쓴다는 패턴이 이번 미션에서 가장 인상 깊었습니다.
  • useEffect 같은 로직을 나중에 hooks/ 폴더로 분리한다는 얘기를 들었습니다: 그러면 지금 App 안에 같이 있는 GET / POST 함수도 같이 그쪽으로 옮기는 건지, 아니면 useEffect 만 분리하고 fetch 함수는 따로 두는 건지 궁금합니다. 보통 어떤 기준으로 나누는지 알려주시면 다음 미션에서 적용해보고 싶습니다.
  • 공통 Modal 컴포넌트 리팩토링 (children props) 은 이번에 적용하지 못했습니다. 다음 단계로 미뤄두었습니다.

리뷰를 통해 부족한 부분을 개선하고 싶습니다. 잘 부탁드립니다!


Directory Structure

스크린샷 2026-05-04 오후 11 31 58

Image

스크린샷 2026-05-04 오후 11 36 24 스크린샷 2026-05-04 오후 11 36 50

Video

2026-05-04.11.33.22.mov

EM-H20 added 22 commits April 6, 2026 18:55
- isAddModalOpen state 추가 및 Header onAddClick 연결
- 기존 isModalOpen → isDetailModalOpen 으로 이름 변경
- 모달 조건부 렌더링을 삼항 연산자로 변경
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements a restaurant management system featuring category filtering and modal-based interfaces for viewing details and adding new entries, integrated with a backend API. Feedback focuses on improving code maintainability and performance, specifically by refining state setter naming, addressing useEffect dependency issues, optimizing filtering logic with useMemo, and removing unused props. Additionally, it is recommended to delegate ID generation to the server and externalize hardcoded category strings into constants for better scalability.

Comment thread src/App.jsx Outdated
return <h1>Self-Paced React</h1>;
const [isAddModalOpen, setIsAddModalOpen] = useState(false);
const [category, setCategory] = useState("전체");
const [isDetailModalOpen, setIsModalOpen] = useState(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

상태 변수 isDetailModalOpen에 대응하는 setter 함수의 이름을 setIsDetailModalOpen으로 변경하는 것을 권장합니다. 현재 setIsModalOpenisAddModalOpen과 혼동될 수 있으며, 명확한 이름을 사용하는 것이 코드의 가독성과 유지보수 측면에서 더 좋습니다.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

잼민이가 잘 짚어줬네요 ㅎㅎ isDetailModalOpen 으로 상태 변수를 변경함으로써 setter 함수 이름도 setIsDetailModalOpen으로 변경해주세요!

Comment thread src/App.jsx
Comment on lines +21 to +23
useEffect(() => {
fetchRestaurants();
}, []); //초기만 불러오도록 빈 배열
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

useEffect 내에서 컴포넌트 스코프의 함수인 fetchRestaurants를 호출하고 있지만, 의존성 배열이 비어 있습니다. 이는 ESLint의 exhaustive-deps 경고를 발생시킬 수 있으며, 잠재적인 버그의 원인이 됩니다. fetchRestaurantsuseCallback으로 감싸고 의존성 배열에 추가하거나, 함수 정의를 useEffect 내부로 옮기는 것을 권장합니다.

Comment thread src/App.jsx Outdated
body: JSON.stringify(restaurant),
});
const newRestaurant = await response.json();
setRestaurants((prev) => [...prev, newRestaurant]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

설계 문서(docs/[4,5 미션]-설계.md)의 23번 라인에는 '성공하면 모달 닫고 GET 다시 호출(refetch)' 한다고 명시되어 있으나, 현재 코드는 로컬 상태를 직접 업데이트하고 있습니다. 상태를 직접 업데이트하는 것이 효율적일 수 있지만, 서버에서 생성되는 데이터와의 완전한 동기화를 위해 설계대로 fetchRestaurants()를 호출하는 것이 더 안전할 수 있습니다. 의도된 변경이라면 설계 문서를 업데이트해 주세요.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

잼민이가 리뷰를 잘해주네요 ㅋㅋㅋㅋㅋㅋ ㅜㅜ 설계 문서에 작성한 것과는 상이한 점이 위와 같이 존재하고 있어요. 작성한 코드에는 언제나 의도가 존재한다고 생각하기에, 의민님이 작성해주신 설계 의도와 동일하게 흘러갈 수 있도록 수정해주세요!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

const newRestaurant = await response.json(); setRestaurants((prev) => [...prev, newRestaurant]);

생각해보니 서버에서 생성되는 데이터와의 동기화 측면에서는 POST 응답 값으로 새 배열을 만드는 것 보다

await fetchRestaurants(); 이렇게 해서 서버에서 직접 가져오는 방식이 더 안전한 방식 같네요!

수정하겠습니다!

Comment thread src/App.jsx
Comment on lines +51 to +54
const filteredRestaurants =
category === "전체"
? restaurants
: restaurants.filter((restaurant) => restaurant.category === category);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

filteredRestaurantsrestaurantscategory가 변경될 때만 다시 계산되면 됩니다. 현재는 컴포넌트가 렌더링될 때마다 매번 필터링 로직이 실행되므로, useMemo를 사용하여 불필요한 계산을 방지하는 최적화를 권장합니다.

Comment thread src/App.jsx Outdated
Comment on lines +78 to +82
restaurants={restaurants}
onAddRestaurant={addRestaurant}
onClose={() => setIsAddModalOpen(false)}
/>
) : null}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

AddRestaurantModal 컴포넌트에 restaurants prop을 전달하고 있지만, 해당 컴포넌트 내부에서는 이 prop을 사용하지 않고 있습니다. 불필요한 prop 전달은 제거하여 컴포넌트 간의 의존성을 줄이는 것이 좋습니다.

          <AddRestaurantModal
            onAddRestaurant={addRestaurant}
            onClose={() => setIsAddModalOpen(false)}
          />

Comment on lines +11 to +16
const restaurant = {
id: Date.now(),
category: formData.get("category"),
name: formData.get("name"),
description: formData.get("description"),
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

id를 클라이언트에서 Date.now()로 생성하고 있습니다. 현재 db.json의 기존 데이터들은 문자열 ID를 사용하고 있어 타입 불일치가 발생할 수 있습니다. 또한 json-server는 POST 요청 시 id를 생략하면 자동으로 고유한 ID를 생성해 주므로, 서버의 기능을 활용하는 것이 더 안전하고 깔끔합니다.

            const restaurant = {
              category: formData.get("category"),
              name: formData.get("name"),
              description: formData.get("description"),
            };

Comment thread src/components/AddRestaurantModal.jsx Outdated
Comment on lines +27 to +32
<option value="한식">한식</option>
<option value="중식">중식</option>
<option value="일식">일식</option>
<option value="양식">양식</option>
<option value="아시안">아시안</option>
<option value="기타">기타</option>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

카테고리 목록(한식, 중식 등)이 여러 컴포넌트에 하드코딩되어 있습니다. 이를 별도의 상수 파일로 분리하여 관리하면, 나중에 카테고리가 추가되거나 변경될 때 한 곳만 수정하면 되므로 유지보수가 훨씬 용이해집니다.

Copy link
Copy Markdown

@gxuoo gxuoo left a comment

Choose a reason for hiding this comment

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

안녕하세요, 의민님 😊
중간고사가 끝났음에도 불구하고 미션 구현하시느라 고생 많으셨어요!!

PR 본문을 보니 이번 미션에서 무엇을 의도했고, 어떤 기준으로 선택했는지가 잘 드러나는 거 같아요. 특히 what, how로 설계 문서를 정리한 점과 이번 단계에서 배운 내용을 스스로 언어화해 본 점이 인상 깊었어요.

코드를 확인하니 React를 거의 안 써본 사람이 구현했다고는 안 믿길 정도로 전반적으로 흠 잡을 곳이 거의 없었던 거 같아요. 상태를 App으로 올리고, Header, Filter, List, Modal로 역할을 나눈 흐름이 React 개발을 이미 해보신 느낌인 거 같았어요!

그래서 코멘트로는 의민님의 코드 작성 의도를 물어보는 질문 몇 가지와 약간의 수정 포인트를 남겨봤어요. 확인해보시고 이야기 나눠봅시다! 아래는 질문 주신 내용에 대한 제 생각을 작성해봤어요!


Uncontrolled vs Controlled, 그리고 <option value> 혼동에 대해

좋은 질문이에요. 같은 value라는 이름을 쓰니까 헷갈리는 게 당연합니다. 이건 두 개의 다른 value가 각각 다른 층위에서 일하고 있다고 보면 깔끔해져요.

1. <option value="..."> — HTML 표준, "이 항목의 식별자"

<option>value는 그 옵션이 선택됐을 때 form에 제출되거나 select.value로 읽히는 데이터 라벨이에요. 사용자에게 보이는 텍스트(<option>한국</option>)와 실제 값(value="KR")을 분리하기 위한 거죠.

React가 등장하기 한참 전부터 있던 순수 HTML 스펙이고, controlled/uncontrolled와는 아무 관련이 없어요. 그냥 "이 옵션의 ID는 뭐다"라고 적어둔 거예요.

2. <select>의 controlled/uncontrolled — React 개념, "상태를 누가 소유하는가"

의민님은 Uncontrolled: 필요할 때만 꺼내봄 vs Controlled: 실시간으로 추적 이라고 작성해주셨는데, 제 생각에는 현재 어떤 값이 선택되어 있는지를 누가 관리하느냐로 구분하면 조금 더 명확할 것 같아요.

  • Uncontrolled: DOM이 자체적으로 "지금 선택된 게 뭔지" 기억하고, React state는 몰라요. 값이 필요할 때 ref로 꺼내봐야 알 수 있죠.

    <select name="category">
  • Controlled: React state가 단일 진실 공급원(single source of truth). <select value={state}>로 "지금 선택돼야 할 값"을 React가 강제하고, onChange로 사용자 입력을 받아 state를 업데이트해요.

    <select value={category} onChange={(e) => setCategory(e.target.value)}>

option의 value선택지들의 정적인 메뉴판이고, select의 value prop은 그 메뉴판에서 지금 어떤 게 골라져 있는지를 React가 통제하느냐의 문제예요. 메뉴판에 가격이 적혀 있다고 해서 손님이 뭘 주문했는지 식당이 아는 건 아니잖아요? option의 value는 메뉴판의 가격표, controlled는 "지금 손님 주문이 뭔지 식당(React)이 들고 있느냐"라고 비유할 수 있겠네요.

그래서 아래 코드는 option에 value가 다 박혀 있어도 여전히 uncontrolled예요. select 자체에 value/onChange가 없으면 React는 지금 뭐가 선택됐는지 모르거든요.

<select>
  <option value="KR">한국</option>
  <option value="US">미국</option>
</select>

정리하자면, <option>value는 controlled든 uncontrolled든 항상 필요한 것으로, "선택됐을 때 어떤 값을 전달할지 정의하는 역할"이라고 생각하시면 헷갈리지 않으실 거예요. controlled 여부는 오직 <select>value + onChange가 붙었느냐로만 결정됩니다!

hooks 폴더 분리 기준

좋은 고민이에요. 결론부터 말씀드리면 useEffect만 따로 옮긴다기보다는, "함께 바뀌는 상태 + effect + 관련 함수"를 하나의 custom hook으로 묶는 경우가 많아요.

보통 이렇게 세 개의 층으로 나눠서 생각하시면 좋아요.

  1. API 호출 함수 — apis/ 또는 services/

순수하게 "서버와 어떻게 통신하느냐"만 담당해요. fetch/axios 호출, URL, 헤더, 요청/응답 형태. React와는 무관해서 hook도 state도 없고, 그냥 Promise를 반환하는 함수예요.

// apis/restaurants.js
export const getRestaurants = () => fetch('/api/restaurants').then(r => r.json());
export const createRestaurant = (data) => fetch('/api/restaurants', { method: 'POST', body: ... });
  1. Custom Hook — hooks/

"컴포넌트가 그 데이터를 어떻게 쓰느냐"를 담당해요. state, useEffect 트리거, 리페치 로직 같은 것들이요. 지금 구조라면 useRestaurants() 같은 훅으로 restaurants state, fetchRestaurants, addRestaurant를 한꺼번에 묶는 쪽이 더 자연스러워요.

// hooks/useRestaurants.js
export const useRestaurants = () => {
  const [restaurants, setRestaurants] = useState([]);

  const fetchRestaurants = async () => {
    const data = await getRestaurants();
    setRestaurants(data);
  };

  const addRestaurant = async (newOne) => {
    await createRestaurant(newOne);
    await fetchRestaurants(); // refetch
  };

  useEffect(() => { fetchRestaurants(); }, []);

  return { restaurants, addRestaurant };
};
  1. Component

UI 렌더링과 사용자 인터랙션만 담당. hook을 호출해서 데이터를 받아 쓰기만 해요. 분리 자체가 목적은 아니에요. 기준은 이 세 가지로 보시면 됩니다.

  • 재사용이 필요한가? 같은 데이터를 다른 컴포넌트에서도 쓰게 될 것 같은가
  • App이 너무 비대해졌는가? 한 컴포넌트가 너무 많은 책임을 지고 있는가
  • 관심사를 분리했을 때 더 읽기 쉬운가? 분리해서 오히려 따라가기 어려워지진 않는가

지금처럼 규모가 작고 App 하나에서만 쓰는 단계에서는 굳이 분리하지 않아도 괜찮아요. 다음 미션에서 컴포넌트가 늘어나거나 같은 API를 여러 곳에서 호출하게 될 때 한번 적용해보시면 감이 잡히실 거예요.


코멘트와 본문에 남겨드린 내용 확인해보시고 또 궁금한 점이나 이야기 나누고 싶은 것들에 대해서 언급해주세요! 미션 구현 고생하셨습니다!

Comment thread src/components/AddRestaurantModal.jsx Outdated
e.preventDefault();
const formData = new FormData(e.target);
const restaurant = {
id: Date.now(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

추가되는 음식점 ID 생성 방식을 Date.now()로 사용하고 계시네요~ 이에 대해 제가 처음에 ID 생성에 있어 접근했던 방식과 의천님에게 받았던 리뷰를 참고해서 말씀드려볼게요!

저는 처음에 id를 음식점 배열의 길이 + 1의 숫자로 생성했다가 이를 문자열로 바꾸고, Date.now() + Math.random() 형식으로 문자열을 생성해서 ID를 구성해보려고 했었어요. 지금 보니까 최대한 충돌을 피하려고 어떻게든 시도를 해본 것 같은데 그래도 확실하게 충돌을 피해야겠죠?

crypto.randomUUID()를 사용하면, 라이브러리 설치가 필요 없고 충돌 확률이 극단적으로 낮은 키를 생성헤요. 어느 정도냐면, 키 값을 100조 개 생성해도 키가 충돌할 확률이 복권 당첨 확률보다 훨씬 낮아요! Date.now()의 경우 키가 충돌하려면 최소 단위인 1ms 안에 키가 두 번 이상 생성되어야 하고, 1ms는 사람에게는 극단적으로 짧은 시간이지만 컴퓨터에게는 그렇지 않기에 crypto.randomUUID()를 사용해서 ID를 생성하는 걸 추천드려요!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

오오 이런게 있군요 ㅋㅋㅋㅋㅎ 바로 적용하겠습니다!

Comment thread src/App.jsx Outdated
return <h1>Self-Paced React</h1>;
const [isAddModalOpen, setIsAddModalOpen] = useState(false);
const [category, setCategory] = useState("전체");
const [isDetailModalOpen, setIsModalOpen] = useState(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

잼민이가 잘 짚어줬네요 ㅎㅎ isDetailModalOpen 으로 상태 변수를 변경함으로써 setter 함수 이름도 setIsDetailModalOpen으로 변경해주세요!

description: formData.get("description"),
};
onAddRestaurant(restaurant);
onClose();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

지금 상태에서 음식점 추가가 실패한다면 어떻게 될까요? docs 폴더에 추 설계 md 파일을 확인해보니 설계처럼 동작하지 않는 경우가 있는 것 같아요! 어떤 이유 때문에 발생하는지 고민해보시겠어요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

앗 그러네요 성공하면 모달을 닫아야하는데 현재는 성공여부에 상관없이 무조건 모달이 닫히네요! onAddRestaurant에 await 넣고 onSubmit을 async로 해서 기다린 다음에 닫아야겠네요. 실패에 대한건 이미 App.jsx에서 try-catch로 잡고있으니까요!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

onSubmit에서 await onAddRestaurant(restaurant) 이후에 onClose()를 호출하도록 수정해주신 방향 좋습니다!

다만 한 가지 더 같이 보면 좋을 부분이 있어요. AddRestaurantModal 입장에서는 onAddRestaurant가 실패하면 await 지점에서 에러가 발생해야 onClose()까지 가지 않을 수 있는데요. 현재 App.jsxaddRestaurant 내부에서 try-catch로 에러를 잡기만 하고 다시 던지지 않으면, 호출하는 쪽에서는 실패 여부를 알 수 없어서 결국 모달이 닫힐 수 있어요.

즉, “실패하면 모달을 닫지 않는다”는 흐름까지 보장하려면 catch에서 에러를 다시 throw하거나, addRestaurant가 성공/실패 결과를 반환하도록 만들어서 AddRestaurantModal이 그 결과에 따라 닫을지 결정하게 해볼 수 있을 것 같아요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

음 그러면 App.jsx addRestaurant 안에서

catch (error) { console.error("음식점을 추가하는 중 오류가 발생했습니다.", error); throw error;
이런식으로 에러를 던져야겠군요,,!

Comment thread src/App.jsx Outdated
body: JSON.stringify(restaurant),
});
const newRestaurant = await response.json();
setRestaurants((prev) => [...prev, newRestaurant]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

잼민이가 리뷰를 잘해주네요 ㅋㅋㅋㅋㅋㅋ ㅜㅜ 설계 문서에 작성한 것과는 상이한 점이 위와 같이 존재하고 있어요. 작성한 코드에는 언제나 의도가 존재한다고 생각하기에, 의민님이 작성해주신 설계 의도와 동일하게 흘러갈 수 있도록 수정해주세요!

Comment thread src/App.jsx Outdated
) : null}
{isAddModalOpen ? (
<AddRestaurantModal
restaurants={restaurants}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

해당 props는 어떤 역할을 하고 있나요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

필요할거 같아서 초기 구현시점에 넣었다가 결국 안 쓴 케이스입니다 ㅠㅠ.

삼항 연산자 falsy 값 처리 이슈처럼, 이번 케이스도 제가 자주 놓치는 부분 중 하나인데
다음부터는 더 세밀히 살펴보겠습니다!

Comment on lines 25 to 33
<select name="category" id="category" required>
<option value="">선택해 주세요</option>
<option value="한식">한식</option>
<option value="중식">중식</option>
<option value="일식">일식</option>
<option value="양식">양식</option>
<option value="아시안">아시안</option>
<option value="기타">기타</option>
</select>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이전에 미션을 진행하면서 constants 폴더에 음식점 목록을 분리한 것처럼 카테고리도 분리하면 어떨까요? CategoryFilter.jsx 파일에서도 카테고리 값이 사용되는 것 같은데, 만약 하나의 카테고리가 추가되거나 삭제, 수정하는 상황이 발생하면 카테고리를 사용하고 있는 모든 코드를 수정해야겠죠?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

오 좋은거 같습니다! 그렇게 분리하면 카테고리가 변경될 때 한 곳만 고치면 되고, 나중에 백엔드에서 카테고리 값을 내려주는 구조로 바뀌어도 쉽게 연결할 수 있겠네요!

Comment on lines 7 to 13
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍👍👍

Comment thread src/App.css
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CSS 파일을 하나의 App.css로 불러오고 있는 이유가 있으실까요?? 이전 1,2,3단계 리뷰를 보니까 의천님이 module.css 에 대해서 언급을 해주신 것 같은데 아직 사용하지는 않으신 거 같아서 여쭤봅니다!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

이것도 이번에 적용해서 같이 올리겠습니다!

Comment thread src/App.jsx Outdated
Comment on lines +8 to +9
// 5단계 미션 때문에 상수파일 의존성 삭제.
// import { restaurants as initialRestaurants } from "./constants/restaurants";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

사용하지 않으면 주석처리 보다는 과감하게 삭제하셔도 좋을 거 같아요~

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

넵 삭제하겠습니다.

Copy link
Copy Markdown

@gxuoo gxuoo left a comment

Choose a reason for hiding this comment

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

src/constants 폴더 내부에 RestaurantList.jsx, restaurants.js 2가지 파일이 더 이상 사용되지 않고 있어요. 해당 파일은 삭제해주세요!

추가적으로 코멘트 달아드렸으니 확인해주세요~~

description: formData.get("description"),
};
onAddRestaurant(restaurant);
onClose();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

onSubmit에서 await onAddRestaurant(restaurant) 이후에 onClose()를 호출하도록 수정해주신 방향 좋습니다!

다만 한 가지 더 같이 보면 좋을 부분이 있어요. AddRestaurantModal 입장에서는 onAddRestaurant가 실패하면 await 지점에서 에러가 발생해야 onClose()까지 가지 않을 수 있는데요. 현재 App.jsxaddRestaurant 내부에서 try-catch로 에러를 잡기만 하고 다시 던지지 않으면, 호출하는 쪽에서는 실패 여부를 알 수 없어서 결국 모달이 닫힐 수 있어요.

즉, “실패하면 모달을 닫지 않는다”는 흐름까지 보장하려면 catch에서 에러를 다시 throw하거나, addRestaurant가 성공/실패 결과를 반환하도록 만들어서 AddRestaurantModal이 그 결과에 따라 닫을지 결정하게 해볼 수 있을 것 같아요.

<h2 className="modal-title text-title">{restaurant.name}</h2>
<div className="restaurant-info">
<p className="restaurant-info__description text-body">
<div className={`${styles.modal} ${styles.open}`}>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AddModal과 DetailModal의 구조가 공통점을 꽤나 많이 갖고 있는 거 같아요. 설계 문서에도 공통 Modal 컴포넌트를 사용하는 방향이 있었는데요. 공통 Modal이 레이아웃과 닫기 동작을 담당하고, 각 모달은 내부 콘텐츠만 children으로 넘기도록 나누면 중복을 줄일 수 있을 것 같아요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

오 그렇게 하면, 유지보수도 쉬워지고 좋을거 같네요! 추가해보겠습니다!!

Comment on lines +11 to +21
onSubmit={async (e) => {
e.preventDefault();
const formData = new FormData(e.target);
const restaurant = {
id: crypto.randomUUID(),
category: formData.get("category"),
name: formData.get("name"),
description: formData.get("description"),
};
await onAddRestaurant(restaurant);
onClose();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

약간 부가적인 요소라고 생각하는데, AddRestaurantModalonSubmit 안에서 FormData 생성, restaurant 객체 생성, 추가 요청, 모달 닫기까지 모두 처리하고 있는데요.

동작 자체는 문제 없지만 JSX 안의 이벤트 핸들러가 길어지면 렌더링 구조와 비즈니스 흐름을 한눈에 구분하기 어려워질 수 있습니다. handleSubmit 같은 함수로 분리하면 JSX는 UI 구조에 집중하고, 제출 로직은 함수 이름으로 의도를 드러낼 수 있을 것 같아요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

오오오 좋은거 같습니다!! 그렇게 하면 코드 가독성도 더 좋아질 거 같아요 추가하겠습니다!!

Comment thread src/App.jsx
});
await fetchRestaurants();
} catch (error) {
console.error("음식점을 추가하는 중 오류가 발생했습니다.", error);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

console.error로 에러를 보여주는 건 개발자의 영역에서만 유효하다고 생각해요. 사용자가 항상 콘솔을 열어서 보고 있는 건 아니니, 사용자에게 에러가 났음을 알려줄 수 있는 장치가 추가되면 좋을 거 같아요!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

위 코멘트에서
catch (error) { console.error("음식점을 추가하는 중 오류가 발생했습니다.", error); throw error;
이런식으로 한다 했는데 생각해보니 App.jsx에서
throw new Error("에러메세지")
이렇게 주고 AddRestaurantModal의 onSubmit 에서 error.message 형식으로 주면 되겠네요!

Comment thread src/App.jsx
}
};

const addRestaurant = async (restaurant) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

addRestaurant, fetchRestaurant 두 코드 모두 response에 대한 체크를 진행하지 않는 것 같아요. 이렇게 되면 만약 서버에서 400이나 500을 응답해도 fetch 자체는 resolve 될 수 있어서, response.ok를 확인하지 않으면 실패한 요청도 성공처럼 처리될 수 있어요. 아래의 예시를 참고하면 좋을 거 같아요~

const response = await fetch(BASE_URL, { ... });

if (!response.ok) {
  throw new Error("음식점 추가에 실패했습니다.");
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

이 패턴 참고해서 해보겠습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants