-
Notifications
You must be signed in to change notification settings - Fork 39
[맹은빈] sprint6 #211
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 #211
The head ref may contain hidden characters: "React-\uB9F9\uC740\uBE48-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.
💊 제안
input 과 textarea 두개의 컴포넌트로 나누시는 게 더 명확하고 좋을 것 같아요.
지금과 같은 구조에서는 textarea의 경우 필요없는 prop을 받아야하고 이로인해 명확성도 떨어지는 것 같아요.
특히 type의 경우 input에서는 input 태그의 type 속성이고 Input 컴포넌트에서는 'textarea' 이거나 input 태그의 type 속성 이라 더 모호한 것 같아요~
| placeholder = '입력', | ||
| value, | ||
| onChange, | ||
| inputName = '상품', |
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이 더 적절할 것 같아요~
하단의 h2태그도 label 태그로 바꿔주시고 input이랑 연결해주시면 더 좋겠습니다!
| tagInput, | ||
| handleTagChange, | ||
| handleTagKeyDown, | ||
| 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.
❗️ 수정요청
타입을 추측할 수 있게 tags, tagList와 같은 이름을 추천드려요!
| type='button' | ||
| onClick={() => handleRemoveTag(tag)} | ||
| className='text-white text-[1.6rem] ml-1 bg-gray-500 rounded-3xl' | ||
| > | ||
| × | ||
| </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.
❗️ 수정요청
디자인과 같은 X 버튼이면 좋겠어요!
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.
💊 제안
컴포넌트로 분리하신 점은 좋습니다. 다만 지금의 분리의 경우 그냥 add-item 페이지가 다른 컴포넌트로 분리된 것이라 분리의 장점이 없다고 생각합니다. src/pages/items/index.jsx 처럼 해당 페이지 로직들이 src/pages/add-item/index.jsx에 있는 것이 더 구조상 장점이 있을 것 같아요!
분리를 하신다면 페이지 헤더와 인풋들을 각각 분리하시고 src/pages/add-item/index.jsx에서 조합하시는 것을 추천드립니다!
|
|
||
| <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
| value={formData.name} | ||
| onChange={handleChange} |
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.
💊 제안
지금처럼 상품명을 useState로 제어하면 사용자가 입력할 때마다 컴포넌트가 리렌더링됩니다.
지금 코드상에서는 상품명을 바꿔주는 것이 아니라 제어 컴포넌트로 관리할 필요가 없을 것 같아요!
이를 비제어 컴포넌트로 바꾸셔서 불필요한 리렌더링을 줄이시는 것을 추천드려요!
| value={formData.name} | |
| onChange={handleChange} | |
| onChange={handleChange} |
| if (e.key === 'Enter') { | ||
| e.preventDefault(); | ||
| const value = tagInput.trim(); | ||
| if (value && !formData.tag.includes(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.
💊 제안
중복되는 태그가 생성되지 않도록 해주신 점 좋습니다!
다만 이렇게 되면 사용자가 해당 동작에 대한 피드백을 받지 못하므로, alert, toast, input error 와 같은 방식으로 피드백을 주시면 더 좋을 것 같아요.
| setTagInput(e.target.value); | ||
| }; | ||
|
|
||
| const handleTagKeyDown = (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.
❗️ 수정요청
한글로된 태그를 생성할 때 두번 입력이 발생하는 것 같은 현상이 있습니다. 이는 자모음으로 이루어진 한글을 입력중에 발생하는 현상입니다~
배포사이트에서 한글로 태그를 생성해보시고, 위의 동작을 고쳐보세요~
| /> | ||
|
|
||
| <Input | ||
| 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.
❗️ 수정요청
이런 경우 number 가 더 적절할 것 같아요!
| type='text' | |
| type='number' |
배포링크 https://serene-torte-71ea00.netlify.app/
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게