-
Notifications
You must be signed in to change notification settings - Fork 21
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 #85
The head ref may contain hidden characters: "React-\uAE40\uD76C\uC9C4-sprint6"
[김희진] sprint6 #85
Conversation
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 ( | ||
<div className="TagCard"> | ||
<span>#{name}</span> | ||
<img src={deleteIcon} alt="삭제 아이콘" onClick={handleOnClick} /> |
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.
P3:
버튼 태그가 더 어울릴 것 같습니다.
<label htmlFor={inputId} className="TextareaBar-label"> | ||
{label} | ||
</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.
P2:
label 태그와 input 태그를 연결해주신 것은 좋습니다만 이렇게 되면
inputId가 넘겨지지 않았을때 default 값인 input으로 id가 정해진 input들이 많이 생길 수 있습니다.
(이러한 방식이 inputBar에서도 동일함을 고려하면 그런 가능성은 더욱 크다고 보입니다.)
가능하면 아래의 hook이나 id 생성 라이브러리를 사용해서 이러한 일을 피하세요.
- React useId
https://ko.react.dev/reference/react/useId - Lodash uniqueId
https://lodash.com/docs/4.17.15#uniqueId
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.
P3:
Textarea
,Input
과 같은 이름을 추천드립니다.
@@ -0,0 +1,11 @@ | |||
import './PrimaryButton.css'; | |||
|
|||
function PrimaryButton({ children, onClick, disable = false }) { |
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.
P2:
type도 받을 수 있으면 더 좋을 것 같습니다.
@@ -0,0 +1,11 @@ | |||
import './PrimaryButton.css'; | |||
|
|||
function PrimaryButton({ children, onClick, disable = false }) { |
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.
P2:
컴포넌트 속성명은 가능하면 tag의 속성과 동일하게 쓰는 것이 사용성에서 더 좋습니다.
function PrimaryButton({ children, onClick, disable = false }) { | |
function PrimaryButton({ children, onClick, disabled = false }) { |
onChange, | ||
onKeyDown, | ||
}) { | ||
const eventProps = { | ||
...(onKeyDown && { onKeyDown }), | ||
...(onChange && { onChange }), | ||
}; | ||
|
||
return ( | ||
<section className="InputBar"> | ||
{label && ( | ||
<label htmlFor={inputId} className="InputBar-label"> | ||
{label} | ||
</label> | ||
)} | ||
<input | ||
className="InputBar-input" | ||
type={type} | ||
id={inputId} | ||
name={name} | ||
value={value} | ||
placeholder={placeholder} | ||
disabled={disabled} | ||
{...eventProps} | ||
/> | ||
</section> |
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.
P1:
아래처럼 하셔도 동일동작합니다.
onChange, | |
onKeyDown, | |
}) { | |
const eventProps = { | |
...(onKeyDown && { onKeyDown }), | |
...(onChange && { onChange }), | |
}; | |
return ( | |
<section className="InputBar"> | |
{label && ( | |
<label htmlFor={inputId} className="InputBar-label"> | |
{label} | |
</label> | |
)} | |
<input | |
className="InputBar-input" | |
type={type} | |
id={inputId} | |
name={name} | |
value={value} | |
placeholder={placeholder} | |
disabled={disabled} | |
{...eventProps} | |
/> | |
</section> | |
...eventProps | |
}) { | |
return ( | |
<section className="InputBar"> | |
{label && ( | |
<label htmlFor={inputId} className="InputBar-label"> | |
{label} | |
</label> | |
)} | |
<input | |
className="InputBar-input" | |
type={type} | |
id={inputId} | |
name={name} | |
value={value} | |
placeholder={placeholder} | |
disabled={disabled} | |
{...eventProps} | |
/> | |
</section> |
type="file" | ||
name={name} | ||
id="file" | ||
accept="image/png, 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.
P2:
accept 속성은 유저가 파일선택시 가능한 파일 확장자를 "제안"하는 역할을 합니다.
여기서 중요한점은 제한하는 것이 아니라는 것입니다. 유저는 해당 속성외의 파일을 업로드할 수 있습니다.
따라서 accept로 제안한 확장자 이외의 것을 업로드할 시 onChange
같은 함수에서 이를 확인해
업로드하지 않는 로직이 필요합니다.
function ItemForm({ initialValues = INITIAL_VALUES }) { | ||
const [formData, setFormData] = useState(initialValues); | ||
|
||
const isFormComplete = () => { |
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.
P2:
함수니 이를 알 수 있는 이름이면 좋을 것 같습니다.
checkFormIsValid
같은 이름을 추천드립니다.
const handleChange = (e) => { | ||
const { name, value } = e.target; | ||
setFormData((prevData) => ({ | ||
...prevData, | ||
[name]: 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.
P3:
input의 name속성을 이용해 코드를 정리하신 점이 좋네요 👍
})); | ||
}; | ||
|
||
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.
P2:
태그 input에서 한글을 입력한 후 enter를 해보시면 이벤트가 두번 발생됩니다.
이는 한글 입력시 버그로 확인해보시고 아래 글을 참고해서 수정해보세요~
요구사항
체크리스트 [기본]
체크리스트 [심화]