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

React 김주동 sprint7 #97

Conversation

joodongkim
Copy link
Collaborator

요구사항

기본

  • 상품 상세 페이지 주소는 “/items/{productId}” 입니다.

  • response 로 받은 아래의 데이터로 화면을 구현합니다

  • 목록으로 돌아가기 버튼을 클릭하면 중고마켓 페이지 주소인 “/items” 으로 이동합니다.

  • 문의하기에 내용을 입력하면 등록 버튼의 색상은 “3692FF”로 변합니다.

  • response 로 받은 아래의 데이터로 화면을 구현합니다

심화

주요 변경사항

스크린샷

image

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@joodongkim joodongkim added the 순한맛🐑 마음이 많이 여립니다.. label Sep 21, 2024
Copy link
Collaborator

@GANGYIKIM GANGYIKIM left a comment

Choose a reason for hiding this comment

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

주동님 이번주차 스프린트 미션하시느라 고생하셨습니다~
점점 익숙해지시는 것 같아 좋네요~

} finally {
setIsLoading(false);
}
};

useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
resize 시 이벤트를 연결하는 부분과 pageSize가 변하면 데이터를 불러오는 로직이 분리되면 더 좋을 것 같아요

  useEffect(() => {
    const handleResize = () => {
      setPageSize(getPageSize());
    };

    window.addEventListener("resize", handleResize);
    return () => {
      window.removeEventListener("resize", handleResize);
    };
  }, []);

useEffect(() => {
    fetchSortedData({ orderBy: "favorite", pageSize });
}, [pageSize])

Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
날짜 계산은 어려운 편이라 라이브러리 사용하신거 좋습니다!
다만 변수명들이 다 명확해서 몇몇 주석은 없어도 충분히 이해되는 것 같아요

Comment on lines +35 to +37
const products = await getProducts({ orderBy, page, pageSize });
setItemList(products.list);
setTotalPageNum(Math.ceil(products.totalCount / pageSize));
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
반환되는 데이터를 아래처럼 사용하실 수도 있습니다~

Suggested change
const products = await getProducts({ orderBy, page, pageSize });
setItemList(products.list);
setTotalPageNum(Math.ceil(products.totalCount / pageSize));
const { list, totalCount } = await getProducts({ orderBy, page, pageSize });
setItemList(list);
setTotalPageNum(Math.ceil(totalCount / pageSize));

`

function TagDisplay ({ tags }) {
if (!tags || tags.length === 0) return null
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
아래코드도 동일동작합니다.

Suggested change
if (!tags || tags.length === 0) return null
if (!tags || !tags.length ) return null

`

const Tag = styled.div`
background-color: ${({ theme }) => theme.colors.gray[50]};
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
styledComponent theme 사용하신거 좋습니다 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
한 파일에 styledComponent관련 코드와 컴포넌트 하위 컴포넌트-CommentItem, EmptyState- 들이 다 들어있으니 가독성에 좋지 않은 것 같아요.
우선 관련된것끼리 배치하시고 가능하면 하위 컴포넌트를 분리하시면 가독성에 더 좋겠습니다.

Comment on lines +33 to +37
const addTag = (tag) => {
if (!tags.includes(tag)) {
setTags([...tags, tag]);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
중복 태그는 입력 안되게 처리하셨네요 😁
UX를 고민하셔서 작업하시는 습관이 좋아요.
다만 이렇게 되면 사용자는 왜 추가가 안되는지 알지 못하기 때문에
에러메시지 같은 방식으로 사용자에게 안된다는 것을 알려주면 더 좋습니다.


return (
<div>
{title && <Label>{title}</Label>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
label 태그는 사용자가 인터렉션할 수 있는 태그들의 설명을 위한 태그라서
이런경우 아래의 file input과 연결해주시거나 아니면 p, div 같은 태그로 변경하시는걸 추천드립니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
아이콘 컴포넌트를 만드셔서 크기와 색상을 조작해서 사용하시는거 좋네요 👍

Comment on lines +22 to +25
onClick={() => {
onSortSelection("recent");
setIsDropdownVisible(false);
}}
Copy link
Collaborator

@GANGYIKIM GANGYIKIM Sep 23, 2024

Choose a reason for hiding this comment

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

P2:
dropdown option들 모두 click 했을 때 새로 선택된 값 넘기고 안 보이게 해주는 로직이니 공통함수로 빼도 좋겠네요.

const onClick = (e) => {
 onSortSelection(e.target.value);
 setIsDropdownVisible(false);
}

...

<button type='button' value='recent' onClick={onClick}>최신순</button>
<button type='button' value='favorite' onClick={onClick}>인기순 </button>

@GANGYIKIM GANGYIKIM merged commit 4f9c678 into codeit-bootcamp-frontend:React-김주동 Sep 23, 2024
GANGYIKIM pushed a commit that referenced this pull request Oct 29, 2024
* chore: 머지 후 브랜치 삭제 github action 추가

* Initial commit from Create Next App

* fix: 머지 후 브랜치 삭제 github action 수정

* env: workflows 폴더로 이동

* First commit Sprint Mission5 on React

* First commit Sprint Mission5 on React (#78)

* Sprint mission 6 commit.

* Sprint Mission 6 Update 1

* update sprint mission 6

* Sprint mission 6 update commit.

* delete .bak files

* Update Comments

* modify ItemComment

* React 김주동 sprint7 (#97)

* First commit Sprint Mission5 on React

* Sprint mission 6 commit.

* Sprint Mission 6 Update 1

* update sprint mission 6

* Sprint mission 6 update commit.

* delete .bak files

* Update Comments

* modify ItemComment

* refactoring to typescript

* add LoginPage

* First commit

* REFACTOR: styled-components

* Merge branch 'Next-김주동' of https://github.com/joodongkim/10-Sprint-Mission into Next-김주동

* change from React.FC<type> to ({}:type)

---------

Co-authored-by: withyj <[email protected]>
Co-authored-by: hanseulhee <[email protected]>
GANGYIKIM pushed a commit that referenced this pull request Nov 5, 2024
* chore: 머지 후 브랜치 삭제 github action 추가

* Initial commit from Create Next App

* fix: 머지 후 브랜치 삭제 github action 수정

* env: workflows 폴더로 이동

* First commit Sprint Mission5 on React

* First commit Sprint Mission5 on React (#78)

* Sprint mission 6 commit.

* Sprint Mission 6 Update 1

* update sprint mission 6

* Sprint mission 6 update commit.

* delete .bak files

* Update Comments

* modify ItemComment

* React 김주동 sprint7 (#97)

* First commit Sprint Mission5 on React

* Sprint mission 6 commit.

* Sprint Mission 6 Update 1

* update sprint mission 6

* Sprint mission 6 update commit.

* delete .bak files

* Update Comments

* modify ItemComment

* refactoring to typescript

* add LoginPage

* First commit

* REFACTOR: styled-components

* Merge branch 'Next-김주동' of https://github.com/joodongkim/10-Sprint-Mission into Next-김주동

* change from React.FC<type> to ({}:type)

* feat: replace interface to type

* refactoring for nextjs

* feat: sprint#10 first commit

* review: sprint#10

---------

Co-authored-by: withyj <[email protected]>
Co-authored-by: hanseulhee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
순한맛🐑 마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants