-
Notifications
You must be signed in to change notification settings - Fork 44
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
[Week15] 우지석 #485
The head ref may contain hidden characters: "part3-\uC6B0\uC9C0\uC11D-week15"
[Week15] 우지석 #485
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.
지석님 늦게나마 제출하시느라고 고생하셨고, 밀린 요구사항들도 같이 작업하시느라고 수고많으셨습니다!
커리큘럼대로 새로운 개념들 계속 익히면서 정해진 미션도 제시간에 하려면 아무래도 벅차실텐데요
그런만큼 시간을 더 많이 쓰면서 부족한 부분 채우셨으면 좋겠습니다!
지금 잘 이해되지 않더라도 묵묵히 해내다보면 어느새 이해되는 지점이 올테니까요,
계속해서 화이팅입니다!
} | ||
|
||
function Button({ folderData, selectedFolderId, onFolderClick }: ButtonProps) { | ||
const [activeButton, setActiveButton] = useState<number | null>(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.
변수명이 activeButton 이어서 어떤 값이 할당되는건지 한번에 잘 안와닿았던 것 같아요!!
button 이다보니 요소가 할당되는건가 싶기도 했고요
아래보니 buttonId 가 저장되는 것 같은데 그렇다면 상태명도 activeButtonId 라고 보다 구체적으로 작성해주시면 좋을 것 같습니다!!
변수네이밍은 기본적이지만 꾸준히 신경써야 하는 부분인 것 같아요!
let url = `${BASE_URL}/links`; | ||
if (selectedFolderId !== null) { | ||
url += `?folderId=${selectedFolderId}`; | ||
console.log("Id :", selectedFolderId); |
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 올리기 전 꼭 남겨야하는 콘솔이 아니라면 삭제 부탁드려요! (전체적으로 확인)
const FooterContent = styled.div` | ||
display: flex; | ||
justify-content: space-between; | ||
white-space: nowrap; | ||
`; | ||
const FooterYear = styled.div` | ||
color: rgba(103, 103, 103, 1); | ||
`; | ||
|
||
const StyledLink = styled(Link)` |
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.
어떤 부분은 선언부 사이에 개행이 들어가있고 (19라인) 어떤 부분은 없는데 (15~16라인)
개행을 처리하는 방식도 컨벤션을 정해 프로젝트 전체적으로 통일해주시면 좋겠습니다!
@@ -61,7 +61,7 @@ export function PasswordInput({ | |||
/> | |||
<div | |||
id="password-errorText" | |||
className={cx("errortext", { error: !isError })} | |||
className={cx("error-text", { error: !isError })} |
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.
isError 가 아닌 상황인데 error
클래스가 적용되는게 의미상 맞지 않는 것 같아요! (반대가 되어야 할 것 같은)
useEffect(() => { | ||
const fetchLinkData = async () => { | ||
try { | ||
const accessToken = localStorage.getItem("accessToken"); | ||
if (accessToken) { | ||
const response = await axios.get( | ||
`${BASE_URL}/users/${userId}/links?folderId=${folderId}`, | ||
{ | ||
headers: { | ||
Authorization: `Bearer ${accessToken}`, | ||
}, | ||
} | ||
); | ||
setLinkData(response.data.data); | ||
} | ||
} catch (error) { | ||
console.error("링크 데이터 가져오기 에러:", error); | ||
} | ||
}; | ||
|
||
fetchLinkData(); | ||
}, [folderId, userId]); |
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.
요런 부분이 여러번 반복되고 있는데요, 시간이 된다면 공통화할 방법을 고민해보시면 좋을 것 같아요!
fetchFolderData(); | ||
if (folderData) { | ||
fetchFolderOwner(); | ||
} | ||
}, [folderId, folderData]); |
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.
- 처음에는 folderData 가 null 이니까
fetchFolderData
만 실행 fetchFolderData
가 성공적으로 실행되면setFolderData
를 통해 상태 업데이트 (folderData
에 데이터가 들어감)- 리렌더링되면서 useEffect 실행됨 (
folderData
가 dependencies 에 포함되어서folderData
가 달라진 것 감지) - 다시 91 라인에서
fetchFolderData
실행되고 folderData
있으므로fetchFolderOwner
도 실행 (92~93라인)
제가 판단하기엔 위와 같이 코드가 실행되어서 4번처럼 불필요하게 fetchFolderData
가 두번 실행되지 않을까 싶은데요
useEffect 를 두개로 나누어서
folderId 만을 dependency 로 하는 useEffect 에 fetchFolderData
를 넣고
folderData 를 dependency로 하는 useEffect 에 fetchFolderOwner
를 넣는 식으로 분리해보면 어떨까 싶네요!
|
||
return ( | ||
<StyledFolderInfoContent> | ||
{folderData && folderOwner && ( |
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 validateEmail = isEmailValid(email); | ||
const validatePassword = isPasswordValid(password); |
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.
함수명이랑 boolean 타입 변수명의 이름이 반대로 된 것 같아요 😂
보통 함수명 작성 시 동사로 시작하게 짓고 (validateXX)
boolean 형은 isXX 과 같은 형태로 짓습니다
useEffect(() => { | ||
const accessToken = localStorage.getItem("accessToken"); | ||
if (!accessToken) { | ||
router.push("/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.
로그인하지 않고 접근할 수 없는 페이지들에서 이 로직을 공통으로 사용할 수 있겠네요!
@@ -0,0 +1,25 @@ | |||
import { useCallback, useEffect, useState } from "react"; | |||
|
|||
export function useFetch<T>(url: string): T | 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.
잘 정의는 해주셨는데 아직 쓰는 곳이 없는거였군요?!
제네릭까지 넣어주셨는데 다음 미션때 여유되면 한군데라도 적용해보면 좋겠네요!
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게
링크 : http://5-weekly-mission-rust.vercel.app/