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

[Khan] 웹자판기 1주차 PR #19

Merged
merged 19 commits into from
May 12, 2022

Conversation

Han-Seung-Chan
Copy link
Collaborator

@Han-Seung-Chan Han-Seung-Chan commented May 11, 2022

안녕하세요, 빰빰! Khan 입니다! 2주동안 잘 부탁드립니다 :)

데모페이지

vending.machine.-.Chrome.2022-05-11.20-24-31.mp4

구현사항

  • 자판기 부분 레이아웃
  • 투입 된 금액에 따라 구매할 수 있는 상품이 나타난다
  • 상품이 없을 땐 품절 처리 및 선택 안되게
  • 모든 이벤트 message에 나타내기

미구현 부분

  • 반환버튼
  • 지갑 페이지
  • 상품 카운트 처리로 품절 처리
  • 금액 자동보정

@Han-Seung-Chan Han-Seung-Chan changed the title [Khan] 웹자판기 1주차 PR - 1 [Khan] 웹자판기 1주차 PR May 11, 2022
@ppamppamman ppamppamman self-requested a review May 11, 2022 12:35
@ppamppamman
Copy link

안녕하세요 리뷰를 맡게 된 빰빰입니다! 리뷰에 앞서서 이번 프로젝트 기간동안 잘 부탁드립니다!

@Han-Seung-Chan Han-Seung-Chan added the review-FE New feature or request label May 12, 2022
@ppamppamman ppamppamman merged commit 58099d6 into codesquad-members-2022:Khan May 12, 2022
Copy link

@ppamppamman ppamppamman left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!
전반적으로 깔끔하게 작성되어 있어서 리뷰하는데 어려움 없이 슉슉 넘어갈 수 있었습니다! 잘 하고 계시다는 생각이 들어요👏🏻👏🏻👏🏻

Comment on lines +1 to +15
import { css } from 'styled-components';

const flexCenter = css`
display: flex;
justify-content: center;
align-items: center;
`;

const flexBetween = css`
display: flex;
justify-content: space-between;
align-items: center;
`;

export { flexCenter, flexBetween };

Choose a reason for hiding this comment

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

mixin기법을 잘 인지하고 활용하시는 것 같아요! 자주 활용되는 코드를 글로벌에서 관리하는 방식은 좋은 활용방식이라 생각합니다!
스타일드컴포넌트에서는 같은 방식을 테마프로바이더를 만드는 형태로도 관리할 수 있습니다.
https://styled-components.com/docs/advanced#theming

Comment on lines +1 to +13
export async function fetchData(url) {
try {
const response = await fetch(url);

if (!response) {
throw new Error('api failed');
}

return response.json();
} catch (error) {
throw new Error(error);
}
}

Choose a reason for hiding this comment

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

fetch 함수에 대해 간단한 에러 핸들링을 시도하셨군요. 좋은 시도입니다.
에러 핸들링은 단지 에러를 보여주는 것 뿐만 아니라 다음의 시도들도 함께 시도될 수 있습니다.

  1. 에러라면 시스템 레벨에서 n회 다시 시도하게 하기 (재귀적으로 달성 가능하겠죠?)
  2. 유저가 다시시도 할 수 있도록 UI적으로 처리하기

@@ -0,0 +1,24 @@
import { createGlobalStyle } from 'styled-components';
import reset from 'reset-css';

Choose a reason for hiding this comment

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

reset도 많이 시도되는 방식이지만, normalize도 같은 결에서 시도되고는 합니다. 둘의 차이점을 명확하게 아시는 것이 중요하리라 여겨집니다.

Copy link
Collaborator Author

@Han-Seung-Chan Han-Seung-Chan May 12, 2022

Choose a reason for hiding this comment

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

다음 pr에서 normalize.css로 수정해보겠습니다!!

Choose a reason for hiding this comment

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

앗 이거는 권장사항이나 그런게 아니라 차이점을 아시면 좋겠다는 의미였어요 :)

Comment on lines +20 to +24
useEffect(() => {
const drinkUrl = `${process.env.PUBLIC_URL}data/drink.json`;

getDrinkData(drinkUrl, setDrinkData);
}, []);

Choose a reason for hiding this comment

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

이 드링크데이터가 어디서 쓰이는지를 확인해보려고 소스코드를 전체적으로 훑었는데, DrinkMenu에서만 활용되는 것 같더라구요. 그렇다면, 굳이 App에서 만들어지지 않아도 괜찮은것같은데 혹시 App에서 불러서 지속적으로 내려주는 이유가 있을까요?

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 +26 to +30
const getDrinkData = async (url, setData) => {
const response = await fetchData(url);

setData(response.drink);
};

Choose a reason for hiding this comment

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

