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

[#10] 상품목록 UI 작업 #24

Merged
merged 27 commits into from
Aug 16, 2021
Merged

[#10] 상품목록 UI 작업 #24

merged 27 commits into from
Aug 16, 2021

Conversation

momongss
Copy link
Collaborator

@momongss momongss commented Aug 12, 2021

관련이슈

실제 소요시간

8h

체크리스트

  • base branch를 확인했나요?

설명

  • 대략적인 레이아웃을 잡았습니다.
    • ul reset, box-sizing 등이 태그마다 따로 설정되어있습니다. 나중에 global 에서 처리하게되면 뺄 예정입니다.
  • OO순 눌렀을 때 제대로 동작
    • 추천순 제외하고 동작합니다. (인기순, 최신순, 낮은가격순, 높은가격순)
  • 페이지별로 눌렀을 때 제대로 동작

의논할 것.

상품 테이블에 등록일 컬럼을 추가해야할 것 같습니다.

@momongss momongss added feature 기능 frontend 프론트엔드 labels Aug 12, 2021
@momongss momongss added this to the 1주차 스프린트 milestone Aug 12, 2021
@momongss momongss self-assigned this Aug 12, 2021
@momongss momongss changed the title Feat/frontend product list [#10] 상품목록 UI 작업 Aug 12, 2021
Copy link
Collaborator

@sanginchun sanginchun left a comment

Choose a reason for hiding this comment

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

로직을 뷰와 분리해서 컨테이너에 작성하는 부분은 적용이 안 되어 있어서 조금 논의가 필요할 것 같아요. 그 때 뭔가 확실히 정하지 못해서 어려움이 있으셨을 듯 합니다. 고생하셨습니다 :)

client/src/components/ProductList/ProductList.tsx Outdated Show resolved Hide resolved
Comment on lines 56 to 65
useEffect(() => {
function fetchProductList() {
const resData = apiMock.getProductList(listOrder.current, currentPage.current);
totalProductCount.current = resData.totalProductCount;
totalPage.current = resData.totalPage;
setProductList(resData.productList);
}
fetchProductList();
}, []);

Copy link
Collaborator

Choose a reason for hiding this comment

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

상품리스트 API가 나와야 구체적으로 작성할 수 있을 것 같긴 한데 커스텀 훅을 사용하면 좋을 것 같네요. 😃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

훅을 잘 몰라서.. 논의하면서 진행하면 좋을 것 같습니다.

client/src/components/ProductList/ProductMock.ts Outdated Show resolved Hide resolved
client/src/components/ProductList/ProductMock.ts Outdated Show resolved Hide resolved
client/src/components/ProductList/ProductMock.ts Outdated Show resolved Hide resolved
Copy link
Member

@Choi-Jinwoo Choi-Jinwoo left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 :)

상품테이블에 createdAt 컬럼은 있는데 따로 필요하신건가요?

client/index.d.ts Show resolved Hide resolved
client/src/components/ProductList/ProductItem.tsx Outdated Show resolved Hide resolved
client/src/components/ProductList/ProductItem.tsx Outdated Show resolved Hide resolved
client/src/components/ProductList/ProductItem.tsx Outdated Show resolved Hide resolved
Comment on lines 57 to 62
function fetchProductList() {
const resData = apiMock.getProductList(listOrder.current, currentPage.current);
totalProductCount.current = resData.totalProductCount;
totalPage.current = resData.totalPage;
setProductList(resData.productList);
}
Copy link
Member

Choose a reason for hiding this comment

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

함수 표현식으로 사용하기로 했었으니 수정하면 좋겠습니당 ㅎㅎ

그리고 저는 useEffect에서 사용되는 함수들은 useCallback으로 감싸서 useEffect 밖에 적어두는 편인데 다른 분들은 어떠신가요?

@sanginchun @juyoungpark718 @momongss

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 useCallback을 잘 사용하지 않았어서, 하지만 사용하는거 좋다고 생각합니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

함수표현식으로 변경하겠습니다!. useCallback 으로 감싸는 얘기는 어떤 얘기인지 잘 모르겠네요. ㅠㅠ 이따 논의해보면 좋겠습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

useCallback 으로 감싸는 걸로 수정했습니다! 혹시 괜찮은 방식인지 봐주시면 감사하겠습니다.

Comment on lines 66 to 76
const onClickSortButton = (order: Order): void => {
listOrder.current = order;
const resData = apiMock.getProductList(listOrder.current, currentPage.current);
setProductList(resData.productList);
};

const onClickPageButton = (pageNum: number): void => {
currentPage.current = pageNum;
const resData = apiMock.getProductList(listOrder.current, currentPage.current);
setProductList(resData.productList);
};
Copy link
Member

Choose a reason for hiding this comment

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

이 부분이 중복되는걸 봐선 useEffet의 뎁스로 page, order와 같은 부분을 넣고 둘중 하나라도 상태가 바뀌면 이 부분들을 호출하도록 하는건 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

중복되고 있으니 한 곳에 몰아넣는 건 좋은 것 같은데 useEffect 의 뎁스로 넣는다는 얘기는 어떤 얘긴지 잘 모르겠네요 ㅠㅠ. 이것도 이따 논의해보면 좋겠습니다.

Comment on lines 84 to 86
onClick={() => {
onClickSortButton(Order.popularity);
}}
Copy link
Member

Choose a reason for hiding this comment

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

JSX 문법 내부에서 화살표 함수를 사용하면 컴포넌트가 랜더링을 할때마다 새로운 함수를 만들어 최적화가 깨질수 있다고 합니다 (대체론 괜찮다고 하네요)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

onClickSortButton(Order.popularity);
onClickSortButton(Order.recent);
onClickSortButton(Order.priceLow);
onClickSortButton(Order.priceHigh);

이렇게 onClickSortButton 이 인자만 다르게 여러번 사용되어서 사용되어서 화살표 함수를 사용해 인자를 다르게 넣어줬습니다.
저는 다른 인자마다 함수를 따로 만들어주는 방법외에 생각이 나지 않아서 일단 이렇게 해줬습니다..
더 좋은 방안이 생각나면 개선하겠습니다. 😂

Copy link

Choose a reason for hiding this comment

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

오 지나가다 새로운 지식 하나 배워갑니다 👀

Copy link

Choose a reason for hiding this comment

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

onClickSortButton() 이 함수를 뱉는 방식은 어떨까요?

function onClickSortButton(order) {
  return (event) => {
    ...
  }
}

이런 식...?
반환되는 내부 함수가 onClickSortButton의 역할을 하니 네이밍의 차별성은 필요할 것 같기두 하네요 ㅎㅎ

client/src/components/ProductList/ProductList.tsx Outdated Show resolved Hide resolved
client/src/components/ProductList/ProductList.tsx Outdated Show resolved Hide resolved
Comment on lines +7 to +9
<div>
<ProductListWrapper></ProductListWrapper>
</div>,
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 기존처럼 해서 병합되고 나중에 App.tsx로 대체해야될것 같습니다 :)

