[4,5단계미션] 고규민 미션 제출합니다#97
Conversation
- 식당 및 카테고리 데이터를 RestaurantData.jsx로 분리 - AddRestaurantModal의 카테고리 선택문을 .filter().map()을 활용하여 개선 - 상수(CATEGORY_IMAGE)와 Props(setCategory) 명칭을 컨벤션에 맞춰 수정
There was a problem hiding this comment.
Code Review
This pull request implements a restaurant management application in React, including features for listing, filtering by category, viewing details, and adding new restaurants. It introduces several UI components such as Header, Modal, CategoryFilter, and RestaurantList, along with global CSS styles and initial data. The reviewer provided feedback regarding the lack of server-side persistence for adding restaurants, the need to reuse the common Modal component in the detail view, and the benefit of using centralized constants for categories. Suggestions were also made to improve prop naming for clarity and to fix a minor syntax error in a React Fragment.
| const handleAddRestaurant=(newRestaurant)=>{ | ||
| setRestaurants((prev)=>[...prev, newRestaurant]); | ||
| }; |
There was a problem hiding this comment.
fetch의 POST 메서드를 사용하여 사용자가 입력한 데이터를 JSON 형태로 서버에 전송합니다. 이때 서버와의 통신이 완료될 때까지 await로 기다린 후, 서버가 생성해준 id가 포함된 응답 데이터를 받아오도록 구현했습니다!
| export default function RestaurantDetailModal({restaurants, setIsDetailModalOpen}){ | ||
| return( | ||
|
|
||
| <div className="modal modal--open"> | ||
| <div className="modal-backdrop" | ||
| onClick={setIsDetailModalOpen}></div> | ||
| <div className="modal-container"> | ||
| <h2 className="modal-title text-title">{restaurants.name}</h2> | ||
| <div className="restaurant-info"> | ||
| <p className="restaurant-info__description text-body">{restaurants.description}</p> | ||
| </div> | ||
| <div className="button-container"> | ||
| <button className="button button--primary text-caption" | ||
| onClick={setIsDetailModalOpen}>닫기</button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
PR 설명에서 언급하신 공통 Modal 컴포넌트가 RestaurantDetailModal에서는 사용되지 않고 있습니다. 코드 중복을 줄이고 일관된 모달 구조를 유지하기 위해 Modal 컴포넌트를 재사용하도록 리팩터링하는 것을 권장합니다.
References
- 컴포넌트 재사용성을 높이고 중복 코드를 제거하는 것은 유지보수성을 향상시킵니다. (link)
| import "./CategoryFilter.css" | ||
|
|
||
| const CATEGORYS=["전체", "한식", "중식", "일식", "양식", "아시안", "기타"]; |
There was a problem hiding this comment.
CATEGORYS 배열이 컴포넌트 내부에 하드코딩되어 있습니다. RestaurantData.js에 정의된 CATEGORY_LIST를 활용하면 카테고리 정보가 변경되었을 때 한 곳에서 관리할 수 있어 유지보수에 유리합니다.
References
- 상수 데이터는 별도의 파일에서 관리하여 데이터 일관성을 유지해야 합니다. (link)
There was a problem hiding this comment.
Gemini 코멘트대로, 한 곳에 상수를 선언하고 여러 곳에서 재사용하면 관리하기 더 용이할 것 같아보여요!
현재 코드에서는 CATEGORYS, CATEGORY_LIST 와 같이 여러 곳에서 각각 다른 이름으로 사용되고 있는데, 이 부분도 통일되면 더 좋을 것 같아요
There was a problem hiding this comment.
말씀하신 대로 하드코딩된 배열을 지우고 RestaurantData.js 상수를 가져와 사용하도록 수정했습니다!
| restaurants={selectedRestaurant} | ||
| setIsDetailModalOpen={()=>setIsDetailModalOpen(false)}/>} |
There was a problem hiding this comment.
prop명이 setIsDetailModalOpen 이면, 의미상 저는 "모달이 열려있는 지의 여부를 변경하는 함수" 라는 생각이 드는 것 같아요. 실제로 <App> 에 있는 setIsAddModalOpen 함수도 그렇고요.
그런데, 내부에 들어가는 함수는 () => setIsDetailModalOpen(false) 인지라 사실상 "모달을 닫는 함수" 가 되고, 이어받은 자식 컴포넌트인 <RestaurantDetailModal> 입장에서는 동일한 이름의 함수를 받는데, 막상 매개변수는 넣을 수 없고, 호출되기만 하면 무조건 모달이 닫히도록 작동하니 혼란스러울 것 같다는 생각이 들었어요.
두 사용처의 의미를 통일시켜보는 것은 어떨까요? 예를 들어 부모가 받은 setter 함수를 그대로 자식에게 넘기고 싶었던 경우라면 화살표 함수로 한 번 감싸지 않고 setter 자체를 넘길 수 있어 보이고, 정말로 모달을 닫는 함수를 받도록 고안된 것이라면 prop명을 onClose 등의 네이밍으로 정해볼 수 있어보입니다.
There was a problem hiding this comment.
처음에는 데이터(category)처럼 상태와 프롭명을 일치시키는 것이 흐름을 추적하기에 더 직관적이라 생각해서 통일했었습니다.
하지만 주신 피드백을 바탕으로 학습해 보니, 데이터와 달리 '닫기' 같은 '동작'은 자식 컴포넌트가 부모의 내부 상태 명칭(setIsAddModalOpen)까지 알 필요가 없다는 점을 알았습니다. 마치 리모컨 버튼에 '거실 에어컨 전원 끄기'라고 적기보다 그냥 '전원'이라고 적어야 어디서든 쓸 수 있는 것과 같은 원리더군요!
덕분에 자식 컴포넌트의 독립성과 재사용성을 고려하여, 동작 중심의 onClose라는 네이밍으로 리팩터링했습니다. 좋은 관점을 공유해주셔서 감사합니다!
|
|
||
| export default function Modal({ title, children, onClose }) { | ||
| return ( | ||
| < > |
There was a problem hiding this comment.
React Fragment(< >) 사용 시 불필요한 공백이 포함되어 있습니다. 표준 문법인 <>로 작성하는 것이 가독성과 일관성 측면에서 좋습니다.
References
- 표준적인 React 문법과 스타일 가이드를 준수해야 합니다. (link)
There was a problem hiding this comment.
추가로 코드를 작성하다 보면 줄바꿈, 공백, 탭 등 포매팅 면에서 모든 부분을 신경쓰기 어렵다 보니 놓치는 경우가 생길 것이고, 추가로 불필요한 공백이 섞여 있어 어지럽혀진 코드를 정리하기 위한 시간도 들 수 있다고 생각해요.
매번 꼼꼼하게 신경쓰는 것도 물론 좋지만 근본적으로 실수를 하지 않을 수 있는 환경을 만드는 것도 저는 중요하다 생각하는데요, 혹시 이런 작업을 도와줄 기술이나 라이브러리 어디 없을까요?
There was a problem hiding this comment.
리뷰어님 말씀대로 포매팅에 쏟는 에너지를 줄이고 실수를 방지하는 환경을 만드는 게 정말 중요한 것 같습니다. Prettier를 도입하고 Format on Save 설정까지 마쳤습니다!
There was a problem hiding this comment.
@kokunut 👋🏻
안녕하세요 규민님, 오랜만이네요!
React가 많이 생소하죠. 특별한 방식이 꽤 많이 생기는 것 같기도 하고, UI면 UI지 뭔 계층이 이렇게 생기냐... 하는 생각이 저는 입문할 당시에 들어요.
생소해 하시면서도 계속해서 궁금하신 점이나 의문 드시는 점을 많이 탐구하려 하신 것 같기도 하고요! 학습하신 내용들 많이 적어주셔서 어떤 것들을 접하고 계신지 덕분에 잘 보이고, 이에 대해서 이야기를 많이 해볼 수 있을 것 같아요.
앱을 켜면 서버에서 목록을 가져오고, 새 식당을 추가하면 서버에 저장하게 만들어 새로고침을 해도 데이터가 사라지지 않습니다.
우선은 매우 중요한 부분인데 현재 새 음식점을 추가했을 때 서버에 저장이 되고 있지 않은 것 같아요. 코드를 살펴보았는데 아예 POST 요청이 보이지 않더라고요. 클라이언트 자체는 음식점이 추가될 때마다 UI에는 반영되고 있어서 추가된 것처럼 보이기 쉬워서 특히 주의할 부분이라 생각해요.
모달을 닫는 기능을 만들다가 화면이 멈추는 에러를 만났는데, 알고 보니 함수를 넘겨줄 때 뒤에 괄호를 붙여서 작성했던 게 문제였습니다. 왜 이런 에러가 나는지 찾아보니 괄호를 붙이는 순간 "나중에 실행해"가 아니라 "지금 당장 실행해"라는 명령이 되어버렸습니다. 렌더링되자마자 부모 상태를 바꾸고, 다시 리렌더링이 일어나면서 또 함수가 실행되는 무한 루프 과정을 직접 겪어보며 함수 참조의 중요성을 배웠습니다.
오, 좋은 경험 하셨습니다. myFunc와 myFunc() 가 비슷해보이지만 정말 많이 다릅니다. 차이는 이겁니다: 함수 그 자체를 넘기느냐, 함수를 실행한 결과를 넘기느냐. 이 차이를 이제 아시니 함수를 훨씬 더 잘 응용하실 수 있을 겁니다. 예를 들면 함수들을 매개변수로 주입받아 콜백 함수로 실행한다든지요.
처음에는 모달을 띄울 때 CSS에 display: none을 적어두고 특정 클래스를 붙여서 보여주려 했습니다. 그런데 리액트에서 {isOpen && } 같은 방식을 쓰니까, CSS에서 억지로 숨기거나 보여주는 코드가 전혀 필요 없다는 걸 알게 되었습니다. 리액트는 상태에 따라 아예 요소를 DOM에 넣었다가 뺏다가 하기 때문입니다. 그래서 CSS의 복잡한 오픈/클로즈 설정을 다 지우고도 코드가 훨씬 깔끔하게 돌아가는 걸 보며, 리액트가 화면을 제어하는 방식이 자바스크립트와 어떻게 다른지 확실히 느낄 수 있었습니다.
Q. CSS를 사용하는 방식은 React의 conditional rendering이라 불리는 condition && <Component /> 와 같은 형태보다 안 좋은 방식인 것일까요?
조금 번거롭더라도 사용자에게 바로 반응하는 웹을 만들려면 리액트 상태로 관리하는 게 맞다는 것을 알게 되었습니다.
Q. 비제어 컴포넌트는 권장되지 않는 방식인 것일까요?
고민해 보시고 다음 리뷰요청 시 함께 고민해서 규민님의 생각을 들려주시면 좋을 것 같습니다! 이번 주차도 화이팅입니다
| restaurants={selectedRestaurant} | ||
| setIsDetailModalOpen={()=>setIsDetailModalOpen(false)}/>} |
There was a problem hiding this comment.
prop명이 setIsDetailModalOpen 이면, 의미상 저는 "모달이 열려있는 지의 여부를 변경하는 함수" 라는 생각이 드는 것 같아요. 실제로 <App> 에 있는 setIsAddModalOpen 함수도 그렇고요.
그런데, 내부에 들어가는 함수는 () => setIsDetailModalOpen(false) 인지라 사실상 "모달을 닫는 함수" 가 되고, 이어받은 자식 컴포넌트인 <RestaurantDetailModal> 입장에서는 동일한 이름의 함수를 받는데, 막상 매개변수는 넣을 수 없고, 호출되기만 하면 무조건 모달이 닫히도록 작동하니 혼란스러울 것 같다는 생각이 들었어요.
두 사용처의 의미를 통일시켜보는 것은 어떨까요? 예를 들어 부모가 받은 setter 함수를 그대로 자식에게 넘기고 싶었던 경우라면 화살표 함수로 한 번 감싸지 않고 setter 자체를 넘길 수 있어 보이고, 정말로 모달을 닫는 함수를 받도록 고안된 것이라면 prop명을 onClose 등의 네이밍으로 정해볼 수 있어보입니다.
| import "./CategoryFilter.css" | ||
|
|
||
| const CATEGORYS=["전체", "한식", "중식", "일식", "양식", "아시안", "기타"]; |
There was a problem hiding this comment.
Gemini 코멘트대로, 한 곳에 상수를 선언하고 여러 곳에서 재사용하면 관리하기 더 용이할 것 같아보여요!
현재 코드에서는 CATEGORYS, CATEGORY_LIST 와 같이 여러 곳에서 각각 다른 이름으로 사용되고 있는데, 이 부분도 통일되면 더 좋을 것 같아요
| required | ||
| > | ||
| <option value="">선택해 주세요</option> | ||
| {CATEGORY_LIST.filter((category) => category !== "전체").map( |
There was a problem hiding this comment.
여기에 쓰인 CATEGORY_LIST는 아래와 같기 때문에
export const CATEGORY_LIST = ["한식", "중식", "일식", "양식", "아시안", "기타"];category !== "전체" 로 필터링하는 의미가 사실상 없어요! 이전 코멘트와 이어지는 건데, 그래서 한 곳에서 선언하고 관리하는 방식이거나, "전체"가 있는 리스트인지 이름만 봐도 구분되면 좋을 것 같아요
코드란 건 조금만 커져도 헷갈리기 워낙 쉬워지는 것 같아요
There was a problem hiding this comment.
RestaurantData.js 한 곳에서 상수를 선언하고 관리하는 방식으로 수정했습니다. 상수에 '전체'가 포함되지 않은 것을 확인하고 의미 없던 filter 로직을 제거했습니다!
| useEffect(() => { | ||
| fetchRestaurants(); | ||
| }, []); |
There was a problem hiding this comment.
useEffect를 사용하지 않고 그냥fetchRestaurants()를 해당 라인에 적는 것은 안 되려나요?useEffect를 이곳에 사용해주신 이유는 무엇인가요?useEffect의 두 번째 인자에 오는[]는 무엇을 의미할까요?
There was a problem hiding this comment.
- useEffect를 사용하지 않고 그냥 fetchRestaurants()를 해당 라인에 적는 것은 안 되려나요? useEffect를 이곳에 사용해주신 이유는 무엇인가요?
답변:
리뷰어님 말씀대로 useEffect 없이 fetchRestaurants()를 바로 적게 되면, '렌더링 → 데이터 호출 → 상태 업데이트 → 재렌더링'이 끝없이 반복되는 무한 루프에 빠질 수 있다고 알고 있습니다. 이를 방지하고 데이터 호출 시점을 명확하게 하기 위해 useEffect를 사용했습니다.
- useEffect의 두 번째 인자에 오는 []는 무엇을 의미할까요?
답변:
[]은 의존성 배열로 이 로직을 언제 다시 실행할 것인지 결정하는 기준이라고 학습했습니다. []을 사용하면 컴포넌트가 처음 나타날 때 딱 한 번만 실행되는데, 서비스 시작 시 데이터를 한 번만 가져오면 되는 현재 상황에 가장 적합한 방식이라고 생각했습니다. 그리고 두번째 인자에 []이 없어도 리렌더링할 때마다 추가작업이 진행되어 무한 루프에 빠질 수 있다는 점도 학습했습니다!
|
|
||
| export default function Modal({ title, children, onClose }) { | ||
| return ( | ||
| < > |
There was a problem hiding this comment.
추가로 코드를 작성하다 보면 줄바꿈, 공백, 탭 등 포매팅 면에서 모든 부분을 신경쓰기 어렵다 보니 놓치는 경우가 생길 것이고, 추가로 불필요한 공백이 섞여 있어 어지럽혀진 코드를 정리하기 위한 시간도 들 수 있다고 생각해요.
매번 꼼꼼하게 신경쓰는 것도 물론 좋지만 근본적으로 실수를 하지 않을 수 있는 환경을 만드는 것도 저는 중요하다 생각하는데요, 혹시 이런 작업을 도와줄 기술이나 라이브러리 어디 없을까요?
| setSelectedRestaurant(item); | ||
| setIsModalOpen(true); | ||
| const handleAddRestaurant=(newRestaurant)=>{ | ||
| setRestaurants((prev)=>[...prev, newRestaurant]); |
There was a problem hiding this comment.
특히 데이터를 업데이트할 때 정보가 꼬이지 않도록 prev를 활용해 안전하게 상태를 변경하는 법을 익혔습니다.
그런데, 이렇게 작성해도 작동하지 않나요? "정보가 꼬이지 않도록" 은 무엇을 의미할까요? 🤔
const handleAddRestaurant = (newRestaurant) => {
setRestaurants([...restaurants, newRestaurant]);
};There was a problem hiding this comment.
질문 주신 내용을 보고 다시 한번 깊게 찾아보았습니다! 단순히 [...restaurants, newRestaurant]를 사용해도 당장은 문제가 없어 보이지만, 리액트의 상태 업데이트가 비동기적으로 일어난다는 점이 핵심이었습니다.
"정보가 꼬인다"는 것은 상태 업데이트가 비동기적으로 처리되기 때문에 발생할 수 있는 문제를 의미합니다. 예를 들어 사용자가 추가 버튼을 매우 빠르게 여러 번 누를 경우, 리액트가 첫번째 클릭에 대한 상태 업데이트를 완료하기도 전에 두 번째 업데이트가 시작될 수 있습니다. 이때 setRestaurants([...restaurants, ...]) 방식을 쓰면 두 번째 작업이 아직 업데이트되지 않은 옛날 데이터를 기준으로 새로운 리스트를 만들어버려, 결국 첫 번째 추가했던 데이터가 사라져버릴 수 있습니다.
또, prev를 사용하면 큐를 통해 가장 최신 상태값을 받아오기 때문에, 데이터가 누락되지 않는다는 점도 학습했습니다.
덕분에 제가 작성한 코드를 다시 한번 꼼꼼히 뜯어보며 왜 이렇게 사용해야 하는지 더 자세히 공부할 수 있는 계기가 되었습니다. 감사합니다!
답변: 물론 서서히 사라지는 애니메이션처럼 화면에 요소가 잠시 머물러야 하는 특수한 상황에서는 CSS 방식이 더 유리할 수도 있겠지만, 기능이 복잡해질수록 리액트처럼 확실하게 요소를 없애주는 방식이 나중에 코드를 다시 볼 때도 훨씬 명확하고 좋은 방식인 것 같습니다!
답변: 비제어 방식은 값을 나중에 한 번에 가져올 때 코드가 짧아진다는 장점이 있지만, 입력창이 비어있을 때 버튼을 바로 막거나 실시간 글자 수를 체크하는 기능을 만들기에는 좋지 않다고 생각했습니다. 반면 제어 방식은 실시간 피드백을 주기에 훨씬 유리해서 더 좋은 방식이라고 생각했습니다. 상황에 따라 입력만 받고 끝나는 아주 단순한 폼이라면 비제어 방식을 써볼 수도 있겠지만, 사용자에게 바로바로 반응하는 웹을 만든다면 제어 방식을 더 우선적으로 고려할 것 같습니다!
리뷰어님, 상세한 가이드 정말 감사합니다! 사실 처음 PR을 제출할 때 충돌이 발생해서 단순히 pull을 안 해서 생긴 문제라고만 알고 있었는데, 설명을 듣고 나니 원본 저장소(upstream)와 제 포크 저장소 사이의 동기화가 왜 필요한지 정확히 이해하게 되었습니다. 알려주신 대로 꼼꼼하게 알려주신 덕분에 Git 활용 능력이 한 단계 더 성장한 것 같습니다. 앞으로는 새 작업을 시작하기 전에 반드시 동기화부터 진행하는 습관을 들이겠습니다. 확인 부탁드립니다! 감사합니다! |

안녕하세요 김의천 리뷰어님! 프론트엔드 4기 고규민입니다.
벌써 여러 번 뵙게 되는 거 같은데 이번에도 잘 부탁드립니다! ㅎㅎ
이제 리액트를 학습한 지 2주차가 되는데 확실히 써보니 상태만 잘 관리하면 UI가 알아서 그려지는 방식이 확실히 편하게 느껴지는 것 같습니다..! 물론 아직은 생소한 개념이 많아 적응하는 중이라 어렵기도 하지만, 이번에는 코드가 어떻게 돌아가는지 한 줄씩 뜯어보며 코드를 읽는 데에 집중해 보았습니다!
리뷰어님의 피드백을 토대로 많이 고민하고 티키타카하면서 제대로 성장해보고 싶습니다! 감사합니다!
🛠️ 작업 내용
4단계:
추가 모달과 상세 모달에서 겹치는 부분을 Modal.jsx라는 하나의 공통 틀로 만들었습니다. 새로 만든 틀 안에 필요한 내용물만 갈아 끼워 넣는 방식으로 구조를 바꾸니, 모든 모달의 제목과 닫기 기능을 한곳에서 관리할 수 있어 관리하기가 훨씬 수월해졌습니다. 또 부모가 자식에게 보내주는 함수 이름도 용도가 바로 보이게 통일해서 데이터가 어디서 어디로 흐르는지 파악하기 좋게 정리했습니다.
5단계:
json-server를 연결해 데이터가 계속 유지되도록 했습니다. 앱을 켜면 서버에서 목록을 가져오고, 새 식당을 추가하면 서버에 저장하게 만들어 새로고침을 해도 데이터가 사라지지 않습니다. 서버와 대화할 때는 async/await를 사용해 순서대로 작동하게 만들었고, 특히 데이터를 업데이트할 때 정보가 꼬이지 않도록 prev를 활용해 안전하게 상태를 변경하는 법을 익혔습니다.
💡 공부한 내용
모달을 닫는 기능을 만들다가 화면이 멈추는 에러를 만났는데, 알고 보니 함수를 넘겨줄 때 뒤에 괄호를 붙여서 작성했던 게 문제였습니다. 왜 이런 에러가 나는지 찾아보니 괄호를 붙이는 순간 "나중에 실행해"가 아니라 "지금 당장 실행해"라는 명령이 되어버렸습니다. 렌더링되자마자 부모 상태를 바꾸고, 다시 리렌더링이 일어나면서 또 함수가 실행되는 무한 루프 과정을 직접 겪어보며 함수 참조의 중요성을 배웠습니다.
처음에는 모달을 띄울 때 CSS에 display: none을 적어두고 특정 클래스를 붙여서 보여주려 했습니다. 그런데 리액트에서 {isOpen && } 같은 방식을 쓰니까, CSS에서 억지로 숨기거나 보여주는 코드가 전혀 필요 없다는 걸 알게 되었습니다. 리액트는 상태에 따라 아예 요소를 DOM에 넣었다가 뺏다가 하기 때문입니다. 그래서 CSS의 복잡한 오픈/클로즈 설정을 다 지우고도 코드가 훨씬 깔끔하게 돌아가는 걸 보며, 리액트가 화면을 제어하는 방식이 자바스크립트와 어떻게 다른지 확실히 느낄 수 있었습니다.
사실 처음에는 입력창 값을 관리할 때 useRef랑 다를 게 별로 없지 않나 라고 생각했습니다. 타이핑할 때마다 화면을 다시 그리지 않아도 되니 성능 면에서 별 차이가 없을 거라 느꼈습니다. 그런데 이에 대해 궁금해서 찾아보다가 "글자 수를 8자로 제한"하거나 "입력 내용에 따라 버튼을 즉시 활성화"해야 하는 기능에서 확연한 차이가 나타났습니다. useRef는 값이 변해도 화면에 바로 반영되지 않기 때문에 사용자에게 실시간으로 피드백을 줄 수가 없었습니다. 조금 번거롭더라도 사용자에게 바로 반응하는 웹을 만들려면 리액트 상태로 관리하는 게 맞다는 것을 알게 되었습니다. 이 과정에서 ...prev를 써서 이전 데이터를 복사하고 안전하게 업데이트하는 방식도 함께 익혔습니다.
컴포넌트 계층 구조도
video
4.5.mp4