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

경북대 FE_이효은_3주차 과제 Step3 #52

Open
wants to merge 34 commits into
base: hyoeunkh
Choose a base branch
from

Conversation

Hyoeunkh
Copy link

@Hyoeunkh Hyoeunkh commented Jul 11, 2024

안녕하세요 멘토님!
이번에 step1, 2, 3을 나눠서 PR 날리라고 하셨는데,
브랜치 나눠서 작업했지만 머지가 안 된 상태여서 step1,2의 커밋 기록이 모두 포함되어 있어요.
23648ba 부터 step2이고,
3f610e0 부터 step3입니다.
step3 PR에만 피드백 달아주시면 감사하겠습니다!

+++
step1에서 받았던 코드리뷰 부분을 step3에도 반영해두겠습니다!


<질문>
/components/Theme/ItemList.tsx
페이지네이션 구현할 때 ref 를 Grid에 두니 적용이 안되는데, 원래 상위 컴포넌트에 두는것이 아닌가요?
현재 <div ref={ref}></div> 를 추가해주었는데 이렇게 해도 되는 건지 궁금합니당!

감사합니다

Hyoeunkh added 30 commits July 8, 2024 22:39
Copy link

@sjoleee sjoleee left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~ 리뷰 남깁니다.

질문을 남겨주셨었군요!
일단 '적용이 안되는데'의 의미를 정확히 알아야 할 것 같아요.

intersection observer는 타겟이 뷰포트에 들어왔는지를 알려주는 api라서 사용하신 라이브러리도 Grid가 뷰포트에 들어오면 지정된 동작을 수행하도록 만들어져 있을거에요.
일반적으로 타겟 전체가 뷰포트에 들어와야 실행되도록 해두었을 것 같은데요, Grid가 뷰포트보다 크면 원하는 동작이 발생하지 않을 수 있겠네요.

라이브러리를 활용해주셨는데, 가능하면 api를 직접 사용해보시는걸 추천드려요~!

Comment on lines +29 to +37
const HandleFilteredList = () => {
if (isLoading) {
return <Loading />;
}
if (error) {
return <HandleBox>{error.message}</HandleBox>;
}
return <FilteredList filterdList={filterdList} />;
};
Copy link

Choose a reason for hiding this comment

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

컴포넌트 안에 컴포넌트를 만드셨군요?
이러면 어떤 단점이 있을까요?

Comment on lines +23 to +25
if (isLoading) {
return <Loading />;
}
Copy link

Choose a reason for hiding this comment

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

로딩중이면 아예 리스트를 감춰버리나요??
좋은 ux가 맞을까요?

return <HandleBox>{error.message}</HandleBox>;
}
const products = data?.pages.flatMap((page) => page.products) ?? [];
if (products.length == 0) {
Copy link

Choose a reason for hiding this comment

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

가능하면 항상 일치연산자를 사용해주세요~

Comment on lines +17 to +28
if (error instanceof AxiosError) {
if (error.response?.status === 404) {
console.error('해당하는 랭킹 상품이 없습니다:', error);
throw new Error('해당하는 랭킹 상품이 없습니다.');
} else {
console.error('데이터를 불러오는 중에 문제가 발생했습니다:', error);
throw new Error('데이터를 불러오는 중에 문제가 발생했습니다.');
}
} else {
console.error('Unexpected error occurred:', error);
throw new Error('Unexpected error occurred');
}
Copy link

Choose a reason for hiding this comment

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

이런 형태로 사용하게되면 react query가 핸들링하는 에러는 메시지만 담고있겠군요
다른 정보가 필요하게 되면 어떡하죠?

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.

2 participants