Comment on lines 7 to 22
const ProductItem = styled.li`
box-sizing: border-box;
width: 300px;

padding: 0 10px;
margin: 20px 0;

list-style: none;
`;

const Img = styled.img`
width: 100%;
`;

const Name = styled.div``;
const Price = styled.div``;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ProductItem 안에 styled 컴포넌트들이 컴포넌트 안에 작성 vs 바깥에 작성 이 부분에 대해서도 논의해보면 좋을 것 같네요 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 고민하던 부분입니다!. 논의해보면 좋겠습니다.!

client/src/components/ProductList/ProductList.tsx Outdated Show resolved Hide resolved
Comment on lines 57 to 62
function fetchProductList() {
const resData = apiMock.getProductList(listOrder.current, currentPage.current);
totalProductCount.current = resData.totalProductCount;
totalPage.current = resData.totalPage;
setProductList(resData.productList);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 useCallback을 잘 사용하지 않았어서, 하지만 사용하는거 좋다고 생각합니다 !

Comment on lines 10 to 15
export enum Order {
popularity,
recent,
priceLow,
priceHigh,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

파스칼 케이스가 뭔가 나은거 같습니다 .. 저는 파스칼 한표

Copy link

@hwookim hwookim left a comment

Choose a reason for hiding this comment

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

프론트까지 코드가 깔끔..!
배울 점이 참 많은 것 같습니다. 😊

저희 팀은 아직 프론트를 시작하지 않아서,
시작할 때 많이 훔쳐가도록 하겠습니다 ㅎㅎㅎㅎㅎㅎ

client/src/components/ProductList/ProductItem.tsx Outdated Show resolved Hide resolved
Comment on lines 83 to 110
<SortButton
onClick={() => {
onClickSortButton(Order.popularity);
}}
>
인기순
</SortButton>
<SortButton
onClick={() => {
onClickSortButton(Order.recent);
}}
>
최신순
</SortButton>
<SortButton
onClick={() => {
onClickSortButton(Order.priceLow);
}}
>
낮은가격순
</SortButton>
<SortButton
onClick={() => {
onClickSortButton(Order.priceHigh);
}}
>
높은가격순
</SortButton>
Copy link

Choose a reason for hiding this comment

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

텍스트와 함수에 넘기는 인자를 매칭시킨 object를 만들어서 map을 활용하면 중복을 좀 줄일 수도 있을 것 같은데 어떠신가요? 😄

Copy link
Collaborator Author

@momongss momongss Aug 13, 2021

Choose a reason for hiding this comment

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

오.. 그런 식으로 할 수 있겠네요. 정말 좋은 생각인 것 같습니다.

<SortButtonList>
  <SortButton onClick={onClickSortButton(Order.popularity)}>인기순</SortButton>
  <SortButton onClick={onClickSortButton(Order.recent)}>최신순</SortButton>
  <SortButton onClick={onClickSortButton(Order.priceLow)}>낮은가격순</SortButton>
  <SortButton onClick={onClickSortButton(Order.priceHigh)}>높은가격순</SortButton>
</SortButtonList>

저는 위 경우에는 sort 버튼들이 추가되거나 삭제되는 일 없이 보통 정적으로 유지되고, 숫자도 많지 않아서 일단 위와 같이 명시하는게 좋다고 생각해서 이번엔 그대로 유지하겠지만 다음번에 중복을 줄일 때 유용하게 활용할 수 있을 것 같습니다. 감사합니다. 👍

);
};

function creatPageNumbers(totalPage: number): number[] {
Copy link

Choose a reason for hiding this comment

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

이 친구만 함수가 별도로 분리되어있는 기준이 있나욤??👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

컴포넌트의 state 에 종속된 부분이 없어서 분리했습니다!

Comment on lines 8 to 14
box-sizing: border-box;
width: 300px;

padding: 0 10px;
margin: 20px 0;

list-style: none;
Copy link
Member

Choose a reason for hiding this comment

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

오 스타일의 역할(?) 마다 분리를 하신건가요?

Copy link
Collaborator Author

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 24
case Order.priceLow:
return productList.sort((a: ProductItemType, b: ProductItemType) => a.price - b.price);
case Order.priceHigh:
return productList.sort((a: ProductItemType, b: ProductItemType) => b.price - a.price);
case Order.recent:
return productList.sort(
(a: ProductItemType, b: ProductItemType) =>
new Date(b.uploadDate).getTime() - new Date(a.uploadDate).getTime()
Copy link
Member

Choose a reason for hiding this comment

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

가격, 최신순의 정렬은 백엔드에서 해당 방법대로 정렬된 데이터를 보내줘야 할 것 같아요!

Comment on lines 78 to 80
width: 1200px;
display: flex;
flex-wrap: wrap;
Copy link
Member

Choose a reason for hiding this comment

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

위에서는 margin, width 등을 \n 으로 분리해서 넣어줬었는데 여기는 아니네요..
통일 하면 좋을 듯 합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 좋은 지적이네요. 반영하겠습니다. 👍


const Main = styled.div`
width: 1200px;

Copy link
Member

Choose a reason for hiding this comment

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

style 작성하실 때 한줄 띄우는건 어떤 의도이신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 연관이 있어보이는 것들끼리 모아두려는 의도입니다.
그런데 엄격한 기준이 있는게 아니라 습관적으로 하는 거라 통일성이 없어보일 때도 있을 겁니다. 😂😂😂

Copy link
Collaborator

@juyoungpark718 juyoungpark718 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 :) 다만, lint에서 warning이 뜨는 부분이 있는거 같은데, 확인해서 지울 수 있으면 지우면 좋겠습니다 ! mock/api.ts missing return type, fetchProductList missing dependency 두 개가 있습니다 ! 😄

Comment on lines 1 to 3
export const getKoreanMoneyFormat = (amount: number): string => {
return `${amount.toLocaleString()}원`;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

단순히 포맷팅하는 함수니까 toLocaleString처럼 앞에 to를 붙이는게 조금 더 적절하지 않을까 생각이 들지만, 지금 작성하신 이름도 의미는 충분히 전달되는 것 같습니다. util 함수들이 더 다양해지면 prefix를 잘 맞춰보면 좋을 것 같네요. 😄

Copy link
Member

Choose a reason for hiding this comment

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

너도 get보단 toformat이 조금 더 알맞지 않나 싶네요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 to가 더 알맞아 보이네요. to로 수정하겠습니다.

Copy link
Collaborator

@sanginchun sanginchun left a comment

Choose a reason for hiding this comment

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

첫 프론트작업을 맡으셔서 리뷰가 상당히 많아졌군요. 그만큼 수정도 많이 하시고... 그래도 덕분에 팀원들 간에 각자 그동안 리액트를 어떻게 사용해왔는지도 공유되고, 저 개인적으로는 몰랐던 것들도 많이 배웠습니다. 고생 많으셨습니다!! 😃

Copy link
Member

@Choi-Jinwoo Choi-Jinwoo left a comment

Choose a reason for hiding this comment

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

수고하셨어요 ㅎㅎ 단위 포맷하는 부분 코멘트 하나 남겨뒀는데 approve 해두겠습니당 :)

Comment on lines 1 to 3
export const getKoreanMoneyFormat = (amount: number): string => {
return `${amount.toLocaleString()}원`;
};
Copy link
Member

Choose a reason for hiding this comment

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

너도 get보단 toformat이 조금 더 알맞지 않나 싶네요 ㅎㅎ

@momongss momongss merged commit 8fe818f into develop Aug 16, 2021
@momongss momongss deleted the feat/frontend-product-list branch August 16, 2021 03:32
Comment on lines +94 to +99

return (
<>
{productListResponse === null ? (
<div>로딩중..</div>
) : (
Copy link
Member

Choose a reason for hiding this comment

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

if (productListResponse === null) return Loading

return ( .... )

이렇게도 괜찮을 것 같아요! 개인 취향인듯합니다! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 기능 frontend 프론트엔드
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants