Conversation
There was a problem hiding this comment.
윤수님 고생하셨습니다.
전반적으로 기능적인 구현에 대해서는 잘 해주셨지만
몇가지 생각해봐야 할 부분이 있을 것 같습니다.
우선 PR은 내 코드가 main branch에 merge 되기 전 마지막 단계이기 때문에
불필요한 console이나 주석은 제거하고 올려주시는 것이 좋습니다.
두번째로 스타일 컴포넌트를 사용하시는 것 같은데
스타일 컴포넌트도 하나의 컴포넌트처럼 취급 될 수 있기에 컴포넌트 이름 짓는 것 처럼
대문자로 지어주셔야 합니다!
세번째로 코드에 작성드리지는 않았지만, 비슷한 구조로 이루어져 있는 함수나
비슷한 역할로 사용되는 state가 있다면 하나로 합쳐서 관리하는 방법도 고민해 보시면 좋을 것 같아요
예를 들면 지금 회원가입 하는 부분에서 state가 여러개 사용되고 있고,
해당 state의 값을 변화시키는 함수의 구조가 비슷한 것들이 있는데
이 부분들은 한번 고민해 보셔서 다른 방법으로 관리하는 것을 고민해 보세요!
| export default function AddLink() { | ||
| return ( | ||
| <F.addLink> | ||
| <div style={{ width: "20px", height: "20px", position: "relative" }}> |
There was a problem hiding this comment.
인라인 스타일은 가급적 사용을 지양해주세요!
| userName: "", | ||
| img: "", | ||
| }); | ||
| const [login, setLogin] = useState<string | null>(null); |
There was a problem hiding this comment.
| const [login, setLogin] = useState<string | null>(null); | |
| const [login, setLogin] = useState<string>(""); |
해당 state에 null이 들어오게 되는 경우가 아니라면 state의 기본값을 넣어주는 것이 좋습니다.
그리고 userData와 login이라는 state는 Header에서만 사용되는 부분이기에
Header에서 선언하고 사용하는 것이 좋겠네요!
App 컴포넌트에서 사용하게 되면 불필요한 렌더링이 발생할 수 있습니다.
| </F.addLink> | ||
| ) : null} | ||
| </F.Main> | ||
| <F.Main ref={targetRef}>{scrollAddUrl ? <AddLink /> : null}</F.Main> |
There was a problem hiding this comment.
| <F.Main ref={targetRef}>{scrollAddUrl ? <AddLink /> : null}</F.Main> | |
| <F.Main ref={targetRef}>{scrollAddUrl && <AddLink />}</F.Main> |
&& 연산자를 이용하면 불필요한 null을 입력하지 않아도 됩니다
| // e.preventDefault(); | ||
|
|
||
| try { | ||
| const res = await signin(userData); | ||
| console.log(res); |
There was a problem hiding this comment.
PR 올리실 때 console이나 주석은 제거해주세요!
코드 가독성에 영향을 미칩니다
| required | ||
| /> | ||
| {emailError && <p style={{ color: "red" }}>{emailError}</p>} | ||
| {wrongMail ? null : <p style={{ color: "red" }}>이메일이 중복되었습니다.</p>} |
There was a problem hiding this comment.
| {wrongMail ? null : <p style={{ color: "red" }}>이메일이 중복되었습니다.</p>} | |
| {!wrongMail && <p style={{ color: "red" }}>이메일이 중복되었습니다.</p>} |
요렇게 할 수도 있겠죠?
| const [showPassword, toggleShowPassword] = useToggle(false); | ||
| const [showConfirmPassword, toggleShowConfirmPassword] = useToggle(false); |
There was a problem hiding this comment.
기본값이 boolean 값일 경우 state의 이름은 의문형으로 지어주는 것이 좋습니다.
|
질문에 대한 제 생각을 말씀드려보자면! 이미 에러 문구에 답은 나와있다고 볼 수 있겠습니다. toggleShowPassword 함수는 useToggle이라는 훅에서 만들어서 가져온 함수인데 하지만 img 태그에서 사용하는 onClick 이벤트에는 MouseEventHandler 타입이 필요하다는 그러나 useToggle이라는 함수는 img 태그에서만 사용되는 태그가 아니기 때문에 |
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게