Skip to content
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

[오동혁] Week13 #411

Merged

Conversation

ohdong9795
Copy link
Collaborator

@ohdong9795 ohdong9795 commented May 12, 2024

요구사항

기본

  • React 프로젝트에서 진행했던 작업물을 Next.js 프로젝트에 맞게 변경 및 이전해 주세요.
  • 팀내 2 ~ 3명이 짝을 지어 페어프로그래밍으로 로그인, 회원가입 페이지에 사용하는 Input 컴포넌트를 만들어 주세요.
  • Vercel로 Github 레포지토리를 연결해 Next.js 프로젝트를 배포해 주세요.

심화

주요 변경사항

스크린샷

멘토에게

@ohdong9795 ohdong9795 requested a review from devToram May 12, 2024 04:07
Copy link
Collaborator

@devToram devToram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다 :)

folder: {
count: number;
id: number;
links: { createdAt: string; description: string; id: number; imageSource: string; title: string; url: string }[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 따로 Link type 으로 빼도 좋을 거 같아요!


return url;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api.ts 파일은 api 와 관련된 타입을 정의하는 파일인 거 같은데 url 에 파라미터를 넣는 util 함수가 같이 들어간 거 같아요!
따로 빼는 걸 추천드립니다

return date.getFullYear() + '. ' + (date.getMonth() + 1) + '. ' + date.getDate();
}

export function calcDateDiff(date1: Date, date2: Date) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 util 의 로직대로라면 3 years ago 를 리턴하기 위해 minute ~ year 을 다 계산하게 되네요!
diff 를 특정 구간대로 나눠서 year 에 해당하는 시간대인 경우 year 만 계산하는 식으로 하면 약간은 개선될 거 같습니다
(dateFns 와 같은 라이브러리를 사용하시는 것도 추천드려요!)

}

function Item({ createdAt, url, title, description, imageSource, folderData, editable }: ItemProps) {
const [openPopover, setOpenPopover] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean 값의 변수 이름은 보통 is~, has~ 로 짓는 게 일반적이라 추천드립니다!

Suggested change
const [openPopover, setOpenPopover] = useState(false);
const [isPopoverOpen, setIsPopoverOpen] = useState(false);

Comment on lines +114 to +116
{userData ? (
!loading &&
error === null && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삼항 연산자 내부에 조건들이 많아서 가독성이 떨어질 수 있을 거 같아요!
그리고 로직상으로 보면 userData 는 있는데 로딩중이거나 에러가 있을 때의 처리가 안되어있는 거 같습니다

Comment on lines +66 to +71
entries.forEach((entry) => {
if (entry.isIntersecting) {
setIsView(true);
} else {
setIsView(false);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entries.forEach((entry) => {
if (entry.isIntersecting) {
setIsView(true);
} else {
setIsView(false);
}
entries.forEach((entry) => setIsView(entry.isIntersecting));

Comment on lines +40 to +42
{!loading &&
error === null &&
(data?.length === 0 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여러 개의 조건의 경우 따로 위에서 useMemo 를 통해 어떤 걸 의미하는 지 변수화해서 렌더부에서는 변수만 하나 적어주시면 훨씬 렌더부가 간단해져요!

<FolderNameSpan>{folderName}</FolderNameSpan>
</>
) : (
<div></div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<div></div> 가 필요한 게 아니라면 위에서 삼항연산자가 아닌 && 연산자를 써도 될 거 같아요!
(!loading && error === null) &&

fetchHeaders = sendRequestParams.headers;
fetchBody = sendRequestParams.body;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 {fetchUrl ~ fetchBody} 와 같이 Object 를 만든 뒤에 뒤에 sendRequestParam 으로 넘겨진 것들을 오버라이딩하는 하는 방식으로 쓰면 간단할 거 같아요! (혹시 이해안되시면 멘토링때 질문주셔요)

Comment on lines +60 to +86
fetchHeaders = fetchHeaders
? fetchHeaders
: typeof fetchBody === 'object' && fetchBody !== null
? { 'Content-Type': 'application/json' }
: {};

const option = {
method: fetchMethod,
headers: fetchHeaders,
body: fetchBody ? JSON.stringify(fetchBody) : null,
};

try {
const response = await fetch(fetchUrl as RequestInfo | URL, option as RequestInit);
const result = await response.json();

setData(result);
if (response.ok) callback?.(result);
} catch (e) {
const error = e as Error;
setError(error);
errorCallback?.(e as Error);
} finally {
setLoading(false);
finalCallback?.();
}
})();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

특별한 처리가 없는 거 같아서 util 로 빼면 좋을 거 같아요

@devToram devToram merged commit 1081e01 into codeit-bootcamp-frontend:part3-오동혁 May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants