-
Notifications
You must be signed in to change notification settings - Fork 57
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
[이윤경] week12 #368
The head ref may contain hidden characters: "part3-\uC774\uC724\uACBD"
[이윤경] week12 #368
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.
Next와 TS로 깔끔하게 마이그레이션 잘 하셨네요. Input도 focus out시 실행할 핸들러를 지정하는 부분은 조금 더 수정이 필요해보이긴 하지만, 잘 구현해주신 것 같습니다. 특히 변수/함수 이름을 적절하게 잘 지으셔서 쉽게 이해할 수 있었습니다. 바쁘시겠지만, 다음 번엔 커밋을 나눠 주시면 더 읽기 쉬울 것 같습니다. 바쁜 스케쥴에도 불구하고 고생하셨습니다!! 👍
const [inputType, setInputType] = useState(type); | ||
const [isError, setIsError] = useState<boolean>(false); | ||
|
||
const togglePasswordVisibility = (e: React.MouseEvent<HTMLButtonElement>) => { |
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.
이름 잘 지으셨네요 👍
errorMessage?: string; | ||
} | ||
|
||
function AuthInput({ label, type, placeholder, onValid, errorMessage }: AuthInputProps) { |
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>
은 더 많은 attribute가 있으므로 나머지 모든 prop을 ...rest
등으로 받아서 <input>
에 적용해주시면 좋을 것 같습니다.
label: string; | ||
type: "text" | "email" | "password"; | ||
placeholder?: string; | ||
onValid?: () => boolean; |
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.
onValid
로 onBlur
의 역할을 구현하신 것 같은데, 이 이름은 onValid
보다는 onBlur
같은 게 되는 것이 더 적절해 보입니다. Validation check를 수행하는 건 Blur가 되었을 때 일어나는 여러 일 중에 하나이기 때문입니다.
/> | ||
{type === "password" && ( | ||
<button className={styles.eyeButton} onClick={togglePasswordVisibility}> | ||
<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.
Next/Image를 사용하셨군요 👍
return ( | ||
<nav className={styles.nav}> | ||
<div className={styles.navContainer}> | ||
<Link href="/"> |
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.
👍
} | ||
|
||
function AuthInput({ label, type, placeholder, onValid, errorMessage }: AuthInputProps) { | ||
const [inputType, setInputType] = useState(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.
prop으로 받아온 값을 state의 초기값으로 지정하신 것 같네요. 잘 구현해주셨습니다.
/> | ||
</button> | ||
)} | ||
{isError && <p className={styles.errorMessage}>{errorMessage}</p>} |
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.
더 정확히 하려면 isError && errorMessage && <p ...
가 되어야 될 것 같습니다. errorMessage
가 없으면 p
요소 자체가 생성이 안 되면 더 좋을 것 같습니다.
요구사항
기본
주요 변경사항
멘토에게
매운맛을 바라지만 또 순한맛을 바라고.... 제 코드만큼 혼란스럽군여..(커밋도 차근차근 수정하지 못하다 보니 커밋도 까먹어버렸네요,, 호호 )