-
Notifications
You must be signed in to change notification settings - Fork 0
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
학생 인증 기능 추가, 유저 정보 등록 리팩터링 #111
Conversation
- `undefined`도 세팅할 수 있도록 변경
- undefined가 직렬화 불가능해서 undefined로 세팅이 안되는듯 (아예 무시됨)
- 유저 정보에 따라 텍스트 다르게 표시 - ssafyMember가 아닌 경우 - 이미 인증된 유저인 경우 - 인증하지 않은 유저인 경우
parameters.msw.handler -> parameters.msw.handlers
b802334
to
22a715b
Compare
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.
UserRegisterForm
쪽은 리팩토링 진행했고,
NameCard
부터 학생인증에 필요한 컴포넌트, 스토리, 페이지 작업했습니다.
학생인증은 이 PR로 기능은 다 구현됐어요.
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.
UserRegisterForm
쪽은 전부 StudentCertificationForm
이랑 동일한 흐름으로 바꿔놨습니다.
- 외부에서
onSubmit
전달받음 (기존에는 내부에서 핸들링) - 불필요한 컨텍스트 제거
- 텍스트 큰거 렌더링 하는 용도로 만들어둔
Question
컴포넌트 제거 - Form 타입과 API 타입 분리해서 작성 (기존에는 두 타입이 일치)
@@ -18,7 +18,7 @@ import { defaultify } from '~/utils'; | |||
// import 구문 옆에다 주석 달면 린트 오류가 발생해서 여기 적어둡니다. | |||
|
|||
export interface TrackProps<Track extends keyof typeof tracks> { | |||
name?: Track; | |||
name?: Track | null; |
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.
트랙이 없는경우 서버에서 값이 null
로 넘어오기 때문에 추가
@@ -123,8 +121,7 @@ export const ModalInModal = () => { | |||
<Button onClick={handleOpenModal}>트리거 버튼</Button> | |||
</div> | |||
|
|||
{/* 이 모달은 `_app.tsx`에서 한번만 렌더링합니다. */} | |||
<GlobalModal /> | |||
{/* `GlobalModal`은 `_app.tsx`에서 한번만 렌더링합니다. */} |
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.
스토리북의 Preview.tsx
에 반영했습니다.
parameters: { | ||
msw: { | ||
handlers: { | ||
// 기본적으로 정의한 유저 정보 핸들러가 데이터를 `userInfo.initialUser`로 채우기 때문에 | ||
// Effect 에서 세팅하는 `userInfo.uncertifiedSsafyUserInfo`와 충돌합니다. (500ms 딜레이를 가지고 API를 응답하기 때문에) | ||
// 해당 핸들러를 제거할 목적으로 override 합니다. | ||
member: [certifyStudent], | ||
}, | ||
}, | ||
}, | ||
}; |
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.
페이지 스토리를 추가하실 때는 이런식으로 핸들러 오버라이드 하시면 됩니다.
우려되는 점은, 이쪽은 타입이 아예 정의되지 않아서 오탈자를 잡아주지 않는다는 건데 (작업하면서 handlers
를 handler
로 오타내서 의도한대로 동작하지 않은 경우도 있었어요)
StoryObj
타입을 확장시킬까 생각중입니다.
일단은 사용하게 된다면 스펠링을 주의해서 적어주시면 될것 같아요.
참고로 member
키에 들어가는 핸들러의 기본값은 여기서 볼 수 있는데, 지금 여기있는 49번 라인처럼 작성하면 머지되는게 아니라 아예 교체하게 됩니다.
전 이 페이지에서 이 API만 필요하기 때문에 상관없지만, 만약에 어느 하나를 제거할 생각이라면
member: memberHandlers.filter(h => h === certifyStudent)
이런식으로 사용하시면 될 것 같아요.
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.
참고할게요! 곧 하나 올라갈듯..!!
parameters: { | ||
msw: { | ||
handlers: { | ||
member: [], | ||
}, | ||
}, | ||
}, |
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.
여기도 마찬가지로,
유저 정보가 없어야 하는데, 기본적으로 msw에서 유저정보를 500ms 딜레이를 가지고 응답하기 때문에 오버라이드 했습니다.
useEffect(() => { | ||
const myInfo = !ssafyMember | ||
? userInfo.nonSsafyUserInfo | ||
: certified | ||
? userInfo.certifiedSsafyUserInfo | ||
: userInfo.uncertifiedSsafyUserInfo; |
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.
msw
에서 사용하던 data
값들을 그대로 가져와서 쓰는 것이니,
필요하신 경우 저처럼 mock 데이터 가져와서 쓰세요!
if (certificationSuccess) { | ||
return ( | ||
<DelayedRedirection to={routes.main()} seconds={3}> | ||
<PreviewCertifiedMyInfo userInfo={myInfo as NonNullable<UserInfo>} /> | ||
</DelayedRedirection> | ||
); | ||
} |
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.
원래는 이 페이지의 JSX 반환문에서
success ? <인증성공화면 /> : <인증Form />
이런식으로 보여주려고 했는데,
바로 아래에 이미 인증된 상태라면 리다이렉션 시키는 조건문 로직 떄문에,
인증 성공 이후에 트랙 정보나 인증상태 이런거 세팅하는 순간 거기 걸려서 아래로 내려오질 못하게 됩니다.
그래서 더 위에서 JSX를 반환하는 방식으로 했어요.
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.
기존에 있었던 컨텍스트를 삭제하고 여기로 옮겼습니다.
replace?: boolean; | ||
} | ||
|
||
const DelayedRedirection = (props: DelayedRedirectionProps) => { | ||
const { children, to, seconds, replace = false } = props; | ||
const { children, to, seconds = 3, replace = false } = props; |
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.
좋습니다!
개인적으로 개발자간에 합의만 된다면 default value가 있는 게 좋다고 생각했어요
}; | ||
|
||
const formatSsafyInfo = (year: number, campus: string) => { | ||
return `${campus}캠퍼스 SSAFY ${year}기`; |
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.
별건 아닌 부분이지만 아마 year 대신 semester를 사용해서 용어를 맞춰놔도 좋을 것 같아요!
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.
저번에 말씀드렸듯이 의미상으로 semester
가 아닌 year
가 맞아서 이렇게 놔뒀습니다!
@@ -51,8 +54,8 @@ export default RedirectionGuide; | |||
|
|||
const selfCss = css( | |||
{ | |||
minHeight: '100vh', | |||
height: 'max-content', | |||
minHeight: pageMinHeight, |
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.
minHeight
를 전체적으로 통일하기 위해 사용한 부분일까요?
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.
네. vh로만 높이를 주면 높이가 낮은 디바이스에서 어색하게 보여서요.
Issues
구분
주요 변경점
<Name />
,<Avatar />
리팩터링 #107 Merge 이후에 업데이트 할게요.제꺼 리뷰-머지되면npx storybook@latest upgrade
사용해서 스토리북 업그레이드 한번 해주세요.스크린샷
기타