-
Notifications
You must be signed in to change notification settings - Fork 39
[김지현] sprint6 #185
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 #185
The head ref may contain hidden characters: "React-\uAE40\uC9C0\uD604-sprint6"
Conversation
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.
지현님 6주차 미션 수고하셨습니다.
다음에는 배포 주소도 같이 올려주시면 더 좋을 것 같아요!
다음 7주차 미션도 화이팅입니다!
| const buttonStyles = { | ||
| login: "w-128 h-48 rounded-lg bg-blue100 text-lg text-white font-semibold", | ||
| shopping: | ||
| "w-357 h-56 rounded-[40px] bg-blue100 text-2lg tablet:text-xl text-white font-semibold", | ||
| additem: | ||
| "py-8 px-24 rounded-lg bg-blue100 text-lg text-white font-semibold", | ||
| upload: | ||
| "w-74 h-42 rounded-lg bg-blue100 text-lg text-white font-semibold disabled:bg-gray400 disabled:cursor-not-allowed", | ||
| }; | ||
|
|
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.
💊 제안
모든 버튼 스타일에서 bg-blue100 text-white font-semibold가 중복되는 것 같아요~
(disabled:bg-gray400 disabled:cursor-not-allowed도 다 공통 적용되어야 하지 않을까싶습니다)
이런 경우 중복되는 친구들을 아래처럼 작성해주시면 코드가 더 명확해집니다.
| const buttonStyles = { | |
| login: "w-128 h-48 rounded-lg bg-blue100 text-lg text-white font-semibold", | |
| shopping: | |
| "w-357 h-56 rounded-[40px] bg-blue100 text-2lg tablet:text-xl text-white font-semibold", | |
| additem: | |
| "py-8 px-24 rounded-lg bg-blue100 text-lg text-white font-semibold", | |
| upload: | |
| "w-74 h-42 rounded-lg bg-blue100 text-lg text-white font-semibold disabled:bg-gray400 disabled:cursor-not-allowed", | |
| }; | |
| const buttonStyles = { | |
| login: "w-128 h-48 rounded-lg text-lg ", | |
| shopping: "w-357 h-56 rounded-[40px] text-2lg tablet:text-xl", | |
| additem: "py-8 px-24 rounded-lg text-lg", | |
| upload: "w-74 h-42 rounded-lg text-lg", | |
| }; | |
| // 사용시 | |
| // <button className={`cursor-pointer bg-blue100 text-white font-semibold disabled:bg-gray400 disabled:cursor-not-allowed ${buttonStyles[type]}`} |
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 과 textarea 두개의 컴포넌트로 나누시는 게 더 명확하고 좋을 것 같아요.
또한 버튼에서 코멘트 드린것과 같이 type은 input의 타입을 넘기게 하는것이 좋으니 더욱 분리하시면 좋을 것 같아요!
| if (type === "input") { | ||
| return ( | ||
| <div className="flex flex-col justify-center gap-16"> | ||
| <div className="text-2lg font-bold">{label}</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.
❗️ 수정요청
label 태그로 작성하시고 input이랑 연결해주세요~
| @@ -1,8 +1,19 @@ | |||
| function Button({ children, onClick }) { | |||
| function Button({ children, type, onClick, disabled }) { | |||
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은 일반적으로 버튼 태그의 type: "button" | "reset" | "submit" 을 의미합니다.
type은 중요한 속성이니 type prop가 "button" | "reset" | "submit"을 받게 하시고, style의 종류는 variant와 같은 이름으로 변경하시는 것을 추천드립니다.
| }; | ||
|
|
||
| const handleEnterTag = (e) => { | ||
| if (e.key === "Enter" && !e.nativeEvent.isComposing) { |
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.
👍 칭찬
isComposing 쓰신거 잘하셨어요!
| import Header from "../components/Header"; | ||
| import InputField from "../components/InputField"; | ||
| import plus from "../assets/icons/plus.svg"; | ||
| import x from "../assets/icons/x.svg"; |
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.
💊 제안
CloseIcon 과 같은 명확한 이름을 추천드려요!
| <img | ||
| className="size-22 cursor-pointer" | ||
| src={x} | ||
| onClick={() => handleDeleteTag(index)} | ||
| /> |
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.
❗️ 수정요청
이런 클릭 가능한 요소의 경우 버튼을 사용해주세요. 더 적절한 태그가 있는데 이미지 태그에 onClick 을 주게되면 button에게 기대하는 동작을 제대로 수행하지 못합니다!
| <img | |
| className="size-22 cursor-pointer" | |
| src={x} | |
| onClick={() => handleDeleteTag(index)} | |
| /> | |
| <button type="button onClick={() => handleDeleteTag(index)}> | |
| <img | |
| className="size-22" | |
| src={x} | |
| /> | |
| </button> |
| <img className="size-48" src={plus} /> | ||
| 이미지 등록 | ||
| <input | ||
| className="hidden" | ||
| type="file" | ||
| onChange={handleUploadImg} | ||
| ref={imgRef} | ||
| disabled={imgPreview} | ||
| /> |
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.
💊 제안
이미지 파일만 받을 수 있도록 구현하시면 더 좋을 것 같아요~
참고로 accept 속성을 이용하시고, handleUploadImg 함수 내부에서 추가적으로 확장자를 한번 더 검사하셔야합니다.
accpet 속성은 제안일 뿐 제한이 아니기 때문에 유저가 accept의 명시된 확장자 이외의 파일도 올릴 수 있습니다~
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.
💊 제안
저는 관련있는 것들을 잘 모아두는 것이 가독성을 높여준다고 생각합니다. 지금의 경우 아이템을 생성하는 폼과 관련된 state 들을 각각의 useState로 관리하고 있습니다.
이런 경우 formValue 와 같은 하나의 state로 관리하시거나, useReducer를 이용해보시면 더 가독성 좋게 코드를 작성하실 수 있습니다.
https://ko.react.dev/learn/choosing-the-state-structure
https://ko.react.dev/learn/extracting-state-logic-into-a-reducer
이는 알고만 계시다가 추후 typescript 적용하실때 다시 생각해보시면 도움이 되실 것 같아요.
| onChange={(e) => setItemDetail(e.target.value)} | ||
| placeholder="상품 소개를 입력해주세요" | ||
| /> | ||
| <InputField |
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.
❗️ 수정요청
판매가격은 숫자만 받게 해주세요~
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게