-
Notifications
You must be signed in to change notification settings - Fork 26
[Sprint5] 이승현 #92
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
[Sprint5] 이승현 #92
The head ref may contain hidden characters: "React-\uC774\uC2B9\uD604"
Conversation
[Fix] delete merged branch github action
…현-sprint1 Add landing page with HTML, CSS and images
…현-sprint2 [이승현] Sprint2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요, 승현님~첫 리액트 미션은 어떠셨나요? 고생 많으셨습니다. 😁
전반적으로 요구사항들은 잘 구현해주셨어요.
다만, @latest, vite-project 등의 샘플 폴더는 정리가 필요할 것 같아요.
현재 프로젝트의 맥락에 맞는 파일들만 남겨주세요!
참고로, 최상위 디렉토리는 React 폴더가 아닌 node_modules, src...가 되어야 합니다.
다음으로, 가장 필요한 건 미사용,오타 등의 코드 정리일 것 같아요.
곳곳에 미사용 코드나 오타 등이 꽤 발견 되고 있어요.
이런 부분들이 잘 보이지 않는다면, eslint 관련 규칙들을 찾아보셔도 좋을 것 같아요.
vite 빌드 툴을 사용하고 계시니까, eslint 적용이 어렵지 않을 거예요!
npm create vite@latest프로젝트 생성 시 React를 선택해서 eslint.config.js 파일을 확인해 보세요.
다음 규칙을 추가하시면 미사용 변수에 대한 에러가 표시됩니다:
'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }],
그 다음에는 상수 또는 뜻이 있는 숫자 값을 그대로 넣는 것이 아닌, 변수로 선언해보면 좋을 것 같아요. 이런식으로 정리만 해도 가독성이 꽤 좋아지는 것을 느끼실 겁니다! 🤓
| @@ -0,0 +1,99 @@ | |||
| import React, { useState, useEffect } from 'react'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React 17부터는 Babel 등 트랜스파일러가 JSX를 변환할 때, 자동으로 필요한 함수를 import해주기 때문에, 개발자가 직접 import React from 'react'를 파일의 최상단에 추가하지 않아도 JSX를 사용할 수 있어요.
혹시 확장의 snippet으로 컴포넌트 생성을 하셨을까요? 이번 기회에 디스코드 개발_자료 채널에서 알려드린 방법으로 직접 snippet을 만들어보시는 것을 추천드려요! 🙂
| <> | ||
| <SectionContainer> | ||
| <SectionTitle>베스트 상품</SectionTitle> | ||
| {bestProductsLoading ? <p>로딩 중...</p> : <BestProductGrid products={bestProductsToDisplay} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로딩 UI 구현까지 해주셨네요! 여기서 사용자 경험을 좀 더 개선해보면 좋을 것 같아요. 현재는 다음 상품을 불러올 때마다 Layout shift가 발생해서 조금 아쉬워요. Skeleton UI를 적용해보는 것은 어떨까요?
| @@ -0,0 +1,99 @@ | |||
| import React, { useState, useEffect } from 'react'; | |||
| import styled from 'styled-components'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
미사용 코드는 꼭 정리 후에 제출해주세요! 기술 과제 시 마이너스 요인이 될 수 있어요. 🤓
| AllProductGrid, | ||
| SectionContainer, | ||
| SearchInputContainer, | ||
| // ActionButtons, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 마찬가지로, 사용하지 않는다면 제거 후 제출해주세요! 만약 나중을 위해 주석처리를 해놓은 거라면 해당 부분만 제외한 후 커밋을 하시면 됩니다. 🙂
| <DropdownMenu> | ||
| {options.map((option) => ( | ||
| <DropdownItem onClick={() => handleOptionClick(option)}> | ||
| {option.label} | ||
| </DropdownItem> | ||
| ))} | ||
| </DropdownMenu> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map() 호출 내부의 JSX 요소는 항상 key가 필요해요. 이를 통해 React가 리스트 내 항목들의 위치를 고유하게 식별할 수 있어요. 고유한 key를 제공해 주세요.
| width: 100%; | ||
| height: 42px; | ||
| font-size: 16px; | ||
| font-weight: 400px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
속성 단위에 오타가 있네요. 수정 부탁드려요. 🙂
| @media ${props => props.theme.moble} { | ||
| padding: 4px 15px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기에도 오타가 있네요! ㅎㅎ
| import React from 'react'; | ||
| import { BrowserRouter as Router, Routes, Route, Navigate } from 'react-router-dom'; | ||
| import Navbar from './components/common/Navbar.jsx'; | ||
| import ItemsPage from './pages/ItemsPage/ItemsPage.jsx'; | ||
| import AddItemPage from './pages/AddItemPage/AddItemPage.jsx'; | ||
| import { createGlobalStyle, ThemeProvider } from 'styled-components'; | ||
| import { theme } from './styles/theme.js'; | ||
| import GlobalStyle from './styles/Globalstyle.jsx'; | ||
| import Layout from './components/layouts/Layout.jsx'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Globalstyle 경로의 대소문자가 불일치해요. 배포 시 문제가 될 수 있으니 수정해 주시고, 미사용 모듈도 정리가 필요합니다. 🙂
| export const API_BASE_URL = 'https://panda-market-api.vercel.app'; | ||
| export const PRODUCTS_PER_PAGE = 10; | ||
| export const BEST_PRODUCTS_COUNT = 4; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 해당 파일에 상수들을 분리하는 기준이 있을까요? 🤔
PRODUCTS_PER_PAGE 변수가 있는 것을 보면, pageGroupSize도 이 곳에 위치해야 할 것 같아요. 관심사 분리를 할 때, 명확한 기준을 세우지 않고 분리하면 오히려 복잡도를 키울 수 있어요. 페이지네이션, 인기 상품 등의 명확한 목적이 있다면 pageGroupSize 변수처럼 가까운 곳에 선언하는 것이 더 나은 선택일 수 있어요.
요구사항
기본
심화
주요 변경사항
스크린샷
https://panda-market-17-seung.netlify.app/items
멘토에게