-
Notifications
You must be signed in to change notification settings - Fork 39
[이지현] Sprint6 #194
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 #194
The head ref may contain hidden characters: "React-\uC774\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번째 미션 작업 고생하셨어요~
5번째 PR에 적은 코멘트와 겹치는 내용은 드리지 않았으니,
참고해주시면 좋겠습니다~
다음 미션도 화이팅이에요!
| )} | ||
| <input | ||
| type="file" | ||
| accept="image/*" |
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
| <button onClick={handleImageUpload} css={addItemImageButtonStyle}> | ||
| <img src={plusImage} alt="이미지 등록" /> | ||
| <p>이미지 등록</p> | ||
| </button> |
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로 하시고 file 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.
👍 칭찬
이미지 업로드 컴포넌트 잘 분리해주셨고 에러 메시지도 잘 추가해주셨어요~
| @@ -0,0 +1,49 @@ | |||
| import { css } from "@emotion/react"; | |||
|
|
|||
| export default function AddItemTop({ isDisabled, onDisableChange }) { | |||
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.
💊 제안
AddItemHeader같은 이름이 더 적절할 것 같아요~
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 두개의 컴포넌트로 나누시는 게 더 명확하고 좋을 것 같아요.
지금과 같은 구조에서는 textarea의 경우 필요없는 prop을 받아야하고 이로인해 명확성도 떨어지는 것 같아요.
특히 type의 경우 input에서는 input 태그의 type 속성이고 InputField 컴포넌트에서는 textarea 이거나 input 태그의 type 속성 이라 더 모호한 것 같아요~
| }; | ||
|
|
||
| return ( | ||
| <main css={addItemContainer}> |
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 태그로 감싸고 등록 버튼이 type submit이면 시멘틱하고 좋을 것 같네요.
| }, 0); | ||
| }; | ||
|
|
||
| const handleTagKeyPress = (e) => { |
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를 통해 로직을 구성하신 것 같아요~
이럴때 키보드 이벤트의 isComposing 속성을 이용해서 로직을 짜실 수도 있습니다.
| {tags.map((tag, index) => ( | ||
| <span key={index} css={tagStyle}> | ||
| #{tag} | ||
| <img |
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 메소드를 태그에 연결하실때는 버튼 태그를 최우선적으로 고려하시면 좋겠습니다~
| <img | |
| <button type="button"> | |
| <img |
| setItemImage(file); // 선택한 이미지 상태 업데이트 | ||
| }; | ||
|
|
||
| const handlePriceChange = (e) => { |
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.
💬 여담
이 로직도 잘 구현하셨습니다!
이런 로직을 구현하실 때는 다양한 테스트를 해보시는 것도 중요합니다.
나중에 판매가격 인풋에서 "," 뒤에 커서를 위치시키시고 값을 지워보시고 이를 개선해보시면 더 좋을 것 같아요.
Github Repo: https://github.com/two678/15-Sprint-Mission/tree/React-%EC%9D%B4%EC%A7%80%ED%98%84-sprint5
배포사이트: https://pandamaket-jihyun.netlify.app/
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게