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

[Team19][Json, Dico] Sidedish 2주차 PR (Final) #73

Merged
merged 42 commits into from
May 4, 2021

Conversation

ha3158987
Copy link
Collaborator

@ha3158987 ha3158987 commented May 2, 2021

완료한 일

  • 캐로셀(슬라이드) 구현
  • 캐로셀 NPM 등록
    dico-json-carousel NPM
    Carousel
  • PopUp 모달 구현
  • Skeleton UI 구현
    Skeleton UI

ha3158987 and others added 30 commits April 19, 2021 16:47
[#1] 개발 환경 구축 완료
- BestTab UI
- 상수 파일
    - const.js
- 재사용 컴포넌트
    - Label.jsx
    - ItemCard.jsx
[#5] feat: ✨ BestTab UI 구현
- App.jsx
    - SlideContainer import
- BestItem.jsx
    - 삭제 (미사용)
- BestItems.jsx
    - ItemCard prop 추가
- BestTab.jsx
    - 스타일 수정
- SlideContainer.jsx
    - UI 구현
- SlideItems.jsx
    - UI 구현
- SlideArrowBtn.jsx
    - UI 구현
- ItemCard.jsx
    - 스타일 수정
    - prop 추가
- Label.jsx
    - prop 추가
    - 기본 값 추가
- App.js
    - ShowMoreBtn import
- ShowMoreBtn.jsx
    - UI 구현
[#7] feat: ✨ Slide UI 구현
- App.js
    - Header 경로 수정
- Header.jsx
    - 경로 변경
    - HeaderLeft & Right 분리
- HeaderLeft.jsx
    - 컴포넌트화
    - Navigations 컴포넌트화
- HeaderRight.jsx
    - 컴포넌트화
- Navigations.jsx
    - 컴포넌트화
    - Dropdown 구현
[#10] feat: ✨ Header Dropdown 구현
- BestItems.jsx
   - API 데이터 동기화
- BestTab.jsx
   - useState, useEffect, API 요청
- BestTabContainer.jsx
   - API 데이터 동기화
- BestTabNavigator.jsx
   - API 데이터 동기화
- ItemCard.jsx
   - prop 변경
- Label.jsx
  - COLOR변수 추가
API 데이터 fetch 요청 + 베스트 반찬 tab 랜덤 출력
- App.js
   - PopUpContainer import
-  PopUpContainer.jsx
   - UI 구현
- PopUpImages.jsx
   - UI 구현
- PopUpInformations.jsx
   - UI 구현
[#15] feat: ✨ 상세 modal 페이지 UI 구현
- 파일 및 폴더 구조 변경
   - common 폴더 생성
- Context.jsx
   - useContext 사용하여 prop drilling 개선
- 팝업 이벤트 구현
   - 수량 변경
   - 주문하기
   - 주문결과 안내 메시지 UI
[#19] refactor: 🔨 리팩토링
Refactoring: 코드 리뷰 코멘트 반영 및 개선
- util.js
   - price에 comma 붙이는 기능 구현
- PopUpItemCountContainer.jsx
   - price에 comma 붙이는 기능 import
- ItemCard.jsx
   - price에 comma 붙이는 기능 import
   - 이미지 background로 수정
- Label.jsx
   - 라벨 배경색상 적용
- 모듈화
   - 시연을 위한 기능 구현을 위해 보류
kowoohyuk and others added 11 commits April 30, 2021 02:17
- 슬라이드 명칭을 캐로셀로 변경
- API 데이터 동기화
- 캐로셀의 ItemCard를 children으로 변경
   - 모듈화를 위함!
- 아이템카드 mini, large 프로퍼티 추가
- 상세모달 캐로셀 추가
- 상세모달 스크롤 추가
- 모든 카테고리 보기 기능 구현
슬라이드 API 데이터 동기화 + 모든 카테고리 보기 기능 구현
- Main.jsx
   - 시연을 위한 loop 설정 추가
- BestTab.jsx
   - SkeletonTab import
- BestTabNavigator.jsx
   - 주석 제거
- SkeletonTab.jsx
   - Skeleton UI 구현
- DicoJsonCarousel.jsx
   - Carousel 구현중
BestTab Skeleton UI 만들기
- Context.jsx
   - 주석 제거
- BestTab.jsx
   - 주석 제거
- PopUpContainer.jsx
   - Skeleton import
- PopUpItemsSlide.jsx
   - 주석 제거
- SkeletonPopUpContainerBody.jsx
   - Skeleton UI 구현
PopUp Skeleton UI 만들기
feat: ✨ carousel loop 기능 구현
[#31] feat: ✨ README.md 작성완료
@ha3158987 ha3158987 added the review-FE FE 리뷰 label May 2, 2021
@crongro
Copy link

crongro commented May 4, 2021

dico-json-carousel NPM

오픈소스 축하해요~

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.

꼼꼼하게 구현하셨군요.

리뷰참고하세요!

@@ -0,0 +1,28 @@
const API = () => {
const URL = "https://codesquad-2021-api.herokuapp.com/sidedish";
Copy link

Choose a reason for hiding this comment

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

url이 소스코드에 있는건 위험하긴해요.
빌드타이밍에 소스코드에 개발환경에 따라 동적으로 URL을 주입시키는 방법을 찾아보실래요?

CRA Replace url 이런키워드 검색~

@@ -0,0 +1,28 @@
const API = () => {
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 +38 to +52
<DetailDataContext.Provider value={detailData}>
<OnFetchDetailDataContext.Provider value={onFetchDetailData}>
<PopUpToggleContext.Provider value={popUpToggle}>
<SetPopUpToggleContext.Provider value={setPopUpToggle}>
<MainItemsContext.Provider value={mainItems}>
<MainItemsActiveContext.Provider value={mainItemsActive}>
<SetMainItemsActiveContext.Provider value={setMainItemsActive}>
{children}
</SetMainItemsActiveContext.Provider>
</MainItemsActiveContext.Provider>
</MainItemsContext.Provider>
</SetPopUpToggleContext.Provider>
</PopUpToggleContext.Provider>
</OnFetchDetailDataContext.Provider>
</DetailDataContext.Provider>
Copy link

Choose a reason for hiding this comment

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

어머나.
학습차원에서 Context API로 모든 상태관리를 한것이라고 짐작은 하고 있지만. 그럼에도 피드백 남깁니다.

먼저 Context 를 여러개 지정하는 것이 반드시 필요할까? 생각해봐야하고요.
다시말해서 해당 컴포넌트에서 정의해서 써도 되거나 props로 전달하는 방식을 해도 되지 않을까?

두번째로 Context정의를 최상단 root 영역에서 해야만하는가?
하위 컴포넌트에서 하는 것도 좋지 않을까? 생각해보세요.

Comment on lines +64 to +66
export function usePopUpToggleContext() {
return useContext(PopUpToggleContext);
}
Copy link

Choose a reason for hiding this comment

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

이렇게 useContext 를 반환하는 함수를 사용하는 경우가 있더군요.
제 생각엔 해당 컴포넌트에서 표현해도 크게 다를거 같진 않고요.

Comment on lines +5 to +6
import API from "../../common/api.js";
import { DicoJsonCarousel } from "../util/dj-slider/DicoJsonCarousel.jsx";
Copy link

Choose a reason for hiding this comment

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

요거만 보면
util은 왜 common 이 아니지? 라고 생각할 수도 있을거 같아요.

팀이 정하기 나름인데, 얼핏봐도 두 개가 어떤 차이가 있는지 구별이 되도록 이름 지으면 더 좋을 거 같아요.

`;

export default function SkeletonTab() {
return (
Copy link

Choose a reason for hiding this comment

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

여기 jsx는 여러가지로 혼란스러워보입니다.

Skeleton이라는 네이밍이 너무 반복적으로 많이 보이고요.
예외적으로 이름에 Skeleton을 제외하는 것도 생각해볼 수 있겠죠.

중첩된 구조가 너무 많아 보입니다.
하위컴포넌트로 분리하는 것도 생각해볼 수 있죠.
다른 파일로 분리하기보다, SkeletonTab.jsx 파일안에 하위컴포넌트를 선언해도 좋고요.

Adela eve
Swing Raccoon
Tami Autumn
Daisy ppamppam
Jenny Beemo
Rano 펭도리
Goody Seong
Dico Neis
Junami Sienna
DD Luke
Q Eamon
Kyle json

const dispatch = useSetPopUpToggleContext();
const detailData = useDetailContext();
const onClick = () => {
dispatch(false);
Copy link

Choose a reason for hiding this comment

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

dispatch 라면 dispatch({isFoo: false});
이런식의 메시지를 전달하는 방식이 더 좋은데요.

그럴려면 useReducer에서 사용하는 것이 맞겠죠.
지금은 useState에서 반환된 메서드를 dispatch 라고 해서 useReducer인가? 착각할만한 코드입니다.

따라서 이름을 dispatch를 쓰지 않던가, useReducer를 사용하던가~

const onMakeOrder = async () => {
const resultData = await API(`/buy/${id}/${count}`);
setOrderResult(resultData.result);
setTimeout(() => setOrderResult(null), 2500);
Copy link

Choose a reason for hiding this comment

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

 delay 함수를 만들어서 사용하시죠.

await delay(2500);
setOrderResult(null);

const onMovePage = (count) => {
if (moving) return;
direction.current = 0;
setX(count > 0 ? (-100 / 3) * 2 : 0);
Copy link

Choose a reason for hiding this comment

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

별문제는 아닌데, 3 이런숫자가 의미하는 것이 무엇인지..
없애면 더 좋겠습니다.


const components = [];
if (loop) {
for(let i = (page - 1) * perPanel; i < (page + 2) * perPanel; i++) {
Copy link

Choose a reason for hiding this comment

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

for 문 안에 여러가지 식이 있는데요. for문전에 미리 계산해두면 어때요?

@crongro crongro merged commit ea521ee into codesquad-members-2021:DJ May 4, 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

3 participants