Feature/13 로그인 회원가입 리팩토링 #15
Conversation
hwookim
left a comment
hwookim
left a comment
There was a problem hiding this comment.
고생하셨습니다 찬님! 👍🏼 👍🏼
로고가 멋지네요 😃
몇가지 보이는 부분에 관하여 리뷰를 남겼으니 한 번 확인해보세요~
추가적으로 불필요한 파일 (storybook 관련 기본 assets등)은 삭제하는 편이 좋을 것 같아요~!
PR 작성 방식에 관한 이야기인데요.
현재 같은 작업 내용에 대하여 PR을 두개 올려주신 것으로 보입니다.
개발/리팩토링으로 PR이 두개 나뉘었어야할 이유가 있을까요?
나뉘더라도, 현재의 PR이 기존의 #12 에서 시작된 리팩토링이라면, PR의 방향은 main이 아닌 #12의 브랜치여야합니다.
리뷰하는 입장에서는 어느 코드를 봐야할지 모르게 되어 혼란스럽습니다. 😅
우선은 최종본으로 보이는 현 PR에 리뷰 달도록 할게요~!
고생하셨습니다 👍🏼 👍🏼
| export interface JoinProps { | ||
| email: string; | ||
| nickname: string; | ||
| password: string; | ||
| confirmPassword: string; | ||
| } |
There was a problem hiding this comment.
주로 컴포넌트에 Props가 있다면 해당 인터페이스는 컴포넌트에 대한 props type 명시라고 인식될 것 같아요.
과연 이 인터페이스가 컴포넌트에 엮여있는 인터페이스일까요? 제가 볼 때는 request에 필요한 내용 같아 보이네요.
컴포넌트와 연관성을 다시 한 번 고려해보시고, Props라는 명칭보다는 구분되는 다른 명칭을 사용하는 편이 좋겠네요 👀
| @@ -0,0 +1,12 @@ | |||
| // 로컬스토리지에서 토큰 가져오기 | |||
| export const getToken = (): string | null => { | |||
| return localStorage.getItem('token'); | |||
There was a problem hiding this comment.
이런 localStorage key같은 경우엔 직접 사용하다보면 오타가 나서 오류가 나는 경우가 굉장히 많아요.
상수를 이용하여 미리 선언해두면 그럴 일이 없지 않을까요? 😄
There was a problem hiding this comment.
get, set, remove 상수 만들어서 사용하는쪽으로 바꿨습니다!
| if (isLoggedIn) { | ||
| navigate('/'); | ||
| } |
There was a problem hiding this comment.
컴포넌트 렌더링 시에 영향을 끼치는 작업들은 useEffect 내부에서 처리해야합니다.
물론 지금은 아무런 문제가 없어 보이고 잘 리다이렉트 되는 것 같지만,
이는 불필요한 렌더링을 야기할 수 있기 때문에 react에서는 권장하지 않는 형태입니다 😄
There was a problem hiding this comment.
warning이 떠서 삭제했습니다!
| const handleInputFocus = () => setIsFocused(true); | ||
| const handleInputBlur = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| setIsFocused(false); | ||
| setHasValue(e.target.value !== ''); | ||
| }; |
There was a problem hiding this comment.
만약 외부에서 props로 onFocus, onBlur를 준다면 어떻게 될까요?
isFocused가 정상적으로 동작할까요? 🤔
There was a problem hiding this comment.
멘토님 리뷰 감사합니다! 말씀 주셨던 부분은 최대한 수정하려고 했습니다! isFocused 동작에 대해서 신경쓰지는 못했는데 확인 후 수정해보겠습니다
#️⃣연관된 이슈
#13
📝작업 내용
로그인 상태관리 + 전체적인 코드 리팩토링
스크린샷 (선택)
💬리뷰 요구사항(선택)