-
Notifications
You must be signed in to change notification settings - Fork 21
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
[NEXTLEVEL 클린코드 리액트 호준] 장바구니 미션 Step1 #7
base: ganeodolu
Are you sure you want to change the base?
Conversation
webpack, eslint, prettier, babel
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 { useNavigate } from "react-router-dom"; | ||
|
||
const GNB = () => { | ||
const history = useNavigate(); |
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.
history 라고 적으셔서 useHistory()를 사용하신 줄 알았습니다ㅎㅋㅋ
이전 버전에서 사용했었던 useHistory때문에 네이밍을 이렇게 하신 건가요?
단순 궁금증에 여쭤봅니다ㅎㅋㅋ
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.
네 말씀하신 내용과 정확히 같아요ㅎㅎ useNavigate를 사용할때 변수명을 보통 어떻게 정하시나요??
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.
저는 그냥 navigate로 사용하고 있습니다ㅎㅎㅋ
<CartListBlock> | ||
<section className="cart-section"> | ||
<header className="flex-col-center mt-20"> | ||
<h2 className="cart-section__title">장바구니</h2> |
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.
h1사용없이 h2, h3를 사용한 것이 자연스럽지 못하다고 생각했습니다.
다른 코드리뷰를 보면서 semantic tag를 위해서 위와 같은 흐름은 부자연스럽다는 이야기를 봐서 말씀드립니다ㅎ
아마도 font-size때문에 그러신것이라고 생각됩니다만 다른 방법으로 semantic한 tag를 지향해보시면 어떨까요?
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을 그대로 사용하였습니다. @younchong 님은 이부분에 h1을 사용하셨나요? 아니면 새로 html을 따로 만드셔서 사용하셨는지 궁금합니다.
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 사용했습니다.
이부분은 수정하긴 했으나, 처음에 저도 인지못했습니다 ㅎㅋㅋ
다시 다른 부분 구현하면서 직접하신게 아니셨겠구나 깨달았습니다.
다음에 다른 곳에서 직접하실 때, 그런것도 있구나 정도로 알면 좋을것 같습니다.
저도 최근 다른 코드리뷰 보면서 알게 된 것이어서 말씀드려봤습니다 ㅎㅋㅋ
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.
세세한 리뷰 감사합니다. MDN에서도 제목 단계를 건너뛰는 것을 피하세요. 언제나 <h1>로 시작해서, <h2>, 순차적으로 기입하세요.
똑같은 내용이 있었네요. html 작성시 유의하겠습니다.
client/src/styles/common/utils.css
Outdated
@@ -0,0 +1,168 @@ | |||
.flex { |
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.
원래 코드 작성하실때 common utils css도 작성하시나요?
다른분들 코드 볼때 css도 common utils로 관리하면 간편하게 className만 더 적어주시면 돼서 편리해보였습니다.
그런데 궁금한점이 프로젝트마다 사용되는 margin, padding, gap등의 값이 다른데
처음 기능 구현전에 css로 다 구현하실 때 잡아두시는 건가요?
처음에 어느정도 부분까지 예상을 하고 값을 설정해두시는 지 궁금합니다.
display: flex 같은 것들은 자주 사용해서 기본적인 utils로 필수 값이라고 생각하지만 gap이나 width의 특정값같은 것들이 궁금해서 여쭤봅니다.
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.
이 부분에 혼동을 드렸네요. common 폴더 부분의 스타일은 실제로 적용되는 부분은 아니고, 상위의 global로 전역스타일을, 그리고 각 component에 styled component를 사용하여 스타일을 적용했습니다.
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.
이부분도 처음 주어진 css부분이란 것을 다른 부분 구현하다 알게됐습니다ㅎㅋㅋ
client/src/App.jsx
Outdated
<QueryClientProvider client={queryClient}> | ||
<GNB /> | ||
<Routes className="App"> | ||
{/* <Route path="/" element={<Home />} /> */} |
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.
따로 주석 남겨두신 이유가 있으신가요?
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.
나중에 url을 기준으로 api를 적용해볼까 생각하여 상품목록부분의 주소를 /product로 하고 싶어서 따로 남겨둔 아직 미완인 부분입니다.
|
||
@media screen and (max-width: 768px) { | ||
.product-container { | ||
grid-template-columns: repeat(3, 1fr); |
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.
반응형을 grid로 구현하셨네요. 저도 grid로 구현했는데,
처음에 배민상회 페이지에서 개발자도구로 봤을 때
상품을 4개씩 끊어서 받아와서 구현했더라고요. 그래서 저도 처음엔 그렇게 받아오는 방법을 고민했었는데, 너무 복잡하고
grid가 훨씬 간편해서 grid로 그냥 구현하긴 했는데,
다른 방식 생각해보신 것 있으신가요?
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.
말씀하신 것처럼 열, 행으로 구현은 grid가 제일 편한 것 같습니다. 다른 방식이라면 flex-wrap으로도 구현이 되지 않을까 생각해봅니당. 그리고 조사하신 배민방식은 페이지처럼 한페이지에 4개씩 백엔드에서 제공하는 것일까요??
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.
배민페이지 방식에 확실히는 모르겠습니다. 6개씩 한 줄로 나오는 방식이긴 한데 반응형은 아니였습니다.
그래서 그냥 프론트에서도 받은 전체 상품을 6개씩 나눠도 할 수는 있을 것 같다고 생각했습니다.
); | ||
}; | ||
|
||
const CartList = ({ isError, isLoading, data, error }) => { |
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.
CartList파일에서 CartItem과 CartListBlock은 파일을 구분지어 import받아오는 방식이
해당 파일과 코드에 대한 가독성을 더 높힐 수 있지 않을까 생각됩니다.
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-query 상태와 redux 상태를 setTimeout으로 동기화 하였는데 react-query 요청 대기기간이 길어지면 오류 가능성이 있음
Step 1
📝 Requirements
필수 요구사항
React Testing Library
&Jest
를 활용해 자유로운 단위의 테스트를 진행한다.GNB
로고
를 누르면 상품목록 페이지로 이동한다.장바구니
버튼을 누르면 장바구니 페이지로 이동한다.주문목록
버튼을 누르면 주문목록 페이지로 이동한다.상품목록
심화 요구사항
User Flow Diagram
혹은Flow Chart
작성Edge Case
대응후기