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

Origin/feat/main/#12 #22

Merged
merged 20 commits into from
Aug 12, 2024
Merged

Origin/feat/main/#12 #22

merged 20 commits into from
Aug 12, 2024

Conversation

ftery0
Copy link
Member

@ftery0 ftery0 commented Aug 10, 2024

#12

main페이지

image

image

window라 폰트 이상할수 있습니다

수정 및 추가된 사항들

authCheck에서 로그인 했는지 안했는지 확인합니다
그후 token 여부를 상태관리 라이브러리로 저장합니다
또 그 토큰 여부를 libs\tokenCheck 에서 불러올수 있게 만들었습니다

토큰여부 불러올때 사용에시
const { getTokenCheck } = tokenCheck();
getTokenCheck() ? "로그인함": ""로그인 하러가기

Copy link
Member

@whywwhy whywwhy left a comment

Choose a reason for hiding this comment

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

수고하셨습니다


}
const submitSignupDataSecond = useCallback(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

반환되는 에러 메시지가 있으면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네 문제상황에 맞는 에러 메시지를 넣겠습니다


export const useSignup = () => {
const SignUpMutation = useSignUpMutation();
const EmailMutation = useEmailNumber();
const [section, setSection] = useState("first");

const [signUpData, setsignUpData] = useState<Sign>({
Copy link
Member

Choose a reason for hiding this comment

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

signUpData를 초기화할 때 여러 필드가 일일이 명시하고 있는데
객체의 초기 상태를 한 곳에서 정의하고 재사용할 수 있도록 상수로 분리 하는 건 어떨까요?

const 상수singup데이터: Sign = { memberId: "", memberPassword: "", memberChckPassword: "", memberName: "", memberEmail: "", memberSchool: "", authCode: "", memberFcmToken: "", };

이런느낌으로 지정한 후

Copy link
Member Author

Choose a reason for hiding this comment

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

constants로 넣어두겠습니다

return data;
}

public async refreshAccessToken(refreshToken: {
refreshToken: string;
}): Promise<NewAccessTokenResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

개인적으로는 에러 핸들링이 있으면 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

<S.emailNumberButton onClick={chckEmailAuthCode}><span>인증하기</span></S.emailNumberButton>
</S.emailTextField>

</S.InputText>
Copy link
Member

Choose a reason for hiding this comment

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

S.InputText가 중복적으로 사용되는데 컴퍼넌트로 분리 하면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

너무 많은 컴퍼넌트 분리는 안좋을거 같고 S.InputText는 자체로도 재사용성을 늘려서 상관없어 보입니다

onChange={handleSignupData}
placeholder="이메일을 입력해주세요"
placeholder="인증번호를 입력해주세요"
onKeyDown={keydownButton}
style={{}}
Copy link
Member

Choose a reason for hiding this comment

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

스타일 제거해도 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

없으면 오류나서 넣어 두었습니다

<option value="대덕소프트웨어마이스터고">
대덕소프트웨어마이스터고
</option>
</S.selectButton>
Copy link
Member

Choose a reason for hiding this comment

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

가독성이 떨어지는데

<option value="대구소프트웨어마이스터고">대구소프트웨어마이스터고</option> <option value="부산소프트웨어마이스터고">부산소프트웨어마이스터고</option> <option value="광주소프트웨어마이스터고">광주소프트웨어마이스터고</option> <option value="대덕소프트웨어마이스터고">대덕소프트웨어마이스터고</option>

이런식으로 변경하는건 어떨까요?

Copy link
Member

@whywwhy whywwhy left a comment

Choose a reason for hiding this comment

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

굿

@ftery0 ftery0 merged commit e439f22 into main Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants