Skip to content
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

[FE]로그인 유지 및 프로필 탭 구현 #97

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

sirloinbh
Copy link
Collaborator

@sirloinbh sirloinbh commented Sep 13, 2023

Auth토큰이 로컬 스토리지에 있으면 페이지가 렌더링될 때마다 로그인 상태 유지
프로필 컨테이너 삭제
프로필 모달에서 탭 위치 조정
Cash Hook, Cash Reducer에 담기는 변수이름 조정 cahAmount ->moneyAmount, cashId ->moneyId
주석처리 부분은 구글 로그인 구현 미정
money 변수의 타입을 number에서 string으로 변경
Issues #70

Auth토큰이 로컬 스토리지에 있으면 페이지가 렌더링될 때마다 로그인 상태 유지
프로필 컨테이너 삭
프로필 컨테이너 삭제
프로필 모달에서 탭 위치 조정
Cash Hook, Cash Reducer에 담기는 변수이름 조정 cahAmount ->moneyAmount, cashId ->moneyId
Issues #70
Copy link
Collaborator

@sinjw sinjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다

@sirloinbh sirloinbh self-assigned this Sep 13, 2023
@sirloinbh sirloinbh added the FE Front End label Sep 13, 2023
@sirloinbh sirloinbh added this to the Bare Minimum Requirement milestone Sep 13, 2023
@@ -14,38 +14,38 @@ const CashModal: React.FC<CashModalProps> = ({ onClose }) => {
const resetButtonText = "리셋";

const dispatch = useDispatch();
const cashId = useSelector((state: RootState) => state.cash.cashId);
const cashAmount = useSelector((state: RootState) => state.cash.cashAmount) || 0;
const moneyId = useSelector((state: RootState) => state.cash.moneyId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전역상태 타입 지정 관련해서 기존에 있던 StateProps를 활용하는 게 아니라 따로 RootState라는 interface를 만드신 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducer가 아닌 store에서 필요한 상태값들을 가져와서 RootState를 사용했습니다.

Copy link
Contributor

@novice1993 novice1993 Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducer가 아닌 store에서 필요한 상태값들을 가져와서 RootState를 사용했습니다.

StateProps가 store에 있는 모든 상태들의 type을 종합해서 정리해놓은 interface라
해당 interface에서 모든 상태의 type을 관리하는 것도 괜찮을 것 같습니다

@@ -41,7 +41,7 @@ export const useGetCash = (cashId: string | null) => {
}

export const useResetCash = () => {
return useMutation((data: { cashId: number, cashAmount: number }) => axios.patch(`http://ec2-13-125-246-160.ap-northeast-2.compute.amazonaws.com/cash/${data.cashId}`, { cash: data.cashAmount }, {
return useMutation((data: { moneyId: number, money: number }) => axios.patch(`http://ec2-13-125-246-160.ap-northeast-2.compute.amazonaws.com/cash/${data.moneyId}`, { "money": data.money }, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 저도 기능 구현 후에 전반적으로 리팩토링 해야하는 부분인데
API url 부분도 axios 파라미터에 문자열을 직접 입력하는 게 아니라
상수로 선언하고 할당해주는 방식이 좋다고 멘토님이 코멘트 해주셨습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const BASE_URL = 'http://ec2-13-125-246-160.ap-northeast-2.compute.amazonaws.com';
네 변수에 할당해서 새로 작성했습니다

} else {
dispatch(setLogoutState());
}
}, [dispatch]);
Copy link
Contributor

@novice1993 novice1993 Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 분기에 else 가 들어간 이유가 궁금합니다
    ( 로그아웃 시 로컬 스토리지 데이터를 remove 하는 로직이 아래에 있는 걸로 알고 있어서, 분기를 나눠서 authToken이 null 값인 경우에 로그아웃 처리를 하시는 이유가 궁금합니다 )

  2. 의존성 배열에 dispatch가 들어간 이유는 뭔가요?
    ( dispatch는 action을 reducer에게 전달해주는 역할만 수행해서, 상태가 변화하는 변수는 아닌 것 같아서 의존성 배열에 추가하신 이유가 궁금합니다 )

dispatch(setLogoutState());
}
}, [dispatch]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 더이상 사용하지 않을듯해서 삭제해도 괜찮을 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 삭제하고 한번 더 push하겠습니다

headers: getAuthHeader()
}));

}

export const useGetCash = (cashId: string | null) => {
export const useGetCash = (moneyId: string | null) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주식주문 관련 작업 부분에서 저도 cash 데이터를 서버에서 받아와야 하는 부분이 있어서
useGetCash 라는 커스텀 훅을 만들었는데,

병현님이 만드신 것에도 cash 데이터를 get 해오는 훅이 있는 것 같아서
추후에 같이 논의해서 하나로 통일하고 나머지는 제거하는 게 좋을 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 확인했습니다

로그인 시 Auth토큰 유무 판별 시 else부분 삭제
API url 변수에 할당
Issues #70
Copy link
Contributor

@novice1993 novice1993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인 완료했습니다

money의 타입을 number->string으로 변동
Issues #70
@sirloinbh sirloinbh merged commit b09a7e2 into dev-client Sep 13, 2023
@sirloinbh sirloinbh deleted the dev-client#8/signuplogin branch September 21, 2023 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE Front End
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants