[2단계 미션] 정창우 미션 제출합니다. #46
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ChangwooJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a dynamic restaurant listing feature to the React application. It involves separating restaurant data into a separate file, implementing dynamic rendering of the restaurant list component, and adding category-based filtering. The changes include new components for the header, body, and aside, as well as CSS modules for styling. The author also raises questions about image path management and code organization within the React project.
Highlights
- Dynamic Restaurant Listing: The restaurant list is now dynamically rendered using data from a separate JavaScript file (restaurantList.js), allowing for easier updates and maintenance of restaurant information.
- Category-Based Filtering: A category filter has been implemented, enabling users to filter restaurants based on their category (e.g., 한식, 중식, 일식). This enhances the user experience by allowing them to quickly find restaurants that match their preferences.
- Component Restructuring: The application's structure has been reorganized into components such as Header, Body, and Aside, promoting modularity and separation of concerns. New components like CategorySortFilter and RestaurantList were also created.
- Styling with CSS Modules: CSS Modules have been implemented for styling, providing better encapsulation and preventing naming conflicts. This improves the maintainability and scalability of the application's styles.
Changelog
Click here to see the changelog
- package-lock.json
- Added 'typescript-plugin-css-modules' as a dev dependency.
- Added several new dependencies related to CSS processing, file watching, and other utilities.
- Updated versions of existing dependencies.
- package.json
- Added 'typescript-plugin-css-modules' as a dev dependency.
- src/App.css
- Added global styles for padding, margin, box-sizing, lists, fonts, colors, and typography.
- src/App.jsx
- Imported Header, Body, and Aside components.
- Implemented the basic layout structure with Header, Body, and Aside components.
- src/component/aside/AddRestaurantModal.tsx
- Created a modal component for adding new restaurants, including form elements for category, name, and description.
- src/component/aside/RestaurantInfoModal.tsx
- Created a modal component for displaying restaurant information.
- src/component/body/CategorySortFilter.jsx
- Implemented a category filter component with a select dropdown.
- src/component/body/RestaurantList.jsx
- Created a component to display a list of restaurants, mapping through the restaurant data and rendering each restaurant item.
- src/const/restaurantList.js
- Added a JavaScript file containing an array of restaurant objects with properties like id, name, description, category, and image source.
- src/css/Body.module.css
- Added CSS module styles for the restaurant filter and restaurant list components, including layout, spacing, and typography.
- src/css/Header.module.css
- Added CSS module styles for the header component, including layout, colors, and typography.
- src/css/Modal.module.css
- Added CSS module styles for modal components, including backdrop, container, form elements, and buttons.
- src/css/Text.module.css
- Added CSS module styles for typography, including text sizes, line heights, and font weights.
- src/pages/Aside.jsx
- Imported and rendered RestaurantInfoModal and AddRestaurantModal components.
- src/pages/Body.jsx
- Implemented state management for the selected category.
- Filtered the restaurant list based on the selected category.
- Rendered the CategorySortFilter and RestaurantList components.
- src/pages/Header.jsx
- Created a header component with a title and an add restaurant button.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In React's realm, components rise,
With data flowing, logic applies.
From static lists to dynamic displays,
A filter's touch, a category sways.
CSS Modules, a styling guide,
Encapsulation, with nowhere to hide.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a restaurant listing application with features like category filtering and modal components for restaurant information and adding new restaurants. The code is well-structured and uses React components effectively. However, there are a few areas that could be improved for better maintainability and user experience.
Summary of Findings
- Image Path Handling: The current implementation of image paths in
restaurantList.jsuses relative paths that might break if the folder structure changes. Consider using absolute paths or a more robust asset management solution. - Modal Component State: The modal components (
AddRestaurantModal,RestaurantInfoModal) are currently always open due to themodal--openclass being hardcoded. Implement state management to control modal visibility. - Accessibility Considerations: The header's '음식점 추가' button lacks proper accessibility features. Adding a descriptive
titleattribute or visually hidden text would improve usability for screen reader users.
Merge Readiness
The pull request is not quite ready for merging. The image path handling and modal state management need to be addressed. Additionally, accessibility improvements would enhance the user experience. I am unable to directly approve this pull request, and recommend that these comments be addressed and that others review and approve this code before merging.
There was a problem hiding this comment.
@ChangwooJ 👋🏻
2단계도 이어서 리뷰해 보겠습니다!
1단계 리뷰로 이동하기 ↩
1️⃣ 답변
먼저, 각 필터에 따라 유동적으로 변하는 컴포넌트를 구현하기 위해선 이미지 역시 고정 값일 수 없었습니다.
이에 저는 음식점 리스트를 모아놓은 파일에 img_src로 경로를 추가하여 제공하였으나, 이것이 올바른 방법인지 판단이 되지 않습니다. 틀린 방법은 아니라고 생각되지만, 만약 이미지의 위치가 바뀌게 된다면 유지보수 관점에서 최악이 될 것이라고 생각됩니다.
이런 경우에는 이미지를 경로등을 관리하는 라이브러리, 혹은 폴더 구조를 정의하여 고정되도록 사용하여야하나요?
꽤 고민되는 질문이군요. 라이브러리를 활용한 해결 방법도 있을 것 같지만 저는 거기까지는 모르겠고, 별도의 라이브러리를 사용하지 않고 해결해 볼 만한 방법을 소개해드리려 합니다.
현재 모든 이미지의 경로를 하드코딩으로 적어주신 것을 확인했어요. 이 경우에는 이미지의 경로를 바꾼다면 이 하드코딩된 값도 직접 바꿔주셔야 하므로 번거로울 수 있을 거에요. 이 문제를 말씀해 주신 것이 맞죠? 😄
저는 이 문제를 해결할 만한 핵심적인 방법은 바로 이미지의 경로들을 변수들로 치환하는 것 이라 생각해요.
- 이미지의 경로를 기반으로 불러오는 import문의 경우 이미지의 경로가 변경되면 vscode가 이를 감지해 import문의 경로를 바꿔줄 수 있어요.
- 1.에서 불러온 이미지 경로들은 하드코딩된 값이 아닌 변수의 형태일 거에요. 따라서 이 변수들을 사용해주는 방법을 사용한다면 이미지의 경로가 변경되어도 일일이 경로명을 바꿀 필요가 없어질 거라 생각해요.
이미지 파일들을 어디에서 불러와 사용하실 건지도 고민해 보시면 좋을 것 같아요. 컴포넌트에서 불러올지, 상수에서 불러올 것인지?
아래에 몇 가지 예시를 들어볼게요.
▼ 아래의 컴포넌트의 경우 이미지를 옮겨 경로가 변경되어도 변경된 경로만 바꿔주면 되고, vscode 역시 이에 반응해 경로를 바꿔주므로 이미지의 경로가 빈번하게 변경되어도 유지보수에 편한 컴포넌트가 될 거라 생각해요.
import myImage from '/imagepath';
const MyComponent = () => {
return <img src={myImage} />
};
export default MyComponent;▼ 이미지가 여러 곳에서 사용되어 각 컴포넌트/모듈에서 일일이 하나하나 import하기 어렵다고요? 이미지들을 관리하는 모듈을 따로 만들 수도 있죠.
import cat from './cat.png';
import dog from './dog.png';
import duck from './duck.png';
import rabbit from './rabbit.png';
export { cat, dog, duck, rabbit };이후 필요로 하는 다른 컴포넌트에서 사용할 때는 이렇게 불러와 사용할 수 있어요.
import { cat, dog, duck, rabbit } from '../images/animals';이 정도면 관리가 한결 간편해진 것 같다고 생각하는데, 어떻게 생각하시나요?
다음으로는 제 첫 page폴더의 의도는 하나의 페이지가 가지는 여러가지 컴포넌트를 묶어두는 용도의 파일을 담아두는 폴더였습니다.
그래서 처음에는 Body.jsx에 각 컴포넌트를 임포트 하는 정도로만 작성이 되었는데, 오늘 미션을 진행하면서 각 컴포넌트에 필요한 데이터를 전달하기 위한 코드가 필요해 불가피하게 의도에서 벗어나게 되었습니다.
몇몇 데이터를 가공하는 코드는 또다시 분리하는 리팩토링 과정을 거칠 수는 있지만, 이 역시도 결국 임포트해야하는 상황이 발생합니다.
그렇다면 제 의도대로 되려면, 지금 현 코드에서처럼 바로 Body->RestaurantList or CategorySortFilter 가 아니라 BodyContainer -> Body -> ... 이런 식으로 접근하였어야 할까요? 혹은, 제 의도가 불필요한 낭비로 이어지나요?
확인해 보았는데요, 제가 보았을 때에는 <Body>가 과도한 기능을 담당하고 있는 컴포넌트라고 생각이 들지는 않았습니다! 충분히 깔끔해보여요.
<Body>는<CategorySortFilter>와<RestaurantList>둘 모두를 하위 컴포넌트로 두고 있고,<CategorySortFilter>와<RestaurantList>는 서로의 상태를 알 수가 없어요. 이 경우에는 부모인<Body>가 두 하위 컴포넌트의 상태를 관리하게 하도록 하는 방법을 사용할 수 있어요. 이를 Lifting State Up 라고 해요.- 위 방법대로
<Body>가 전반적인 두 컴포넌트에 대한 데이터 관리를 맡는다면, 자연스럽게 상태를 다루는 로직은<Body>에 위치할 수 밖에 없으며, 이 과정에서 컴포넌트가 맡게 되는 책임이 늘어나는 것은 사실입니다. 하지만 제가 이야기하고 싶은 것은 이것이 의도에 맞지 않는 과도한 책임 이라고 생각하지는 않고 있어요. - UI 관련 로직이 아닌 데이터 및 상태를 다루는 로직(레스트랑 필터링 등)을 분리하고 싶으신 것 또한 이해됩니다. 다만 이 경우라면 저는 컴포넌트를 하나 더 두기보다는 커스텀 훅(Custom hooks) 를 두시는 것을 추천드려 보고 싶어요.
<BodyContainer>를 두면 구조가 한 단계 더 복잡해지게 되는데 UI에 변화고 없고, 특정 컴포넌트를 여러 번 재사용하는 경우도 아닌 것 같아서 다른 방법을 추천드리게 되었어요. - 커스텀 훅은 state를 지니는 로직을 분리하기 위함이 주 목적 중 하나이니 이 사례에서는 적용하기에 좋을 것이라고 생각합니다!
2️⃣ 리뷰
1주차와 마찬가지로 무난하게 요구사항들을 잘 이행해 주셔서 이 총괄용 코멘트에서 드릴 이야기는 별도로 없습니다. 수고하셨어요 👍🏻
- props/state를 능숙하고 올바른 방법으로 사용해 주셨고
- key값 또한 안티 패턴을 사용하지 않고 유니크한 값을 사용하도록 해 주신 점
을 확인했습니다.
대부분 드리고 싶은 말씀은 코멘트에 적어드렸으므로 코멘트를 확인해 주시면 좋겠습니다.
3️⃣ 질문
다음 리뷰 요청 시, 아래의 질문에 대해 의견을 공유해주실 수 있으실까요?
- Q1. props와 state의 차이점이 무엇인가요? 각 요소의 용도는 무엇인가요?
단순해 보이는 질문이지만 갑자기 질문하면 답변하기 마냥 번거로워지기도 하는 질문이더라고요. 이번 미션에서 확실히 용도를 바로잡고 가면 좋을 것 같아 질문으로 선택했습니다.
- Q2. key값을 사용하지 않거나 겹치는 값을 사용할 경우 어떤 문제가 발생할 수 있을까요?
자세한 원리는 공유하지 않으셔도 괜찮습니다. 어떤 문제가 발생할 수 있을 지 간단하게만 말씀해주셔도 충분하다 생각합니다.
본래는 key값을 사용해보는 것까지가 미션의 의도라는 것을 리뷰어도 잘 알고 있지만, React 사용 경험이 있으시고 key값도 용도에 맞게 사용해 주신 것 같아 드리게 된 추가 질문이에요.
리뷰 반영 중 궁금한 점이 있다면 언제든지 이곳 또는 Discord에 질문해주셔도 좋으며, 코멘트가 이해되지 않거나 다른 생각을 가지신 경우 무조건 반영하시기보다는 리뷰어에게 역으로 질문을 던져보시는 것도 좋은 리뷰 중의 상호작용 방법이라 생각합니다.
그럼, 창우님의 의견을 들려주세요! 💪🏻
1단계 리뷰로 이동하기 ↩
| const handleChange = (e) => { | ||
| onChangeCategory(e.target.value); | ||
| }; | ||
|
|
||
| return ( | ||
| <section className={styles["restaurant-filter-container"]}> | ||
| <select | ||
| name="category" | ||
| id="category-filter" | ||
| className={`${styles["restaurant-filter"]} ${styles["restaurant-filter-container-select"]}`} | ||
| aria-label="음식점 카테고리 필터" | ||
| value={category} | ||
| onChange={handleChange} | ||
| > |
There was a problem hiding this comment.
오, on-, handle- 모두 React에서 흔하게 사용되는 작명 방법인데요, 각각 어떨 때 사용하고 계신 건지 공유해 주실 수 있나요?
There was a problem hiding this comment.
이건 흔히 사용하는 모습을 보고 따라하기 시작하면서 고정된것 같아요.
저는 on-은 흔히 어떤 동작이 일어날때? 의미적으로는 on~~ => "너가 이 요소를 ~~하면"으로 해석합니다.
handle은 말그대로 다루다, manage와 같은 의미로 해석합니다. 앞서 말한 내용에 이어서, "~~하면, 이렇게 다루자"입니다.
There was a problem hiding this comment.
지금 이해하신 내용과 사용 예시는 상당이 좋아보여요. 좀 더 내용을 정제해본다면
on-: props에 붙이면 좋아요. 이 props에 이벤트 핸들링 함수 가 와야 한다는 것을 의미해요. 이벤트의 발생 조건을 이야기하는 이름이니"너가 이 요소를 ~~하면"이라고 말씀하신 바에도 부합하죠?handle-: 이벤트를 핸들링하는 함수에요. 이 함수가 발생한 이벤트에 대해 조치를 행하는 함수가 되는 거에요.
그래서 흔한 형태는 onSomething={handleSomething} 형태에요!
하지만 간혹가다가 여러 단계에 걸쳐 이벤트를 처리하는 경우는 아래와 같이 onSomething={onSomething2} 과 같은 형태도 나올 수는 있어요. 이 경우 handleSomething2가 아니라 onSomething2 인 이유는, onSomething2 도 props의 형태로 주어진 것이며, 직접 이벤트를 핸들링하는 함수가 아니기 때문이에요. 최종 핸들러가 아니죠. 이벤트 발생 시 하는 동작이 정해져 있지 않아 props로 하는 동작의 여지를 남겨두는 공용 컴포넌트에서 특히 자주 볼 수 있어요.
const Button = ({ onClick }) => {
return (
<button onClick={onClick} />
);
};| <section className={styles["restaurant-list-container"]}> | ||
| <ul className={styles["restaurant-list"]}> | ||
| {restaurants.map((restaurant) => ( | ||
| <li key={restaurant.id} className={styles["restaurant"]}> |
There was a problem hiding this comment.
key prop 사용도 잊지 않고 잘 적어주셨고, key값으로 두는 id도 겹치지 않을만한 이름이네요. 좋습니다! 👍🏻
There was a problem hiding this comment.
이 부분에 대해서 약간 찝찝함이 있는데요! 가끔 프론트에서 데이터 목록을 가져다 쓴다고 할 때, 이런 id 값을 그냥 1,2,3,... 이렇게 저장해버리는 경우가 많은데요. 혹시 이런 방식보다는 좀 더 유니크한 방식을 선호하나요??
There was a problem hiding this comment.
수 형태의 ID를 사용하는 상황 말씀하신 것 같군요? 수 자체를 사용하는 것은 나쁜 상황이 아니에요. 오히려 ID 값으로 자주 사용되는 형태에요.
나쁜 상황은 무엇을 ID 값으로 두든간에 유니크하지 않은 ID가 올 수 있거나, ID에 대응되는 항목이 항상 같은 것이 보장되지 못하는 경우라고 보고 있어요. 왜 key값으로 인덱스를 두는 것은 안티 패턴으로 보고 사용을 피하라고 이야기할까요? 앞에 새로운 항목이 추가되는 경우 ID가 하나씩 뒤로 밀리기 때문에 같은 항목이 다른 ID를 부여받는 경우가 생기기 때문이에요. ID에 대응되는 항목이 같아야 하는 원칙을 깨버린거죠.
여담으로 백엔드와 협업하는 상황이라면 십중팔구 이 ID 값은 백엔드에서 유니크한 값을 발급해 줄 거에요. 그러니 ID는 ID답게 사용되도록, 위 원칙을 어기지 않도록 노력해 주시면 될 것 같습니다.
src/pages/Body.jsx
Outdated
| import restaurants from "../const/restaurantList"; | ||
|
|
||
| const Body = () => { | ||
| const [category, setCategory] = useState("전체"); |
There was a problem hiding this comment.
useState는 무슨 역할을 하는 hook일까요?[a, b] = useState(~~)에서a와b는 각각 무엇일까요?
There was a problem hiding this comment.
useState는 데이터를 저장하고 관리하는 역할을 합니다. 데이터의 저장된 상태를 관리하는 상태 관리 훅입니다. 따라서 동작에 따라 새로이 들어온 값을 반영하기도 하고, 객체처럼 저장된 값을 사용할 수 있게도 합니다. 추가적으로 React의 랜더링 방식에서는 상태의 변화가 생기면 리랜더링이 발생하게 되는데, 이 때의 상태를 관리하는 것 중에 하나가useState입니다.a는 상태 값이 반환되는 곳입니다.b는 상태값을 업데이트 하는 역할을 합니다.
| <option value="전체">전체</option> | ||
| <option value="한식">한식</option> | ||
| <option value="중식">중식</option> | ||
| <option value="일식">일식</option> | ||
| <option value="양식">양식</option> | ||
| <option value="아시안">아시안</option> | ||
| <option value="기타">기타</option> |
There was a problem hiding this comment.
이 옵션값들은 항목이 많아지면 많아질수록 반복되는 코드가 많고 변경하기 어려울 수도 있어보인다고 생각되는데, 이후 요구사항이 변경된다면 변경이 쉽도록 관리해 볼만한 방법은 없을까요?
아, 만약 이렇게 하드코딩을 유지하시고 싶으신 이유가 있으시다면 반영하지 않고 그 이유를 공유해 주셔도 좋을 것 같아요. 창우님의 의견도 들어보고 싶군요 😄 하드코딩이 항상 오답인 것은 아니니까요
There was a problem hiding this comment.
이 경우는 하드 코딩보다는 각 옵션의 내용을 분리해두고, map함수의 반복을 이용하는 것이 좋아보입니다. 하지만 옵션의 개수가 적다면.. 그냥 하드코딩으로 제공하는 것이 나을 수도 있겠다는 생각이 듭니다.
There was a problem hiding this comment.
그 경우라면 충분히 하드코딩을 할 이유가 있을 것 같아요!
다만 다시 코드를 보면서 눈치챈 게 있는데 <CategorySortFilter>와 <AddRestaurantModal>의 <option>들이 "전체"를 제외하고 서로 공유하기도 하네요? 두 곳 이상의 사용처라면 또 다른 생각을 할 수 있어보이고요
어쨌든 본질은, 하드코딩이라고 항상 나쁜 건 아니다라고 생각하고 있어요
src/const/restaurantList.js
Outdated
| description: | ||
| "평양 출신의 할머니가 수십 년간 운영해온 비지 전문점 피양콩 할마니. 두부를 빼지 않은 되비지를 맛볼 수 있는 곳으로, ‘피양’은 평안도 사투리로 ‘평양’을 의미한다. 딸과 함께 운영하는 이곳에선 맷돌로 직접 간 콩만을 사용하며, 일체의 조미료를 넣지 않은 건강식을 선보인다. 콩비지와 피양 만두가 이곳의 대표 메뉴지만, 할머니가 옛날 방식을 고수하며 만들어내는 비지전골 또한 이 집의 역사를 느낄 수 있는 특별한 메뉴다. 반찬은 손님들이 먹고 싶은 만큼 덜어 먹을 수 있게 준비돼 있다.", | ||
| category: "한식", | ||
| img_src: "../../../templates/category-korean.png", |
There was a problem hiding this comment.
| img_src: "../../../templates/category-korean.png", | |
| imgSrc: "../../../templates/category-korean.png", |
JavaScript에서는 변수명, 객체의 속성명에는 주로 카멜 케이스를 사용하고, 대부분의 컨벤션에서도 카멜 케이스를 사용하므로 특별한 상황이 아닌 경우 카멜 케이스의 사용을 제안드립니다. 어떠한가요?
물론, 몇몇 사유로 카멜 케이스를 어쩔 수 없이 사용하는 경우도 있어요. 예를 들면 외부 API에서 반환하는 데이터가 간혹가다가 카멜 케이스가 아닌 경우가 있는데, 이 경우에는 상황에 따라 반환받은 값을 관리하기 위해 예외적으로 카멜 케이스를 사용하지 않기도 합니다!
There was a problem hiding this comment.
아 그렇네요! 함수명 등을 카멜케이스로 적는 것은 익숙하지만, 어째서인지 변수명 등에서 두 단어가 올 경우 자꾸만 '_'를 이용하게 되네요.. 카멜케이스의 사용에 좀 더 주의를 기울여봐야겠습니다.
src/pages/Body.jsx
Outdated
| ? restaurants : restaurants.filter((restaurant) => restaurant.category === category); | ||
|
|
||
| const handleChangeCategory = (category) => { | ||
| setCategory(category); |
There was a problem hiding this comment.
category에 직접 값을 할당하지 않으시고 굳이 setCategory를 사용하시는 이유가 있으신가요?
There was a problem hiding this comment.
제가 생각하는 useState란, 리액트의 state(상태)를 관리하는 훅입니다. useState로써 마치 const, let과 같은 어떤 객체가 되는 것이 아니라, 이미 리액트 내부의 존재하는 상태에 새로운 값을 업데이트 하거나 가져다 쓸 수 있도록 돕는 메서드라고 해석했습니다.
즉, 저기서 보이는 category는 실질적인 데이터의 위치라고 생각하지 않으며, 또한 category = "한식" 이런식으로 직접 대입하는 것이 아니라, setCategory 메서드를 통해서 업데이트 하는 것입니다. 그렇기 때문에 만약 category = "한식"과 같은 방식이 가능하다 해도, 리액트는 이것을 새로운 상태로 반영하지 않을 것이라고 생각합니다.
이건 마치
const a = 0;
a = 1;이런 코드를 실행시켰다고 랜더링이 발생하지 않는 것 처럼요!
내부적인 로직은 모르지만.. set~ 이 가진 역할이 이 상태를 업데이트 시킴(즉, 랜더링은 유발시킴)이라고 생각합니다.
There was a problem hiding this comment.
잘 설명해주셨습니다. 상태가 변경되어야 하면 React는 리렌더링(re-rendering), 배칭(batching) 등 새로운 상태를 반영하기 위한 조치를 취하는데, 직접 값을 할당하는 경우 React가 이러한 변화를 모르게 되므로 컴포넌트/UI에 상태와 불일치하는 값이 보이게 되는 현상이 일어날 수 있습니다.
set function의 사용은 단순히 상태를 변경하는 것을 넘어 React에게 상태 변경을 알리는 역할을 겸하므로, set function을 사용하여 상태를 업데이트해야 합니다.
src/pages/Body.jsx
Outdated
|
|
||
| const filteredRestaurants = | ||
| category === "전체" | ||
| ? restaurants : restaurants.filter((restaurant) => restaurant.category === category); |
There was a problem hiding this comment.
Array.prototype.filter 고차함수 사용 정말 좋네요 👍🏻 의도도 잘 드러나고 처리도 깔끔해요
| import React from "react"; | ||
| import styles from "../../css/Body.module.css"; | ||
|
|
||
| const CategorySortFilter = ({ category, onChangeCategory }) => { |
There was a problem hiding this comment.
CategorySortFilter이름이 깔끔하고 이해하기 쉽네요 👍🏻- 개인적인 의견이기는 합니다만
category라는 prop에, 이 카테고리가 "선택되어 있는" 카테고리라는 설명이 있으면 의미를 명확하게 해줄 수 있다는 생각이 들었는데, 어떻게 생각하시나요?
There was a problem hiding this comment.
아 정말 좋은 아이디어네요! 특별한 가공을 거치지 않은 데이터라면 항상 원본을 사용했던 것 같아요. 부모 컴포넌트에서도 "선택되어 있는"이라는 의미가 들어가면 좋겠지만, 그렇지 않다고 해도 적어도 자식 컴포넌트에서는 의미가 더 명확한 것이 좋을 것 같습니다. 특히 부모에서의 코드 내용을 자식에서는 파악하기 어려우니까요!
src/css/Text.module.css
Outdated
| .text-title { | ||
| font-size: 20px; | ||
| line-height: 24px; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .text-subtitle { | ||
| font-size: 18px; | ||
| line-height: 28px; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .text-body { | ||
| font-size: 16px; | ||
| line-height: 24px; | ||
| font-weight: 400; | ||
| } | ||
|
|
||
| .text-caption { | ||
| font-size: 14px; | ||
| line-height: 20px; | ||
| font-weight: 400; | ||
| } No newline at end of file |
There was a problem hiding this comment.
App.css에도 동일한 속성들이 있는 것 같아요, 따라서 둘 중 한 쪽의 스타일은 제거해 주셔야 할 것 같아요. 크게 두 가지 선택으로 갈릴 것 같은데, 창우님이라면 어느 쪽을 고르실 것 같나요? 그 이유도 들어보고 싶어요.
App.css에 있는 속성을 제거한다Text.module.css에 있는 속성을 제거하고, 이 css 파일을 삭제한다
There was a problem hiding this comment.
앗.. 실수했네요.. 저라면 App.css에 둘 것 같습니다. 왜냐하면 text-title의 명칭만 보아도, 특별히 어떤 컴포넌트에서만 사용하고자 명명한 classname은 아닌 것 같아요. 실제로도 여러군데에서 사용되기도 하고요.
|
step1 미션의 리뷰에 이러 step2 리뷰 감사히 읽었습니다
이런 방법은 생각하지 못했어요!
아, 그렇군요. 말씀해주신 것 중
라는 말씀이 굉장히 와닿네요. 물론 가독성이나 유지보수와 관련된 고민은 좋지만, 잘생각해보면 해당 과정이 복잡성을 증가시키는데에 반해 효과가 미미하다면 굳이 그렇게 할 이유가 없겠군요. 같은 의미이지만 조금 더 모호하다고 느낀 것은, 바로 위에 설명하신 커스텀 훅이에요. 분명 커스텀 훅 등을 사용하는 것이 유지보수나 재사용성, 또는 확실치 않지만 어떤 경우에는 부가적인 기능을 담을 수 있기에 유용하다고 생각해요. 하지만 이번 코드처럼 아주 짧은 내용을 담고 있다면 어떤가요? 간단한 코드이지만 그냥 두기에는 조금 난잡해보이고, 그렇다고 훅으로 분리하기에는.. 필요 이상이 아닌가 하는 생각도 조금 듭니다.
A1. 으음.. 명확한 단어로 설명하기 어려운 것 같아요. props와 state 둘 모두 정보를 담고 있는 객체라는 점은 같습니다. 하지만 그 역할은 조금 다른 것 같아요. props는 어떤 데이터를 담고 보관하기 위한 용도가 아닙니다. 부모가 자식에게 필요한 데이터를 보내기 위한 어떤 카트와 같은 역할이라고 생각합니다. 반면 state는 어딘가에 전달하고자 하는 용도가 아닙니다. state는 해당 컴포넌트 내에서 현재 데이터를 상태로 저장하고, 그 값이 변하면 상태 변화로 인지하여 랜더링을 발생하는 역할을 합니다. 조금 더.. 능동적인 역할?이라고 느껴집니다.
A2. 이건 더 어려운 질문인 것 같아요.. key값의 부재시 발생하는 문제 등의 원리는 잘모릅니다. 다만, 좀 더 포괄적으로 key값 등을 인지하고 있는 것 같아요. |
There was a problem hiding this comment.
@ChangwooJ 👋🏻
2단계 미션 수행하시느라 고생하셨습니다! 무조건 반영하시기보다는 리뷰어에게 질문을 던지는 느낌이 많이 들어 과제를 시키는 것이 아니라 개발자 대 개발자로써 이야기하는 느낌이 많이 들었습니다 👍🏻 긴 글을 쫘라락 많이 적은 것 같았는데 하나하나 고민해 주시느라 수고 많으셨습니다! 다음 미션에서는 다른 리뷰어님과 함께 색다른 경험을 하시길 바래요
질문에 대해 제가 생각하는 답변 적어봅니다.
만약 public폴더 내에 위치 시킨다면 어떤 문제가 발생하나요?
src, public 중 어디에 둘 지라... 참 고민되는 주제군요. 결론부터 적어보자면 많은 상황에서는 /src 에 두는 것이 추천되는 편입니다. 하지만 상황에 따라 달라질 여지는 있습니다.
/src 내에 이미지들을 두었을 때에는 아래의 장점을 누릴 수 있습니다. 다르게 말하면 /public에 두면 이러한 장점들을 놓치게 될 수 있다는 의미도 되겠죠?
/src내에 있는 이미지들은 빌드 시 프로젝트에 포함되어 있는 개념이며,/public내에 있는 이미지들은 포함되지 않는 개념입니다. 빌드 시/src에 있는 이미지들은 관리 대상이 될 것입니다./src에 있는 이미지들의 경로가 잘못되어 있거나 없을 경우 vscode 등의 편집기에서도 에러가 하이라이팅되고, 빌드 과정에서도 에러가 발생하며 컴파일이 실패합니다. 에러를 컴파일 타임에 잡을 수 있다는 것은 조기에 문제를 발견할 수 있다 는 의미입니다. 훨씬 빠른 피드백 을 받게 되죠. 뒤늦게 런타임에서 엑스박스가 뜬 이미지를 보는 것보다 생산성 및 안정성 면에서 뛰어날 것입니다./src에 있는 이미지들은 빌드 시 *번들러 에 의해 필요한 여러 조치가 취해지며 빌드됩니다. 이미지 압축, 최적화, 해쉬 처리 등 필요한 조치가 취해지며 결과물에 포함되게 됩니다. 반면/public에 있는 이미지들은 별도의 조치 없이 원본 그대로가 결과물로 나오게 됩니다.
*번들러: 웹 애플리케이션을 개발하기 위해 필요한 HTML, CSS, JS 등의 여러 모듈로 이루어진 자원들을 모아서, 하나 혹은 최적의 소수 파일로 결합하는 도구입니다. CRA 프로젝트에서의 Webpack, 지금 미션을 수행하신 프로젝트에서의 Vite 등은 모두 번들러의 일종입니다.
결론적으로 이미지와 같은 자원을 /src 에 두면 프로젝트 내에 포함되어 관리 및 제어를 받으므로 추천되는 방식이라고 이야기할 수 있겠습니다.
물론 /public 에 두는 것이 더 추천되는 경우도 몇몇 있으니 상황에 따라 /public 도 사용할 수 있습니다.
- 매우 많은 이미지들을 동적으로 관리해야 하는 경우: 이 경우에는 public에 이 이미지들을 두고 동적 URL로 관리하는 것이 더 좋을 수도 있을 것입니다.
- URL을 통해 해당 이미지에 직접 접근해야 하는 경우:
favicon.ico,manifest.json,robot.txt등의 파일들은 브라우저, 또는 검색 엔진이 해당 주소에 직접 접근하여 필요한 조치를 취하므로/public이 추천되는 선택입니다. (CRA 프로젝트를 만들 때 항상favicon.ico는 public에 있었기도 합니다.)
같은 의미이지만 조금 더 모호하다고 느낀 것은, 바로 위에 설명하신 커스텀 훅이에요. 분명 커스텀 훅 등을 사용하는 것이 유지보수나 재사용성, 또는 확실치 않지만 어떤 경우에는 부가적인 기능을 담을 수 있기에 유용하다고 생각해요. 하지만 이번 코드처럼 아주 짧은 내용을 담고 있다면 어떤가요? 간단한 코드이지만 그냥 두기에는 조금 난잡해보이고, 그렇다고 훅으로 분리하기에는.. 필요 이상이 아닌가 하는 생각도 조금 듭니다.
이미 말씀드렸지만 구조가 이미 간단한 상황입니다. 커스텀 훅이 과도한 분리라 생각하신다면 도입하지 않으셔도 괜찮다고 생각합니다. 저는 창우님이 필요하신 점이 state를 다루는 로직들을 분리 하는 것이라고 생각했기 때문에, 이 사용처에 맞는 커스텀 훅 을 추천 방법으로써 드렸습니다.
컴포넌트가 이미 복잡한 UI 구조를 가지고 있는데 도메인 로직들까지 더해져서 난잡해진다면 그 때 도입해주셔도 충분하다 생각합니다.
이 장단점을 고려하여 개발해 주시면 좋을 것이라 생각합니다 ✔️
A1. 으음.. 명확한 단어로 설명하기 어려운 것 같아요. props와 state 둘 모두 정보를 담고 있는 객체라는 점은 같습니다. 하지만 그 역할은 조금 다른 것 같아요. props는 어떤 데이터를 담고 보관하기 위한 용도가 아닙니다. 부모가 자식에게 필요한 데이터를 보내기 위한 어떤 카트와 같은 역할이라고 생각합니다. 반면 state는 어딘가에 전달하고자 하는 용도가 아닙니다. state는 해당 컴포넌트 내에서 현재 데이터를 상태로 저장하고, 그 값이 변하면 상태 변화로 인지하여 랜더링을 발생하는 역할을 합니다. 조금 더.. 능동적인 역할?이라고 느껴집니다.
저도 막상 다른 분이 질문하시면 긴가민가 하더군요~ 데이터 흐름을 위주로 말씀하셨는데 맞는 말씀입니다. 다만 좀 더 눈에 들어오시기 쉽도록 간단요약해서 적어보겠습니다.
1️⃣ props
- 컴포넌트 밖에서 주어짐
- 읽기 전용
- 외부에서 컴포넌트를 제어하기 위해 사용
- 다른 컴포넌트로 데이터를 전달하기 위해 사용
2️⃣ state
- 컴포넌트 내부에서 사용함
- 읽고 수정할 수 있음
- 컴포넌트 자신의 상태를 변경 및 관리하기 위해 사용
이러면 내용이 그나마 잘 들어오실까요...?
A2. 이건 더 어려운 질문인 것 같아요.. key값의 부재시 발생하는 문제 등의 원리는 잘모릅니다. 다만, 좀 더 포괄적으로 key값 등을 인지하고 있는 것 같아요.
제가 생각하는 key값의 용도는 색인과 비슷하다고 생각합니다.
마침 얼마전 <군주론>이라는 책을 빌렸는데, 도서관에서는 이 책이 한권만 존재하지는 않죠. 만약 색인이 존재하지 않다면, 도서관에서는 제가 이 책을 빌려갔다는 사실은 알아도, 기록해 놓지 않는다면 어느날 반납된 <군주론>이 제가 반납한 것일 수도, 혹은 다른 대출자가 반납한 것일 수도 있게되는 것이죠.
그런 의미에서 key값은 말그대로 index의 역할을 한다고 생각합니다. 내용상 같은 데이터가 들어와도 착오가 없도록 말이죠.
어려우실 수 있는 거 이해됩니다. 이번 미션의 주제도 key값을 "사용해보자" 지 왜 쓰는지 "이해해보자" 는 아니었습니다. 이미 React 경험이 있다고 말씀해주셔서 조금 더 깊게 들어가보았을 뿐이니 걱정하지 않으셔도 됩니다.
발생할 수 있는 증상들만 나열한다면 React key를 사용하지 않거나 잘못 사용할 경우 아래의 문제가 발생할 수 있습니다.
- 성능 저하가 발생할 수 있습니다. 요소가 많아질수록 React key를 올바르게 사용했을 때와 비교했을 때 업데이트가 더 느려지는 것이 체감되기 시작합니다.
- UI가 잘못 업데이트되거나 버그가 발생할 수 있습니다. 어라라, 분명 첫 번째 항목을 지우는 버튼을 눌렀는데 세 번째 항목이 지워집니다.
React가 key를 잘 활용하지 못하면 실제로는 그럴 필요가 없음에도 불필요하게 DOM을 다시 만드는 작업을 하기도 하고, key에 대응하는 요소가 잘못 이어져 엉뚱한 항목을 변경하거나 제거할 수도 있게 됩니다.
이전 기수에서는 인덱스를 key 값으로 두는 것이 안티패턴이라는 설명을 했었는데, 그 코멘트를 첨부해 드리고자 합니다.
| <option value="전체">전체</option> | ||
| <option value="한식">한식</option> | ||
| <option value="중식">중식</option> | ||
| <option value="일식">일식</option> | ||
| <option value="양식">양식</option> | ||
| <option value="아시안">아시안</option> | ||
| <option value="기타">기타</option> |
There was a problem hiding this comment.
그 경우라면 충분히 하드코딩을 할 이유가 있을 것 같아요!
다만 다시 코드를 보면서 눈치챈 게 있는데 <CategorySortFilter>와 <AddRestaurantModal>의 <option>들이 "전체"를 제외하고 서로 공유하기도 하네요? 두 곳 이상의 사용처라면 또 다른 생각을 할 수 있어보이고요
어쨌든 본질은, 하드코딩이라고 항상 나쁜 건 아니다라고 생각하고 있어요
src/pages/Body.jsx
Outdated
| ? restaurants : restaurants.filter((restaurant) => restaurant.category === category); | ||
|
|
||
| const handleChangeCategory = (category) => { | ||
| setCategory(category); |
There was a problem hiding this comment.
잘 설명해주셨습니다. 상태가 변경되어야 하면 React는 리렌더링(re-rendering), 배칭(batching) 등 새로운 상태를 반영하기 위한 조치를 취하는데, 직접 값을 할당하는 경우 React가 이러한 변화를 모르게 되므로 컴포넌트/UI에 상태와 불일치하는 값이 보이게 되는 현상이 일어날 수 있습니다.
set function의 사용은 단순히 상태를 변경하는 것을 넘어 React에게 상태 변경을 알리는 역할을 겸하므로, set function을 사용하여 상태를 업데이트해야 합니다.
| const handleChange = (e) => { | ||
| onChangeCategory(e.target.value); | ||
| }; | ||
|
|
||
| return ( | ||
| <section className={styles["restaurant-filter-container"]}> | ||
| <select | ||
| name="category" | ||
| id="category-filter" | ||
| className={`${styles["restaurant-filter"]} ${styles["restaurant-filter-container-select"]}`} | ||
| aria-label="음식점 카테고리 필터" | ||
| value={category} | ||
| onChange={handleChange} | ||
| > |
There was a problem hiding this comment.
지금 이해하신 내용과 사용 예시는 상당이 좋아보여요. 좀 더 내용을 정제해본다면
on-: props에 붙이면 좋아요. 이 props에 이벤트 핸들링 함수 가 와야 한다는 것을 의미해요. 이벤트의 발생 조건을 이야기하는 이름이니"너가 이 요소를 ~~하면"이라고 말씀하신 바에도 부합하죠?handle-: 이벤트를 핸들링하는 함수에요. 이 함수가 발생한 이벤트에 대해 조치를 행하는 함수가 되는 거에요.
그래서 흔한 형태는 onSomething={handleSomething} 형태에요!
하지만 간혹가다가 여러 단계에 걸쳐 이벤트를 처리하는 경우는 아래와 같이 onSomething={onSomething2} 과 같은 형태도 나올 수는 있어요. 이 경우 handleSomething2가 아니라 onSomething2 인 이유는, onSomething2 도 props의 형태로 주어진 것이며, 직접 이벤트를 핸들링하는 함수가 아니기 때문이에요. 최종 핸들러가 아니죠. 이벤트 발생 시 하는 동작이 정해져 있지 않아 props로 하는 동작의 여지를 남겨두는 공용 컴포넌트에서 특히 자주 볼 수 있어요.
const Button = ({ onClick }) => {
return (
<button onClick={onClick} />
);
};| <section className={styles["restaurant-list-container"]}> | ||
| <ul className={styles["restaurant-list"]}> | ||
| {restaurants.map((restaurant) => ( | ||
| <li key={restaurant.id} className={styles["restaurant"]}> |
There was a problem hiding this comment.
수 형태의 ID를 사용하는 상황 말씀하신 것 같군요? 수 자체를 사용하는 것은 나쁜 상황이 아니에요. 오히려 ID 값으로 자주 사용되는 형태에요.
나쁜 상황은 무엇을 ID 값으로 두든간에 유니크하지 않은 ID가 올 수 있거나, ID에 대응되는 항목이 항상 같은 것이 보장되지 못하는 경우라고 보고 있어요. 왜 key값으로 인덱스를 두는 것은 안티 패턴으로 보고 사용을 피하라고 이야기할까요? 앞에 새로운 항목이 추가되는 경우 ID가 하나씩 뒤로 밀리기 때문에 같은 항목이 다른 ID를 부여받는 경우가 생기기 때문이에요. ID에 대응되는 항목이 같아야 하는 원칙을 깨버린거죠.
여담으로 백엔드와 협업하는 상황이라면 십중팔구 이 ID 값은 백엔드에서 유니크한 값을 발급해 줄 거에요. 그러니 ID는 ID답게 사용되도록, 위 원칙을 어기지 않도록 노력해 주시면 될 것 같습니다.
| /> | ||
| </div> | ||
| <div className={styles.restaurant__info}> | ||
| <h3 className={`$ {styles["restaurant__name"]} text-subtitle`}> |
There was a problem hiding this comment.
| <h3 className={`$ {styles["restaurant__name"]} text-subtitle`}> | |
| <h3 className={`${styles["restaurant__name"]} text-subtitle`}> |
아마도 오타겠죠? 다음 주차 미션하실 때 같이 수정해 주시길 바래요!
3 participants
안녕하세요! wzrabbit 리뷰어님!
얼마전 올린 1단계 PR에 이어서 이번엔 2단계 미션에 대한 내용입니다! 이번 미션도 잘부탁드리겠습니다!
이번 미션의 내용은 음식점의 데이터 들을 따로 분리해 파일로 만들고, 이를 이용해서 음식점 목록을 보여주던 컴포넌트를 동적으로 랜더링 할 수 있도록 변경하는 내용이었습니다.
React의 랜더링 방식 중 useState의 상태 변경을 이용한 리랜더링 현상을 통해 필터의 선택 값이 바꿀 때마다 새롭게 랜더링하여 원하는 요소를 보여줄 수 있도록 의도하였습니다.
step2.mp4
이번 미션을 진행하면서 두가지 궁금증이 존재했습니다.
하나는 이미지 경로에 대한 저장 방법, 혹은 처리법에 대한 것이고,
다른 하나는 저의 첫 의도와 달리 pages에 존재하는 Body.jsx 파일이 코드가 길어지게 되었다는 점입니다.
먼저, 각 필터에 따라 유동적으로 변하는 컴포넌트를 구현하기 위해선 이미지 역시 고정 값일 수 없었습니다.
이에 저는 음식점 리스트를 모아놓은 파일에 img_src로 경로를 추가하여 제공하였으나, 이것이 올바른 방법인지 판단이 되지 않습니다. 틀린 방법은 아니라고 생각되지만, 만약 이미지의 위치가 바뀌게 된다면 유지보수 관점에서 최악이 될 것이라고 생각됩니다.
이런 경우에는 이미지를 경로등을 관리하는 라이브러리, 혹은 폴더 구조를 정의하여 고정되도록 사용하여야하나요?
다음으로는 제 첫 page폴더의 의도는 하나의 페이지가 가지는 여러가지 컴포넌트를 묶어두는 용도의 파일을 담아두는 폴더였습니다.
그래서 처음에는 Body.jsx에 각 컴포넌트를 임포트 하는 정도로만 작성이 되었는데, 오늘 미션을 진행하면서 각 컴포넌트에 필요한 데이터를 전달하기 위한 코드가 필요해 불가피하게 의도에서 벗어나게 되었습니다.
몇몇 데이터를 가공하는 코드는 또다시 분리하는 리팩토링 과정을 거칠 수는 있지만, 이 역시도 결국 임포트해야하는 상황이 발생합니다.
그렇다면 제 의도대로 되려면, 지금 현 코드에서처럼 바로 Body->RestaurantList or CategorySortFilter 가 아니라 BodyContainer -> Body -> ... 이런 식으로 접근하였어야 할까요? 혹은, 제 의도가 불필요한 낭비로 이어지나요?