-
Notifications
You must be signed in to change notification settings - Fork 39
[전지윤] sprint6 #202
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 #202
The head ref may contain hidden characters: "React-\uC804\uC9C0\uC724-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번째 미션 제출 고생하셨습니다.
시간이 촉박하게 작업하셔서 그렇겠지만 나중에는 커밋을 더 쪼개시고 반응형 작업도 해주시면 더 좋을 것 같아요.
다음 미션도 화이팅입니다!
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가 custom hook으로 분리되고 UI 들도 컴포넌트로 분리되면,
가독성 측면에서 더 좋을 것 같아요.
컴포넌트로 분리하는 것은 생각의 단위를 나누는 것과 같이 때문에 너무 크게 가지고 가시면 로직을 파악하기도 어렵고 유지보수시에도불리합니다!
| tag: [], | ||
| }); | ||
| const inputRef = useRef(); | ||
| const active = Object.values(formData).every((value) => |
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.
💊 제안
boolean 타입임을 알 수 있고 어떤 값인지 알 수 있는 이름을 추천드려요~
| const active = Object.values(formData).every((value) => | |
| const isFormValid = Object.values(formData).every((value) => |
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.
❗️ 수정요청
요구사항에서 이미지가 없어도 '등록' 버튼이 활성화되게 하라고 되어 있어요~ 확인 후 요구사항대로 구현하시면 더 좋을 것 같아요!
|
|
||
| return ( | ||
| <> | ||
| <form className={styles.form} onSubmit={handleSubmit}> |
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.
👍
| <> | ||
| <form className={styles.form} onSubmit={handleSubmit}> |
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.
❗️ 수정요청
form 하나만 반환하고 있어서 Fragment로 감쌀 필요가 없을 것 같아요.
| <> | |
| <form className={styles.form} onSubmit={handleSubmit}> | |
| <form className={styles.form} onSubmit={handleSubmit}> |
| <input | ||
| id="input-image" | ||
| type="file" | ||
| accept="image/png, image/gif, image/jpeg" |
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의 accept 속성은 유저가 어떤 파일을 올려야하는지에 대한 힌트를 제공하는 속성입니다.
유저는 파일 업로드시 accept의 명시된 확장자 이외의 파일도 올릴 수 있으므로
실제 upload 함수에서 한번더 확장자를 검사해주시는 것이 좋습니다.
(사용자가 업로드창에서 옵션을 열어 확장자를 바꾸면 아래처럼 보입니다)

https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/accept
| </header> | ||
| <div className={styles["form__body"]}> | ||
| <div className={styles["input__image"]}> | ||
| <label |
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.
❗️ 수정요청
지금 이미지 삭제 버튼을 눌렀을 때 formData 값만 변경하고 있어서 제대로 동작하지 않습니다.
동일한 이미지를 등록했다가 지우시고 다시 동일한 이미지를 등록하려고 해보시고 잘 동작하도록 로직을 수정해보시면 좋겠습니다~
| name="price" | ||
| value={formData.price} | ||
| onChange={handleChange} | ||
| placeholder="판맥 가격을 입력해주세요" |
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.
❗️ 수정요청
| placeholder="판맥 가격을 입력해주세요" | |
| placeholder="판매 가격을 입력해주세요" |
| function handleTag(e) { | ||
| if (e.key === "Enter") { | ||
| e.preventDefault(); //폼 내부에서 엔터 -> 자동 제출되는거 막음 | ||
| setTimeout(() => { |
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.
❓ 질문
어떤 이유로 setTimeout으로 태그를 추가하는 로직으로 짜셨을까요?
제 생각에는 그럴 이유가 없을 것 같아서요.
만약 한글로 태그 입력시 이를 막기 위해서라면 isComposing 속성을 이용하시면 더 좋을 것 같아요!
https://developer.mozilla.org/ko/docs/Web/API/KeyboardEvent/isComposing
| tag: [...prev.tag, newValue], | ||
| })); | ||
| } | ||
| inputRef.current.value = ""; |
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.
💊 제안
값을 조작하기 위해서 ref를 쓰신 거라면 input value에 state를 연결해 제어 컴포넌트 방식으로 하시는 것이 적절한 방식입니다.
React에서는 DOM을 직접 조작하기보다는 상태 기반으로 데이터 흐름을 제어하는 것이 권장되며 ef는 실제 포커스 조작이나 스크롤 이동 등 DOM에 직접 접근해야 할 경우 사용하시면 됩니다~
| <input | ||
| id="input-price" | ||
| className={styles["input__short-text"]} | ||
| type="text" |
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 type number가 더 적절할 것 같습니다!
요구사항
배포 링크
https://react-15-7.netlify.app/
기본
심화
스크린샷
데스크탑
비어있는 input이 존재하면 '등록'버튼 비활성화
태블릿
모바일
멘토에게