useState의 set함수를 콜백활용하는 방식이군요! 좋은 시도라고 생각됩니다. 이렇게 되면 나중에 fetch관련된 부분만 커스텀 훅으로 개선할 때도 훨씬 간결하게 변화할 수 있겠군요 :)

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 +11 to +15
const selectDrink = (idx) => {
if (totalMoney < drinkData[idx].price) return;
selectedDrinkMessage(drinkData[idx].name);
setTotalMoney(totalMoney - drinkData[idx].price);
};

Choose a reason for hiding this comment

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

지금은 별 문제가 없겠지만, 인덱스는 언제든지 변경가능한 요소라는 점에서, 개인적으로는 인덱스에 어떤 로직이 깊게 관여되지는 않았으면 좋겠어요. id라는 이름으로 유니크하게 사용되는 요소가 있으니 해당 요소를 활용해보시는 건 어떨까요?

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 +20 to +40
quantity ? (
<DrinkMenuItem
key={id}
price={price}
totalMoney={totalMoney}
soldOut={false}
>
<DrinkNameBtn onClick={() => selectDrink(idx)}>{name}</DrinkNameBtn>
<DrinkPrice>{price}</DrinkPrice>
</DrinkMenuItem>
) : (
<DrinkMenuItem
key={id}
price={Number.POSITIVE_INFINITY}
totalMoney={totalMoney}
soldOut={true}
>
<DrinkNameBtn>{name} 품절</DrinkNameBtn>
<DrinkPrice>X</DrinkPrice>
</DrinkMenuItem>
)

Choose a reason for hiding this comment

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

양이 얼마나 남았는지에 대해서 요소가 길어지게 된다면, 향후 컴포넌트화 해볼 수도 있다고 판단합니다.
어떤 기준으로 '컴포넌트'를 만들게 되는지, 스스로 기준을 명확하게 세우는 것이 앞으로 성장하는 것에 있어서 좋은 표지가 되어줄 거라 생각합니다 :D

Comment on lines +48 to +51
border: ${({ price, totalMoney }) =>
price > totalMoney ? '2px solid black' : '2px solid blue'};

background-color: ${({ soldOut }) => (soldOut ? 'red' : '')};

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.

바로 variables.js 만들어서 수정했습니다!

};

return (
<TotalMoneyContext.Provider value={{ totalMoney, setTotalMoney }}>

Choose a reason for hiding this comment

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

컨텍스트를 나눠서 쓰는 이유를 명확하게 인지하고 계신것 같아서 좋다고 생각합니다👏🏻👏🏻

changePage('/');
changeColor(isClickedVM);
}}
clicked={isClickedVM}

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.

스스로 판단할 수 있는 버튼이 버튼다운거다 라고 말씀하시는게 맞을까여?
제가 질문을 이해하지 못한 거 같습니다 다시 한번 설명해주실 수 있나요?

Choose a reason for hiding this comment

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

아 제가 댓글을 애매하게 달았군요. 그러니까, 클릭되었는지 아닌지 여부를 스스로 판단할 수 있는 버튼이 있다면, 이것은 버튼이라 부르기 적절한지 여쭈고 싶었습니다!

hayoung123 pushed a commit that referenced this pull request May 18, 2022
* 1주차 금 PR 리뷰 반영 및 Router Link 리팩토링 (#17)

* Style: 컴포넌트 리스트에 맞는 변수명으로 변경
Related to: #16

* Fix: useEffect의 dependency 수정
Related to: #16

* Refactor: Link 컴포넌트를 NavLink 컴포넌트로 변경
- NavLink의 isActive를 이용해 스타일을 변경함

* LogContext 생성 및, 돈 투입시 log 메세지 출력 기능 구현 (#19)

* Feat: 자판기의 Log를 기록하는 LogContext 생성
- 돈 투입시 INSERT와 관련된 리듀서가 동작한다.
Related to: #18

* Feat: 돈 입력후 엔터시 투입하는 기능 추가

* Feat: Log가 출력되는 컴포넌트 분리 및 투입 메세지 출력
Related to: #14, #18

* Fix: MoneyContext의 input관련 state를 input 컴포넌트로 이동
- 입력할때마다 리렌더링이 발생해서 state를 input 컴포넌트로 이동함

* Style: 사용하지 않은 키워드 삭제

* 상품 투입 및 구매시 메세지를 보여주도록 구현 (#20)

* Feat: 상품 구입시 투입금액 감소 및 구입 메세지 구현
Related to: #12, #14, #18

* Fix: 지갑의 금액보다 큰 금액 입력시 총금액이 투입되는 메세지 추가

* Feat: 상품과 관련된 ProductsContext 추가
- 상품을 선택하면 상품의 수량이 하나씩 소진된다.
- 수량이 모두 소진되면 품절처리된다.

* Feat: 상품을 선택하면 2초 뒤에 배출되는 메세지 구현
Related to: #12, #14, #18

* Fix: 상품 클릭시 재고가 바로 감소되도록 수정

* Refactor: 금액 투입시 투입된 금액과 개수를 메세지로 출력하도록 수정
Related to: #14

* Chore: github pages 관련 세팅 추가 (#21)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants