-
Notifications
You must be signed in to change notification settings - Fork 7
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
멤버 별 템플릿 페이지 구현 #815
멤버 별 템플릿 페이지 구현 #815
Conversation
…de-zap into feat/749-member-page
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.
고생하셨습니다!
코멘트 몇가지 남겼으니 확인해주시고 예비군 조심히 다녀오시길,,
frontend/src/api/members.ts
Outdated
const API_URL = process.env.REACT_APP_API_URL; | ||
|
||
export const MEMBER_API_URL = `${API_URL}${END_POINTS.MEMBERS}`; | ||
|
||
export const getMemberName = async (memberId: number) => { | ||
const url = `${MEMBER_API_URL}?memberId=${memberId}`; | ||
|
||
const response = await customFetch<GetMemberNameResponse>({ | ||
url, | ||
}); | ||
|
||
if ('name' in response) { | ||
return response; | ||
} | ||
|
||
throw new Error(response.detail); | ||
}; |
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.
요거는 ApiClient로 리팩토링하면 좋을거 같아요~!
@@ -38,7 +46,7 @@ const TemplateCard = ({ template }: Props) => { | |||
<Flex width='100%' justify='space-between' gap='1rem'> | |||
<Flex gap='0.75rem' flex='1' style={{ minWidth: '0' }}> | |||
{isPrivate && <PrivateIcon width={ICON_SIZE.X_SMALL} color={theme.color.light.secondary_800} />} | |||
<Flex align='center' gap='0.25rem' style={{ minWidth: '0' }}> | |||
<S.AuthorInfoContainer onClick={handleAuthorClick} style={{ cursor: 'pointer' }}> |
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.
여기서 cursor만 인라인 스타일로 준 이유가 혹시 있을까용?
추가로 useNavigate도 좋지만 만약 라우팅만 변경한다면 Link
를 쓰는 것도 괜찮아보이네요
<Link to={`/templates/${template.id}`} key={template.id}> | ||
<Link to={END_POINTS.template(template.id)} key={template.id}> |
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.
매직넘버가 있었네요~!! 굿굿
{!isChecking && isLogin && memberId && ( | ||
<NavOption route={END_POINTS.memberTemplates(memberId)} name='내 템플릿' /> | ||
)} |
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.
PR 메시지에 MyTemplatePage 라우팅 주소 변경
관련해서 변경된 이유를 함께 설명해주면 리뷰하기 편할 것 같아요~!
const TemplateEditPage = ({ template, toggleEditButton }: Props) => { | ||
const categoryProps = useCategory(template.category); | ||
const { | ||
memberInfo: { memberId }, | ||
} = useAuth(); | ||
|
||
const categoryProps = useCategory({ memberId: memberId!, initCategory: template.category }); | ||
const [title, handleTitleChange] = useInput(template.title); | ||
const [description, handleDescriptionChange] = useInput(template.description); |
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.
이게 memberId의 타입은 number | undefined인데, !로 단언해서 쓰는게 조금 걸리네요.
로그인한 유저의 memberId와 query로 쓰는 memberId를 분리하거나 undefined를 그냥 제거하는 방향도 생각해봐야겠네요.
혹은 해당 쿼리를 보낼 때, 로그인하지 않았다면 에러를 발생시키는 방법도 있겠네요.
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 handleAuthorClick = () => { | ||
if (template?.member?.id) { | ||
navigate(END_POINTS.memberTemplates(template.member.id)); | ||
} | ||
}; |
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.
template 부분을 SuspenseQuery로 하면 해당 옵셔널 체이닝을 제거할 수 있을까요?
MEMBERS: '/members', | ||
MEMBERS_TEMPLATES: '/members/:memberId/templates', | ||
memberTemplates: (memberId: number) => `/members/${memberId}/templates`, |
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.
혹시 :memberId
가 맨뒤로 가지 못하는 이유가 무엇이었나요?!
API endpoint와 route를 분리하긴 해야겠네요,,
export interface GetMemberNameResponse { | ||
name: string; | ||
} |
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.
Get을 빼도 될거 같긴 하지만 굳이 반영 안해도 될 것 같습니다~
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.
제 생각도 Get 빼도 될 것 같아요~
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.
전반적으로 memberId를 단언해서 사용하거나 상위에서 받은 것이 없는 경우 자신의 아이디를 넣어준 경우가 있는데, 조금 더 안전하게 사용하는 방향으로 리펙토링 해봐도 좋을 것 같아요..!
const { memberInfo } = useAuth(); | ||
const memberId = passedMemberId ?? memberInfo.memberId; |
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.
이미 상위(MyTemplatePage
)에서 memberId를 주고있는데 이렇게 처리해 준 이유가 있을까요?
안전성을 확보하기 위해 Props 의 옵셔널 체이닝을 제거해보면 어떨까요?
interface Props {
memberId: number;
}
export interface GetMemberNameResponse { | ||
name: string; | ||
} |
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.
제 생각도 Get 빼도 될 것 같아요~
MEMBERS_TEMPLATES: '/members/:memberId/templates', | ||
memberTemplates: (memberId: number) => `/members/${memberId}/templates`, |
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 { memberInfo } = useAuth(); | ||
const memberId = passedMemberId ?? memberInfo.memberId; |
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 TemplateEditPage = ({ template, toggleEditButton }: Props) => { | ||
const categoryProps = useCategory(template.category); | ||
const { | ||
memberInfo: { memberId }, | ||
} = useAuth(); | ||
|
||
const categoryProps = useCategory({ memberId: memberId!, initCategory: template.category }); | ||
const [title, handleTitleChange] = useInput(template.title); | ||
const [description, handleDescriptionChange] = useInput(template.description); |
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 { memberId: routeMemberId } = useParams<{ memberId: string }>(); | ||
const memberId = Number(routeMemberId); | ||
|
||
const { | ||
memberInfo: { name }, | ||
memberInfo: { memberId: currentMemberId }, | ||
} = useAuth(); | ||
const { | ||
data: { name }, | ||
} = useMemberNameQuery({ memberId }); |
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.
다음 부분에서 memberId 변수명이 겹칩니다. 배포가 급해 임시 방편으로 기존의 memberID를 currentMemberId로 바꾸어 최대한 변경 사항을 줄였으니 추후 리팩토링 부탁드립니다.
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.
배포 기한때문에 추후 변경 부탁드립니당
⚡️ 관련 이슈
close #749
📍주요 변경 사항
🍗 PR 첫 리뷰 마감 기한
10/20 00:00