Feature/19 login #21
Feature/19 login #21
Conversation
dlcks0601
added
✨ Feature
dlcks0601
requested review from
HANTAEDONG,
HongbiKo,
YouD0313,
hwookim and
peppertown
hwookim
left a comment
hwookim
left a comment
There was a problem hiding this comment.
안녕하세요 찬님!
유저 로그인 관련 부분을 하시면서 persist도 사용해보시고 여러모로 노력하시는 모습이 보여서 멋집니다. 👍🏼 👍🏼
몇가지 리뷰를 남겨두었으니 한 번 확인해보시면 좋을 것 같아요!
| if ( | ||
| config.headers && | ||
| 'set' in config.headers && | ||
| typeof (config.headers as AxiosHeaders).set === 'function' | ||
| ) { |
There was a problem hiding this comment.
| if ( | |
| config.headers && | |
| 'set' in config.headers && | |
| typeof (config.headers as AxiosHeaders).set === 'function' | |
| ) { | |
| if (typeof config?.headers?.set === 'function') { |
이런 식으로 간단하게 표기해볼 수도 있겠네요 🤔
| <LogoutBox onClick={handleLogout}> | ||
| <div className='group'> | ||
| <div className='overlap-group'> | ||
| <div className='text-wrapper'>로그아웃</div> | ||
| </div> | ||
| </div> | ||
| </LogoutBox> | ||
| ) : ( | ||
| <LoginBox onClick={handleLogin}> | ||
| <div className='group'> | ||
| <div className='overlap-group'> | ||
| <div className='text-wrapper'>로그인</div> | ||
| </div> | ||
| </div> | ||
| </LoginBox> |
There was a problem hiding this comment.
LoginButton과 LogoutButton의 두 구조가 완전하게 동일하네요.
중복을 줄이려면 어떻게 하는 게 좋을까요?
styled component에 props를 전달하여 내부에서 스타일을 조정할 수도 있는데, 해당 방식을 사용해보는 건 어때요? 👀
| const response = await axios.post( | ||
| 'http://localhost:3333/api/users/login', | ||
| { | ||
| email, | ||
| password, | ||
| } | ||
| ); |
| state.loading = false; | ||
| state.error = null; | ||
|
|
||
| localStorage.setItem('token', action.payload.token); |
There was a problem hiding this comment.
지난 번에 드렸던 key의 상수화 리뷰에 덧붙여서,
아예 유저 관리를 위한 localstorage 접근 자체를 유틸함수화해두면 어떨까요?
반복도 줄고, 실수할 일도 더 줄거에요
| import { AppDispatch } from '@/store/store'; | ||
|
|
||
| export interface LoginProps { | ||
| type LoginFormData = { |
There was a problem hiding this comment.
이 타입은 LoginForm이 기준일까요? 아니면 login api의 request body가 기준일까요?
어떤 걸 기준으로 삼냐에 따라 네이밍도, 관리할 위치도 다시 생각해보면 좋을 거 같아요.
| getDefaultMiddleware({ | ||
| serializableCheck: false, // redux-persist 관련 non-serializable 경고 비활성화 | ||
| }), |
There was a problem hiding this comment.
serializableCheck 옵션을 false로 둘 이유가 어떤 이유일까요? 👀
#️⃣연관된 이슈
#19
📝작업 내용
스크린샷 (선택)
💬리뷰 요구사항(선택)