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

[Jangbagoony - FE- PR1] #21

Merged
merged 43 commits into from
Apr 26, 2021

Conversation

skawnkk
Copy link

@skawnkk skawnkk commented Apr 23, 2021

  • 메인 페이지 마크업
  • 메인메뉴 탭 클릭 시 데이터 Fetch 요청
  • 상세페이지 모달 팝업

TODO

  • 캐러셀 슬라이드 효과
  • 더보기 버튼 클릭 시 데이터 요청하여 2칼럼 보여주기
  • 상품클릭 시 상세페이지의 해쉬정보로 데이터 요청하기
  • 상세페이지 마크업

skawnkk and others added 30 commits April 20, 2021 16:21
샘플카드 및 theme 작업 중
sample- hover 작업 중 #9
기본 프로젝트 골격을 잡기 위해 Dish, DishController, DishRepository, DishService, BestDishResponseDTO의 틀 생성
기본 프로젝트 골격을 잡기 위해 Badge, BadgeRepository 생성
@MJbae MJbae added the review-FE FE 리뷰 label Apr 23, 2021
pay-napster-x pushed a commit that referenced this pull request Apr 26, 2021
@crongro crongro self-requested a review April 26, 2021 06:17
Copy link

@crongro crongro left a comment

Choose a reason for hiding this comment

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

열심히 구현하신 흔적이 보이네요.
잘 구현하셨고요.

컴포넌트 네이밍 보면 Wrapper 라던가 Div라던가 상황에 따라서 약간 다르게 사용하시는 부분이 있는데 컨벤션을 조금더 맞춰보시면 더 좋을 듯.

리뷰가 좀 늦었네요. 리뷰 참고하세요.

import Category from "./components/category/Category";
import Header from "./components/header/Header";
import { createGlobalStyle } from "styled-components";
const GlobalStyle = createGlobalStyle`
Copy link

Choose a reason for hiding this comment

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

GlobalStyle 도 내용이 많아지면 별도 파일로 분리해도 좋습니다.

return props.size === "L" ? "384px" : "308px";
}};
height: ${(props) => {
return props.size === "L" ? "384px" : "308px";
Copy link

Choose a reason for hiding this comment

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

반복적인 체크를 없애기 위해서 styled component 의 attrs 에 대해서 공부해보세요.

https://styled-components.com/docs/api#attrs


function ItemCard({ data, size }) {
const [detailFetchUrl, setDetailFetchUrl] = useState(null);
const [ModalMode, setModalState] = useState(false);
Copy link

Choose a reason for hiding this comment

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

ModalMode 다른 상태와 마찬가지로 소문자로 작성하시죠.

const [ModalMode, setModalState] = useState(false);
const handleClick = (hash) => {
setModalState(!ModalMode); //작업중
setDetailFetchUrl(
Copy link

Choose a reason for hiding this comment

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

detailFetchUrl 을 생략하고 handleClick 에서 바로 URL 생성해서, fetch 요청을 보낼 수도 있을거 같아요.
뭐가 더 간단할지?에 대한 문제일거 같기도 하고요.

Choose a reason for hiding this comment

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

저희는 useFetch 라는 커스텀 훅을 사용해서 fetch를 하는 중이였기때문에 handleclick 안에서 커스텀 훅을 사용할수 없었습니다. 그렇기 때문에 setDetailFetchUrl 을 사용해서 url을 업데이트하면서 useFetch가 실행되게 했는데 크롱이 해주신 리뷰와 같이 handleClick 에서 바로 넘겨줄수 있는 방법이 있을까요?? @crongro

}
cursor: pointer;
`;
const CategoryBtn = styled(Button)`
Copy link

Choose a reason for hiding this comment

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

네 이런 처리 좋네요.

</ButtonLeft>
<CategoryWrapper>
<CategoryColumn>
{!loadingState &&
Copy link

Choose a reason for hiding this comment

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

loadingstate를 좀더 잘 활용하기 위해서 loading 중임을 표시하는 것도 좋을 거 같네요.

Comment on lines +29 to +31
const HeaderLonIn = styled.button``

const HeaderCart = styled.button``
Copy link

Choose a reason for hiding this comment

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

미래를 위한거겠죠? 그렇지 않다면 styled component가 아닌 button으로 표현하셔도 될 거 같고요.

Comment on lines +47 to +48
const [bestDishMenu, bestDishLoading] = useFetch(basicUrl);
const bestDishData = bestDishMenu.body;
Copy link

Choose a reason for hiding this comment

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

해체할당을 잘 활용하시면,

a = [
    {body: "lorem"}, 'foo'
]

const [{body:data}, second] = a;
console.log(data) //lorem

@crongro crongro merged commit 8d70c1b into codesquad-members-2021:jangbagoony Apr 26, 2021
ehgud0670 pushed a commit that referenced this pull request Apr 27, 2021
ghis22130 added a commit that referenced this pull request Apr 29, 2021
wheejuni pushed a commit that referenced this pull request Apr 29, 2021
@skawnkk skawnkk deleted the fe/feature/detail branch April 30, 2021 15:11
hayoung123 added a commit that referenced this pull request May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE FE 리뷰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants