-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: 로그인 페이지 api 연결 #168
feat: 로그인 페이지 api 연결 #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그인 api 연동 고생 많으셨습니다 😄 로그인 관련 hook 로직들 관련해서 코멘트 남겼으니 확인 부탁드립니다! (loginform 이쁘게 잘 나온거같아요 ㅎㅎ)
현재 에러가 발생하는 소셜 로그인 쪽이랑 localStorage 관련해서는 추후 이슈를 파서 진행하는게 좋아 보입니다. 정말 고생 많으셨어요!
@@ -2,6 +2,7 @@ import { css } from '@emotion/react'; | |||
|
|||
export const profileWrapper = (size: 'small' | 'large') => | |||
css({ | |||
all: 'unset', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unset을 선언해준 이유가 궁금합니다! 우선적으로 적용되어 버린 속성들이 있었나욥???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 컴포넌트의 상위 태그를 div에서 button으로 수정하고 나니 button 태그 내에 svg 컴포넌트와 img 태그가 들어갈 때 보여지는 스타일이 달라서 해당 코드를 넣어봤는데 해결되더라구요..! 아래에 코멘트주신 width height 참고하여 스타일 수정해보겠습니닷
@@ -37,4 +42,4 @@ const ProfileIcon = ({ | |||
: profileComponents.normal; | |||
}; | |||
|
|||
export default ProfileIcon; | |||
export default forwardRef(ProfileIcon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProfileIcon 컴포넌트를 Ref로 처리하신 이유가 궁금합니다! 해당 컴포넌트는 atom에 속하기 때문에 forwardRef를 사용하지 않는 방식으로 구현하는 것이 적합하다고 생각이 들어서 질문드려욥! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 컴포넌트가 헤더 내에서 사용될 때 클릭 이벤트가 연결되어야 하더라구요!!!
그래서 button 태그의 props를 받을 수 있도록 구현했습니다!
); | ||
}; | ||
|
||
export default forwardRef(CategoryButton); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컴포넌트 네이밍에 오타가 있는거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
바로 수정하겠습니닷... 🫢
<button type="button" ref={ref} css={S.loginButtonStyling} {...props}> | ||
<ImageComponent /> | ||
</button> | ||
{recent && <RecentLogin css={S.recentLoginStyling} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 제가 맡은 마이페이지에서도 했던 고민인데, 최근 로그인을 표시해주는 것을 단일 atoms로 처리하고, recent 상태 관리를 molecules에서 처리하는 방법도 있는 거 같아요! (단순 의견 제시입니다!)
저도 이번 UI 및 api 연동에서 atoms/molecules/organisms 단위 중 어떤 상태를 어떤 단위에서 처리하는게 더 바람직한지 고민을 많이 했어서 생각해보면 좋은 이슈 같습니다 ⭐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 해당 버튼을 atom으로 정한 이유는 우선 하나의 버튼이기도 하고, boolean 값 하나로 인해 말풍선 유무가 정해지기 때문이에요!! 그리고 말풍선의 가로 길이가 소셜 로그인 버튼의 가로 길이보다 긴데, 말풍선이 떠있는 경우 소셜 로그인 버튼의 스타일에 영향을 주면 안되기 때문에 position
속성의 absolute
와 relative
를 지정해줘야하더라구요!! 이 이유가 가장 컸던 것 같습니당
setTimeout(() => { | ||
navigate('/'); | ||
}, 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지연 시간이 기본 2초로 설정되어 있는데, 만약 사용자가 빠른 전환을 원해서 빈 화면을 클릭하면 바로 넘어 갈 수 있는 로직도 포함하는건 어떤지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2초 후 메인 페이지로 이동하는 부분은 제가 PM분께 질문하여 답변받은 플로우예요!!! 관련해서는 PM분이나 디자이너님과 함께 의논해보면 좋을 것 같습니다 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 저도 해당 부분은 확인했었는데, 해당 로직이 추가되면 좋을 거 같다! 는 의견이었습니다. 한번 코멘트 남겨놓도록 할게욥!! 🚀
@@ -37,17 +37,18 @@ export const useLoginForm = () => { | |||
onSuccess: (res: string) => { | |||
setIsError(false); | |||
setErrorMessage(undefined); | |||
|
|||
alert('로그인에 성공했습니다😀'); | |||
setLoginSuccess(true); | |||
|
|||
dispatch(tokenActions.setToken(res)); | |||
axiosInstance.defaults.headers.Authorization = `Bearer ${res}`; | |||
|
|||
// TODO: 백엔드에서 리프레쉬 토큰 쿠키에 저장시키면, 해당 코드 제거 | |||
localStorage.setItem('accessToken', res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localStroage로 우선 기능 구현 및 테스트 하신 점 확인했습니다! 추후에는 저희가 저번에 의논했던 store에 저장하여 보안 이슈를 향상 시키는 방향도 좋아보이네요 ㅎㅎ
icon={<Envelope />} | ||
placeholder="이메일" | ||
onChange={onChange} | ||
style={{ width: '450px' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스타일 관련 코드는 style.ts에 선언 후 호출하는 방식이 더 깔끔할 거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
분리하겠습니다!!
<Input | ||
name="email" | ||
value={form.email} | ||
icon={<Envelope />} | ||
placeholder="이메일" | ||
onChange={onChange} | ||
style={{ width: '450px' }} | ||
/> | ||
<Input | ||
name="password" | ||
value={form.password} | ||
icon={<Lock />} | ||
placeholder="비밀번호" | ||
isError={isError} | ||
errorMessage={errorMessage} | ||
onChange={onChange} | ||
style={{ width: '450px' }} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useLoginForm을 통해 클라이언트 측에서도 자체 유효성 검사를 처리하는 로직이네요! 훨씬 직관적이고 좋은 방식인거 같습니다 👍 👍
src/hooks/login/useLoginForm.ts
Outdated
const isValidEmailFormat = (email: string): boolean => { | ||
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
return emailRegex.test(email); | ||
}; | ||
|
||
const isValidPwFormat = (pw: string): boolean => { | ||
const pwPattern = /^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d@$!%*#?&]{10,20}$/; | ||
return pwPattern.test(pw); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀하신 거 처럼 회원가입에서도 유효성 검사 함수들이 있을 것이기 때문에 추후에 해당 부분은 utils에 유효성 관련 디렉토리에 따로 정리해서 호출하는 쪽이 더 효과적일 거 같습니다!
src/hooks/login/useLoginForm.ts
Outdated
if (!isValidEmailFormat(form.email)) { | ||
setIsError(true); | ||
setErrorMessage(ERROR.EMAIL.FORM); | ||
return; | ||
} | ||
if (isEmptyString(form.password)) { | ||
setIsError(true); | ||
setErrorMessage(ERROR.PW.EMPTY); | ||
return; | ||
} | ||
if (!isValidPwFormat(form.password)) { | ||
setIsError(true); | ||
setErrorMessage(ERROR.PW.FORM); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에서도 유효성을 검사하는 if문들을 별도의 form 유효성 검사 함수로 묶고, ex) formValidation setIsError(true); setErrorMessage(message);
부분을 별도의 함수로 처리하게 되면(반복되는 setIsError, setErrorMessage 간소화) 폼 제출 핸들러가 더 간소화 될 거 같습니다!
어디까지나 의견이니 참고해주세욥!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
참고해서 수정해보겠습니다!!! 좋은 의견 감사합니다 😶
|
||
const isValidPwFormat = (pw: string): boolean => { | ||
const pwRegex = /^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d@$!%*#?&]{10,20}$/; | ||
return pwRegex.test(pw); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 저는 이전에 정규식 확인절차를 useState함수로 받아왔는데 return값으로 받아오는게 간결하고 깔끔한 방법인 걸 알게되었네용ㅎㅎ👍
로그인 기능 구현하시느라 수고많으셨습니당!!👍 redux 상태를 사용해서 토큰 상태 관리하는 로직을 새롭게 배우게되었네용ㅎㅎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정사항 까지 모두 확인했습니다! 수정까지 고생 많으셨어요. 저도 코멘트 남기면서 많이 배우네욥! 😃
icon={<Envelope />} | ||
placeholder="이메일" | ||
style={{ width: '450px' }} | ||
onChange={onChange} | ||
css={S.loginInputStyling} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스타일 수정하신 부분 확인했습니다!!!
setTimeout(() => { | ||
navigate('/'); | ||
}, 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 저도 해당 부분은 확인했었는데, 해당 로직이 추가되면 좋을 거 같다! 는 의견이었습니다. 한번 코멘트 남겨놓도록 할게욥!! 🚀
const loginValidation = validateLoginForm(form); | ||
|
||
if (!loginValidation.isValid) { | ||
setLoginError(loginValidation.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그인 유효성 함수가 간결해진거 같네요 ㅎㅎ 가독성도 더 좋아진거같습니다.
<button type="button" ref={ref} css={profileWrapper(size)} {...props}> | ||
<NormalProfile css={profileImage} /> | ||
</div> | ||
</button> | ||
), | ||
settings: ( | ||
<div css={profileWrapper(size)}> | ||
<button type="button" ref={ref} css={profileWrapper(size)} {...props}> | ||
<img css={profileImage} src={imgUrl} alt="profile" /> | ||
</div> | ||
</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
버튼 태그 클릭 이벤트를 위해 ref 처리 하신 부분 확인했습니다! atoms가 독립적인 역할을 보장하려면 해당 컴포넌트를 상위 컴포넌트에 div 자체에 클릭 이벤트를 처리하는 방법도 있는 거 같습니다.
어디까지나 방법론적 문제라 코멘트 정도로 생각해주시면 좋을 거 같습니닷! 이전 코멘트에 대한 답변도 감사합니다 ⭐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 방식에 대해서 어느 쪽이 효과적인지는 함께 의견 나누고 싶습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
div 태그 내에 클릭 이벤트를 연결하면 div가 기본적으로 클릭 가능한 요소가 아니기 때문에 eslint 에러가 발생하여 에러 주석 처리가 필요합니다!! 최대한 eslint 에러를 줄이는 것이 좋다고 생각하여 ProfileIcon을 button 태그처럼 사용할 수 있도록 ref 연결을 사용했습니다 😊
💡 작업 내용
💡 자세한 설명
✅ SocialLoginButton
✅ useLoginForm
이메일, 비밀번호 형식 검사 함수를 util 폴더에 정의할까 했는데 이 부분은 회원가입 기능까지 끝난 후에 분리 및 적용하는 것이 나을 것 같아 로그인 훅 내에 작성했습니다!
로그인이 완료되었을 때 토스트 모달을 띄울 수 있도록
loginSuccess
값을 추가했습니다.토스트 모달이 뜨고 2초 뒤 메인 페이지로 이동하도록 하였습니다.
✅ Header
handleProfileIcon
함수 내에 쓰인navigate('/mypage');
는 임의로 넣어두었음을 알려드립니다..!+) 로그인한 사용자의 프로필 이미지가 없는 경우를 위해
default-profile.png
가 지정되도록 파일을 추가했습니다. 위에 올린 영상이 사용자의 프로필 이미지가 null 값이라 default-profile이 적용된 경우입니다!📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
🚩 후속 작업 (선택)
현재 이전의 로그인 훅을 연결만 해둔 상태이기에 새로고침했을 때 localStorage에는 accessToken이 존재하지만 로그인이 풀려보이는 현상이 발생합니다. 이 부분은 추후에 코드 수정해야 할 것 같습니다!
✅ 셀프 체크리스트
closes #167