-
Notifications
You must be signed in to change notification settings - Fork 24
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
[배진한] Sprint6 #106
The head ref may contain hidden characters: "react-\uBC30\uC9C4\uD55C-sprint6"
[배진한] Sprint6 #106
Conversation
@@ -0,0 +1,3 @@ | |||
export const url = new URL('https://panda-market-api.vercel.app/products'); | |||
// export const url = new URL('http://localhost:8000/products'); | |||
export const postUrl = 'http://localhost:8000/products'; |
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을 적용 시켜야 할 것 같습니다.
localhost는 배포를 해도 다른 컴퓨터에서 응답을 안 하겠네요
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.
맞습니다. 우선 그 부분 인지하고 있으시다면 문제 없겠습니다.
복잡하실테니 대표적으로 두가지 경우를 미리 알려드릴게요
- 백엔드주소가 바뀔 때마다 저 값을 수동으로 바꿔 배포하기
- 환경변수에 넣고, 빌드시에 빌드 런타임인 node.js가 process.env를 읽어서 저 값을 채우게 하기
이렇게 하는 방법들이 있습니다.
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 { url } from './endpoint.js'; | ||
|
||
async function getProducts(params = []) { | ||
const query = new URLSearchParams(params).toString(); |
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.
new URLSearchParams(params)
인데, params의 기본 타입은 any[]
입니다. 그런데 한번 이 타입으로 실제로 실행해보시면, URLSearchParams의 constructor에는 [name, value]
tuple이 iterable해야한다는 설명이 나옵니다.
TypeError [ERR_INVALID_TUPLE]: Each query pair must be an iterable [name, value] tuple
설명이 상당히 어려워보이지만, 간단합니다. 그저 [이름, 값]
들이 반복하여 들어가야한다는 겁니다. 예를 들어,
new URLSearchParams([['key1', 'hello'], ['key2', 'world']])
이렇게 되어야 정상이라는거죠. 그러면 이런식으로 쓰고 있을까요? 그렇지않습니다.
sprint6/src/pages/ItemsPage/index.jsx의 PRODS_LIST는 string입니다. 물론, string이 query로 파싱 가능한 형태이기 때문에 잘 동작은 합니다만, 그렇다면 getProducts
의 parameter인 params
에는 어떻게 설명이 되어야할까요?
제 답은, argument를 string으로 쓰고 있다면 string으로 초기화해둘 것
이 정답이라고 봅니다. TypeScript라면 다른 답이 있지만, JavaScript에서는 이게 최선의 답으로 보이네요.
jsdoc으로 예제를 몇개 달아두는것도 좋습니다.
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.
params가 new URLSearchParams
의 constructor로 들어가기 전에, 들어가도 되는지
체크하는 로직이 위에 있는게 최선이지만,
저는 차선책으로 params = ''
하시는 편을 고려했었습니다.
왜냐면 위에서는 params = []
로 하셨기때문에, 이 해결책을 더 선호하시리라 생각한거죠...
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.
네 일단 차선책으로 수정 해 놓겠습니다!
sprint6/src/api/postProductApi.js
Outdated
const postProduct = async (surveyData) => { | ||
const res = await axios.post(postUrl, surveyData); | ||
return res.data; | ||
} | ||
|
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.
type check가 안되기때문에, 요청 전에 surveyData가 javascript object인지 확인하는 과정이 있으면 더욱 좋겠습니다.
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.
예 의도는 맞게 보셨습니다.
그런데 둘째로, surveyData
에 들어갈 값들이 온전히 다 들어간게 맞는지 보셔야 할 필요도 있습니다.
우선은 이 정도로도 괜찮지만, 한번 validation을 진행해보세요.
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.
아 네 일단 surveyData의 형식과 유효성을 체크해야 한다는 것은 인식하고 넘어 가겠습니다.
<a href="https://www.instagram.com/sem/campaign/emailsignup/?campaign_id=13530338586&extra_1=s%7Cc%7C547419126947%7Ce%7Cinstagram%20c%7C&placement=&creative=547419126947&keyword=instagram%20c&partner_id=googlesem&extra_2=campaignid%3D13530338586%26adgroupid%3D126262419014%26matchtype%3De%26network%3Dg%26source%3Dnotmobile%26search_or_content%3Ds%26device%3Dc%26devicemodel%3D%26adposition%3D%26target%3D%26targetid%3Dkwd-1321618852491%26loc_physical_ms%3D1009866%26loc_interest_ms%3D%26feeditemid%3D%26param1%3D%26param2%3D&gad_source=1&gclid=CjwKCAjw_4S3BhAAEiwA_64YhgkFInpQexBqyLXjThDfjkEHXMlvBam2vK2b7L7e_xsKy934puQxuBoCe7IQAvD_BwE"><img src={instagramImg} alt="Instagram" /></a> | ||
</div> |
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.
이거 무슨일인가요...? query string 잘 보세요...!
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.
의도가 그저 인스타그램으로 가기
라면 저 쿼리스트링이 왜 더 있어야하는거죠?
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 전체 복사를 한 게 저거였습니다.
미션 1, 2 초반 때 했던 건데 저런 식으로 돼 있었는지 몰랐네요 지금은 쿼리 라인에 대한 지식이 어느정도 있어서 대충 잘 못 된 걸 아는 거고요
#footerContent { | ||
width: 152rem; | ||
height: 2rem; | ||
display: flex; | ||
justify-content: space-between; | ||
margin-left: 20rem; | ||
margin-right: 20rem; | ||
} | ||
|
||
#copyright { | ||
width: 12.2rem; | ||
height: 1.9rem; | ||
font-size: 1.6rem; | ||
font-weight: 400; | ||
line-height: 1.909rem; | ||
display: flex; | ||
text-align: center; | ||
color: #9CA3AF; | ||
} | ||
|
||
#info { | ||
width: 16.9rem; | ||
height: 1.9rem; | ||
gap: 3rem; | ||
opacity: 0rem; | ||
font-size: 1.6rem; | ||
font-weight: 400; | ||
line-height: 1.909rem; | ||
text-align: center; | ||
color: #E5E7EB; | ||
display: flex; | ||
justify-content: space-between; | ||
} | ||
|
||
#info a { | ||
text-decoration: none; | ||
color: #E5E7EB; | ||
} |
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에 id를 쓰셔야만 하는 이유가 있으셨을까요
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 스타일이 중복하여 입혀지기도 하고
애를 먹으면서 그냥 선택을 명확하게 하려 하다보니 그렇게 됐던 것 같습니다.
웬만하면 클래스 선택자를 쓰는 게 좋다고 하셨었죠?
앞으로 참고하겠습니다!
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.
id
를 써야만 한다면 id
가 있는 DOM은 유일해야만 합니다. 그런 이유가 있다면 쓸만합니다.
그래서 그 의도를 더 여쭤본거구요
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.
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.
처음엔 이 Header 컴포넌트를 RegisterPage와 ItemsPage에서 사용하려고 했었는데요
아래 보시면 NavLink로 ItemsPage일 때 중고마켓 글씨에 스타일을 입히려고 했는데 작동을 안해서
그냥 따로 따로 만들었는데 혹시 왜 작동을 안 하는지 아실까요? 강의 보고 적용을 하긴 했는데 잘 안 되네요..
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 path를 먼저 체크해보셔야 할거같은데요
안되는 상황을 만들고 해결하시기보다 되는 상황을 변형해서 되는 상황을 먼저 만들어보세요
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.
아 해결했습니다!
isActive를 구조분해해서 받아 와야 하네요
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하여 사용하지 않는 파일은 삭제해주시거나, 다른 이유로 필요한 파일이면 상단에 주석이라도 달아주세요.
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.
작업 다시 확인해보세요~
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.
아직 push를 안 했습니다.
그리고 해당 페이지는 지금은 사용하지 않지만 나중에 들어가는 걸로 알고 있습니다.
스프린트 미션 완성본 참고해서 만들어 놨습니다!
sprint6/src/pages/HomePage/index.jsx
Outdated
<> | ||
<Header /> | ||
<main> | ||
<Hero /> | ||
<Section1 /> | ||
<Section2 /> | ||
<Section3 /> | ||
<BottomBanner /> | ||
</main> | ||
<Footer /> | ||
</> |
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.
<Header />
와<Footer />
를 가지는 컴포넌트가 3개 발견됩니다. 그러면 해당 레이아웃을 가지는 컴포넌트를 선언하시는건 어떨까요Section1
,Section2
,Section3
이 분리되어야 하는 이유가 있었을까요?
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.
- 와 각각 공용 컴포넌트화 해 놓으라는 말씀이신거죠?
- 저번 코드 리뷰 때 따로 따로 보는 게 복잡하지 않다고 하셔서 그렇게 했는데 HomePage와 같은 페이지는 붙여도 될 것 같네요
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.
제 1번 코멘트에 'Header 와 Footer를'이 생략 됐네요 저만 안 보이는 건지는 모르겠지만..,
이유는 섹션 별로 들어가는 이미지와 글이 달라서 나눴습니다. 하나의 컴포넌트에 다 몰아 넣는 것 보다는 가독성이 좋을 것 같아서요
정신이 없어서 제가 질문의 의도를 다 정확하게 파악하지 못할 수도 있습니다.
Home Page 같은 페이지는 보시면 아시겠지만 그냥 html과 css로만 구성 돼 있는 페이지입니다.
분리 돼야 할 이유가 스타일적인 부분과 코드의 가독성 부분이지 않을까 생각 되는데요
질문하신 의도가 html과 css로만 구성 돼 있기 때문에 굳이 안 나눠도 된다는 건 줄 알았습니다.
const BottomBanner = () => { | ||
return ( | ||
<div class="skyColorSection"> | ||
<div class="skyColorContent"> | ||
<div class="textBox5"> | ||
<div class="text1">믿을 수 있는</div> | ||
<div class="text1">판다마켓 중고 거래</div> | ||
</div> | ||
<div class="imgBox"> | ||
<img src={bottomBannerImg} alt="bottomBannerImg" /> | ||
</div> | ||
</div> | ||
</div> | ||
); | ||
}; | ||
|
||
export default BottomBanner; |
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.
수정했습니다.
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.
작업 확인해보세요~
); | ||
}; | ||
|
||
export default Section2; |
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.
Section3
으로 고쳐주세요.
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.
작업 다시 확인해보세요~
<ProdNameInputBox | ||
inputValue={values.name} | ||
onChange={handleInputChange} | ||
/> | ||
|
||
<ProdDescriptionInputBox | ||
inputValue={values.description} | ||
onChange={handleInputChange} | ||
/> | ||
|
||
<ProdPriceInputBox | ||
inputValue={values.price} | ||
onChange={handleInputChange} | ||
/> | ||
|
||
<ProdTagsInputBox | ||
inputValue={values.tags} | ||
onChange={handleInputChange} | ||
/> | ||
</form> |
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.
input이 각각 validation되어야 하는데, 그 로직이 서로 같지 않은 경우에는 차라리 handler callback함수를 나눠버리세요. 한 바구니에 모든 계란을 담아버리시면 안됩니다.
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.
handleInputChange 이벤트 핸들러를 여러 개로 나누라는 말씀이시죠?
나눠서 각각의 핸들러에서 나온 값을 가지고 버튼을 활성화 하는 방향으로 하면 될까요?
setValues((preValues) => ({ | ||
...preValues, | ||
[name]: value, | ||
})); | ||
console.log("values", values); |
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.
prev
아이디어를 쓰시는건 좋습니다. 그런데 한단계 더 나아가보세요. state와, validation result는 state의 파생값에 불과합니다.
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.
sprint6/src/pages/RegisterItemPage/components/ProdPriceInputBox/index.jsx
리뷰 답변으로 갈음하겠습니다. 지금은 넘기시고, 다른 방법을 강구해보세요.
const [priceValidation, setPriceValidation] = useState(false); | ||
const [emptyPriceInput, setEmptyPriceInput] = | ||
useState("숫자로 입력해주세요"); | ||
|
||
const handlePriceInputBlur = (e) => { | ||
|
||
if (isNaN(e.target.value)) { | ||
setPriceValidation(true) | ||
setEmptyPriceInput("숫자로 입력해주세요"); | ||
} else { | ||
setPriceValidation(false) | ||
setEmptyPriceInput("숫자로 입력해주세요"); | ||
} | ||
|
||
if (e.target.value.length === 0) { | ||
setPriceValidation(!priceValidation); | ||
setEmptyPriceInput("판매 가격을 입력해주세요"); | ||
} | ||
}; | ||
|
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.
다른 InputBox와 handler는 같은데, 에러는 다르다면 나중에 굉장히 괴로울겁니다.
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.
(다른 InputBox 컴포넌트들과 같이 리뷰하자면) validation의 결과로써 error를 state에 저장하고있습니다. 이를 컴포넌트화 시켜서 비슷비슷한 컴포넌트가 각자 다른 역할을 가지게끔 구상하셨죠. 에러메시지가 validation
으로부터 나오는게 아니라 handler
에서 하드코딩되어 지정되고있습니다. 이렇게 하면 에러메시지가 혹여나 다국어지원
을 해야하거나 할 때, 아니면 유저마다 다른 에러메시지를 보여야 할 때 일이 상당히 많아지게됩니다.
지금은 학습단계이시니 여기에서 멈추셔도 됩니다. 하지만 일의 양을 적게 하여 문제를 푸는것은 개발자로서의 삶의 질
과 큰 연관이 있습니다. 나중에라도 다른 방법을 한번 고민해보세요.
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.
👍
📋 스프린트 미션 6 요구사항
🌐 Sprint6_react-배진한
주요 변경 사항
요구사항
기본 요구사항
공통
프론트엔드 구현 요구사항
랜딩 페이지
중고마켓 페이지
상품 등록 페이지
프론트엔드 심화 요구사항
유효한 조건
멘토에게