-
Notifications
You must be signed in to change notification settings - Fork 51
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
[이정윤] week19 #553
The head ref may contain hidden characters: "part3-\uC774\uC815\uC724-week19"
[이정윤] week19 #553
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.
정윤님~ 리액트 쿼리 공부 열심히하셨군요! ㅎㅎ
항상 가독성 고려해서 정리하면서 꼼꼼하게 코드 짜시는게 정윤님의 큰 장점같아요.
이번에도 너무 잘하셨습니다 👍
크게 고쳐야할건 안보여서 모달 관련해서 드릴 수 있는 리팩토링 주제랑,
마이너하지만 코드를 더 깔끔하게 정리할수있는 포인트 몇개 짚어드려봤어요.
파트 4도 화이팅!! Approve 드리겠습니다 👍
} | ||
|
||
function AddFolderButton({ children }: Props) { | ||
function AddFolderButton() { | ||
const { isOpenModal, openModal, closeModal } = useModal(false); |
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.
모달 관련해서 팁! ㅎㅎ
모달은 클라이언트에서 전역적으로 관리해야하는 데이터에 적합하다고 생각해요.
모달의 동작에 관련된 로직을 한곳에 모아줘야하는데 여러 컴포넌트에서 동시에 모달을 다룰때의 경우도 생각해야하고,
현재 열린 모달에 미리 fetch해서 보내줄 데이터가 있는경우도 있고, 모달의 타입마다 실행되어야하는 인터렉션도 다 다른데
그걸 하나의 컴포넌트 & custom hook으로 관리하다보면 특정 파일의 크기가 너무 비대해질 가능성도 있고,
이런 구조로는 버그가 숨어들 가능성 또한 커진다고 생각하기때문이예요.
지금 구조도 충분히 좋지만 모달의 상태를 하나의 데이터라고 생각하고 전역적인 state로 관리하게끔 바꿔주시면 더 좋을것같다는 생각이 드네요! 한번쯤 생각해볼만한 리팩토링 주제로 추천드리겠습니다 :)
components/AddLink/AddLinkButton.tsx
Outdated
|
||
interface Props { | ||
inputValue: string; | ||
children?: ReactNode; | ||
folderListData: UserFolders[] | 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.
이건 어떨까요?
folderListData: UserFolders[] | undefined; | |
folderListData?: UserFolders[]; |
} | ||
|
||
function AddLinkButton({ inputValue, children }: Props) { | ||
function AddLinkButton({ inputValue, folderListData }: 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.
혹은, 꼭 필수로 있어야하는 prop인데 undefined인 상태를 고려하고싶다면
optional prop으로 만들지않고 빈 배열로 초기화 하는 경우도 있어요.
function AddLinkButton({ inputValue, folderListData }: Props) { | |
function AddLinkButton({ inputValue, folderListData = [] }: Props) { |
<ModalContainer onClose={handleCloseModal}> | ||
<AddLinkModalContent | ||
inputValue={inputValue} | ||
folderListData={folderListData} |
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.
이런 경우도 모달을 띄우는데 미리 fetch해둔 데이터가 필요한 경우죠?
folderListData는 선언해주시면 type처럼 undefined 일수도있는데, 값이 있는 상태에서만 모달을 띄우는게 더 맞지않을까요?
{isOpenModal && folderListData?.length > 0 && (
이런식으로 조건식에 추가해주시거나, useModal에 parameter 하나를 더 추가해서 해당 parameter값에 따라
isOpenModal이 true가 되는 조건(값이 있을때에만 true)을 하나 더 추가해주시면 좋을것같아요 :)
name="addLink" | ||
placeholder="링크를 추가해 보세요" | ||
value={inputValue} | ||
onChange={handleChange} | ||
/> | ||
{error && ( |
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.
이런 꼼꼼한 에러 핸들링 너무 좋네요 🥰
@@ -63,14 +64,20 @@ function CardMenu({ folderListData, url }: Props) { | |||
)} | |||
{isOpenDeleteLinkModal && ( |
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.
이 경우도 현재 사용하는 모달 타입을 바꾸려고 custom hook을 여러번 왔다갔다하는것보다, 앞서 말씀드린것처럼 전역적으로 한번에 하나씩 관리되게하면 훨씬 유지보수에 용이해지겠죠? 코드의 구조도 단순해질수있고요 :)
onClose: () => void; | ||
} | ||
|
||
function AddFolderModalContent({ onClose }: 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.
정윤님 리액트쿼리 어때요? ㅎㅎ 잘쓰시네요 👍👍
</div> | ||
{selectedFolder === folder && <Image src={checkImg} alt="check" />} | ||
</div> | ||
))} | ||
</div> | ||
<ModalButton color="blue">추가하기</ModalButton> | ||
<ModalButton color="blue" onClick={handleAddLink}> |
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.
disabled 상태도 적절하게 추가해주시면 좋을것같아요 :)
전체적으로 selectedFolder 데이터가 있어야만하는 조건문이 반복적으로 보여서요 ㅎㅎ
@@ -38,6 +38,7 @@ function AuthForm({ | |||
const linkText = type === "signIn" ? "회원가입 하기" : "로그인 하기"; | |||
const socialDescription = | |||
type === "signIn" ? "소셜 로그인" : "다른 방식으로 가입하기"; | |||
const button = type === "signIn" ? "로그인" : "회원가입"; |
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.
buttonText정도가 적절한 네이밍일것같네요 :)
</ObserverProvider> | ||
</UserProvider> | ||
</AuthProvider> | ||
<QueryClientProvider client={queryClient}> |
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.
Provider종류가 꽤 많은데, AppProvider라는 컴포넌트를 따로 만들고 필요한 nested 컴포넌트들을 children으로 받아주는건 어때요? ㅎㅎ
수고하셨어요! :) |
요구사항
기본
[프로젝트 전반] https://bootcamp-api.codeit.kr/docs/linkbrary/v1 문서를 참고해 https://bootcamp-api.codeit.kr/api/linkbrary/v1 api를 활용했나요?
[프로젝트 전반] api 요청에 TanStack React Query를 활용했나요?
[로그인, 회원가입 페이지] 로그인은 POST ‘/auth/sign-in’ 을 활용했나요?
[로그인, 회원가입 페이지] 회원가입은 POST ‘/auth/sign-up’ 을 활용했나요?
[로그인, 회원가입 페이지] 이메일 중복확인은 POST ‘/users/check-email’ 을 활용했나요?
[링크 공유 페이지] 폴더의 정보는 GET ‘/folders/{folderId}’, 폴더 소유자의 정보는 GET ‘/users/{userId}’를 활용했나요?
[링크 공유 페이지] 폴더의 링크 데이터는 GET ‘/folders/{folderId}/links’ 를 활용했나요?
[링크 공유 페이지] 유효한 access token이 있는 경우 GET ‘/users’ 로 현재 로그인한 유저 정보를 받아 상단 네비게이션 유저 프로필을 보여 주나요?
[링크 공유 페이지] 유효한 access token이 없는 경우 “로그인” 버튼을 보여 주나요?
[폴더 페이지] 폴더 페이지에서 현재 유저의 폴더 목록 데이터는 GET ‘/folders’ 를 활용했나요?
[폴더 페이지] 폴더 페이지에서 전체 링크 데이터를 받아올 때 GET ‘/links’, 특정 폴더의 링크를 받아올 때 GET ‘/folders/{folderId}/links’를 활용했나요?
[모달] 폴더 이름 변경은 ‘PUT /folders/{folderId}’를 활용했나요?
[모달] 폴더 생성은 ‘POST /folders’를 활용했나요?
[모달] 폴더 삭제는 ‘DELETE /folders/{folderId}’를 활용했나요?
[모달] 링크 삭제는 ‘DELETE /links/{linkId}’를 활용했나요?
[모달] 링크 생성은 ‘POST /links’를 활용했나요?
주요 변경사항
Vercel
보러가기
멘토에게