-
Notifications
You must be signed in to change notification settings - Fork 39
[김태일] sprint7 #214
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
[김태일] sprint7 #214
The head ref may contain hidden characters: "React-\uAE40\uD0DC\uC77C-sprint7"
Conversation
[Fix] delete merged branch github action
…-sprint1 [김태일] sprint1
…일-sprint2 [김태일] Sprint2
GANGYIKIM
left a comment
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 미션 고생하셨어요.
여러개의 미션을 한번에 리뷰 드려서 코멘트 양이 많습니다.
혹시나 해서 말씀드리지만 코멘트 반영은 필수가 아닌 선택사항이에요.
다만 학습에 도움이 되실것 같은 내용들로 남겨드렸으니 시간이 되신다면 반영하시길 추천드립니다.
다음 8번째 미션도 화이팅이에요!
| @@ -0,0 +1,17 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
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.
❗️ 수정요청
| <html lang="en"> | |
| <html lang="ko"> |
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/vite.svg" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Vite + React</title> |
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.
💊 제안
유저가 알 수 있게 타이틀 태그를 수정해주시고 미션에서 추가하셨던 메타 태그도 추가하시면 더 좋을 것 같아요!
| <title>Vite + React</title> | |
| <title>판다마켓</title> |
| <Route path="/LogIn" element={<LogIn />} /> | ||
| <Route path="/SignUp" element={<SignUp />} /> | ||
| <Route path="/SignIn" element={<SignIn />} /> | ||
| <Route path="/Items" element={<Items />} /> |
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.
❗️ 수정요청
품 상세 페이지 주소는 "/items/{productId}" 입니다. 와 같은 요청 사항에 적혀있는 주소로 설정 하시는 것을 추천드려요!
| <Route path="/Items" element={<Items />} /> | |
| <Route path="/items" element={<Items />} /> |
| createRoot(document.getElementById('root')).render( | ||
|
|
||
| <App /> | ||
|
|
||
| ) |
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.
❗️ 수정요청
| createRoot(document.getElementById('root')).render( | |
| <App /> | |
| ) | |
| createRoot(document.getElementById('root')).render(<App />) |
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.
💊 제안
pages/Home 에서는 Components 폴더 하단에 컴포넌트 파일을 두시고 다른 페이지에서는 pages/{pageNmae} 하단에 두셨네요~
한 방향으로 통일하시면 더 좋을 것 같아요!
| {product.images && ( | ||
| <img src={product.images[0]} alt={product.name} /> | ||
| )} |
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.
💊 제안
상품 생성시 이미지가 필수값이 아니라서 없을 수 있습니다. 그럴때 대체 이미지를 보여주시면 더 좋을 것 같아요!
| {product && ( | ||
| <> | ||
| <ProductWrapper> | ||
| <ProductImage> | ||
| {product.images && ( | ||
| <img src={product.images[0]} alt={product.name} /> | ||
| )} |
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.
💬 여담
product의 기본값을 빈객체로 설정하면 아래처럼 작성이 가능합니다.
지금처럼 product 값이 있을때 UI가 보인다면, 로딩중에는 해당 영역이 다 노출되지 않으니 스켈레톤을 추가해주시는 것처럼 영역을 유지할 수 있게 작업하시면 좋겠습니다.
| {product && ( | |
| <> | |
| <ProductWrapper> | |
| <ProductImage> | |
| {product.images && ( | |
| <img src={product.images[0]} alt={product.name} /> | |
| )} | |
| // const [product, setProduct] = useState({}); | |
| <> | |
| <ProductWrapper> | |
| <ProductImage> | |
| {product?.images && ( | |
| <img src={product.images[0]} alt={product.name} /> | |
| )} |
|
|
||
| const isFormValid = inquiryText.trim().length > 0; | ||
|
|
||
| const handleAddComment = () => { |
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.
| {menuOpen && ( | ||
| <Menu> | ||
| {isEditing ? ( | ||
| <MenuItem onClick={handleSave}>저장</MenuItem> |
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.
💊 제안
인터렉션이 가능한 요소라서 div보다 button이 더 적절할 것 같아요.
const handleSave = () => {
onEdit(id, editText.trim());
setIsEditing(false);
setMenuOpen(false);
};
...
<MenuItem disabled={!editText.trim()} onClick={handleSave}>저장</MenuItem>|
|
||
| const QuestionText = styled.div``; | ||
|
|
||
| const EditInput = styled.input` |
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.
❗️ 수정요청
여러줄 입력이 가능한 textarea가 더 적절할 것 같아요!
| const EditInput = styled.input` | |
| const EditInput = styled.textarea` |

요구사항
https://chipper-centaur-e6ae79.netlify.app/Items
기본
심화
주요 변경사항
스크린샷
멘토에게