-
Notifications
You must be signed in to change notification settings - Fork 0
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
[BGRM-50, 58, 59] answerList API 수정 대응 및 무한 스크롤 구현 #57
Conversation
3d190c0
to
37b8df1
Compare
37b8df1
to
0c5aec2
Compare
src/pages/answer-result/index.tsx
Outdated
|
||
setIsLoading(true); | ||
try { | ||
const res = await answerAPI.list(paramRef.current); |
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 res = await answerAPI.list(paramRef.current);
- const data = res.data;
+ const {data} = await answerAPI.list(paramRef.current);
src/pages/answer-result/index.tsx
Outdated
setAnswersData((prev) => [...prev, ...newAnswers]); | ||
|
||
// 마지막 페이지인지 체크 | ||
if (newAnswers.length !== paramRef.current.limit) { |
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.
만약 총 개수가 20개인데, 처음에 10개 받고 그 다음 10개 받고 스크롤해도 다시 통신을 보낼까요??
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.
처음엔 그 다음 통신에서 다시 체크가 필요할거라 생각했는데 다시 보니 개선할 수 있는 부분이 있는 것 같습니다.
page관련 값이 넘어오고 있어서 해당 값을 활용하도록 수정해서 commit 하도록 할게요!
useEffect(() => { | ||
if (colorCodeList?.length) { | ||
const colorArr: string[] = ["#FFFFFF"]; | ||
|
||
colorCodeList.map((answer) => { | ||
if (colorArr.includes(answer.colorCode)) return; | ||
else colorArr.push(answer.colorCode); | ||
}); | ||
|
||
const colorArr = data.list?.map((answer) => answer.colorCode); | ||
if (colorArr?.length) { | ||
setSnowColorArray(colorArr); | ||
} | ||
}); | ||
}; | ||
|
||
const { userInfo } = useUserStore(); | ||
useEffect(() => { | ||
handleSnowflakeColor(); | ||
}, [userInfo]); | ||
} | ||
}, [colorCodeList]); |
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.
- 현재
map()
메서드를 사용하고 있지만, 반환값 없이 반복문으로만 사용하고 있어서forEach
가 더 적합해 보입니다.
- colorCodeList.map((answer) => {
+ colorCodeList.forEach((answer) => {
- 조건문(if문)이 많아서 가독성이 살짝 떨어질 수 있을 것 같습니다.
early return
을 활용하거나, 조건문을 간소화하면 좋을 것 같네요! 아래 코드는 예시코드로 참고만 해주세요!
useEffect(() => {
if (!colorCodeList?.length) return;
const colorArr: string[] = ["#FFFFFF"];
colorCodeList.forEach((answer) => {
if (!colorArr.includes(answer.colorCode)) {
colorArr.push(answer.colorCode);
}
});
// if (colorArr?.length) { // colorArr배열 초기값 길이가 1인데, 체크가 필요한가요 ??
setSnowColorArray(colorArr);
}, [colorCodeList]);
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.
코드 수정하면서 개선 안된 부분들이 좀 있었네요..! 개선사항 지적 감사합니다!
- 기존에 쓰던 map을 그대로 사용했는데 forEach가 더 맞는거 같네요!
- 위에서 주석으로 여쭤보신 부분은, 원래 처음에 짤 때
const colorArr: string[] = ["#FFFFFF"];
이 부분이 달랐어서 넣었던 체크 요소인데 기본 값으로"#FFFFFF"
를 넣어준 뒤 제거를 못했네요;; 체크가 없는 것이 맞습니다.
- const colorArr: string[] = [];
+ const colorArr: string[] = ["#FFFFFF"];
// ...중략
- if (colorArr?.length) {
- setSnowColorArray(colorArr);
- }
+ setSnowColorArray(colorArr);
- 패턴 같은 경우도 말씀하신 대로 좀 더 가독성을 개선 시킬 수 있을 것 같습니다.
reflect PR reviews #57
8f870ad : review에서 나온 사항들을 확인하였고, 개선하여 수정했습니다.
|
무엇을 위한 PR인가요?
변경사항 및 이유
PR 특이 사항
Issue Number