[손원재] JS 자판기 미션 제출 #15
Conversation
hanana1253
left a comment
hanana1253
left a comment
There was a problem hiding this comment.
원재님 코드가 넘 깔끔하고 분리가 잘 되어있네요.
pages 디렉토리 안의 각 메뉴에서 리턴하는 페이지 컴포넌트가 <VendingMachine> 레이아웃 컴포넌트로 감싸져 있는데 라우팅 부분에서 한번에 해줘도 좋지 않을까 라는 생각이 들었습니다.
hustle-dev
left a comment
hustle-dev
left a comment
There was a problem hiding this comment.
React, 라우팅, 쿼리를 직접 만드시면서 그 안의 내부 코드와 동작원리를 많이 학습할 수 있는 좋은 시간이 되었을것 같아요. 시간이 많이 없어서 각 라이브러리의 구현부분을 자세하게 보진 못했는데 간단하게나마 보면서 저 또한 많이 배울 수 있었던것 같습니다. 고생하셨습니다~
| return { value, current: undefined }; | ||
| default: | ||
| return child; | ||
| } |
There was a problem hiding this comment.
먼저 타입이 object인지 확인하고 하면 더 깔끔하게 작성할 수 있을것 같은데 이렇게 하신 이유가 있으신가요?
There was a problem hiding this comment.
코드 작성할때 사용할 타입에만 집중하다보니 object로 걸러낼 생각을 못했던 것 같아요.
말씀하신대로 수정하면 더 깔끔해질 것 같네요!
| @@ -0,0 +1,16 @@ | |||
| /** @jsx createElement */ | |||
| import { createElement, render } from './utils/Soact/v2'; | |||
There was a problem hiding this comment.
여기에서 createElement는 사용하지 않는것 같은데 import를 하신 이유가 있나요?
There was a problem hiding this comment.
아래쪽에 보시면
return <Page />;이 구문이 바벨로 컴파일될때
return createElement(Page)이렇게 컴파일됩니다!
| }; | ||
| const changeQuantity = (e: InputEvent) => { | ||
| setProduct({ ...product, quantity: +(e.target as HTMLInputElement).value }); | ||
| }; |
There was a problem hiding this comment.
useState 쪽 코드를 보고, 이 이벤트관련 코드들을 보니 이렇게 작성하면 사용자가 input의 값을 변경할 때마다 updateDom이 되면서 새로 rendering이 여러번 발생하지 않을까요?
There was a problem hiding this comment.
리액트는 이부분을 더 최적화 했겠지만, 제 방식으로만해도 우선 실제 DOM에 변경은 일어나지 않아요!
비교 알고리즘을 통해 변경사항이 있을때만 변경된 부분에 리렌더링이 발생하기 때문에 state에 의존하고 있지 않다면 rendering은 발생하지 않습니다.
물론 input에 attribute는 변경이 일어납니다.
| const handleSubmit = (e: Event) => { | ||
| e.preventDefault(); | ||
|
|
||
| if (products?.find(({ name }) => name === product.name)) { |
There was a problem hiding this comment.
저도 물론 name으로 상품들을 관리했는데 다시 생각해보니 임의의 id값을 주어서 관리하는것이 더 좋을것 같다는 생각이 들었습니다.
There was a problem hiding this comment.
저도 유틸화하는 입장에서 id를 사용하면 공통화하기 좋아서 그런 생각을 했었어요.
근데 현재 프로젝트에서 우선은 name으로도 충분히 필터링할 수 있어서 이렇게 진행했었습니다.
| alert(`${product.name}는(은) 이미 존재하는 상품입니다.`); | ||
| } else { | ||
| mutate(product); | ||
| setProduct({ name: '', price: 100, quantity: 0 }); |
There was a problem hiding this comment.
요구사항에 최소 금액이 100원이라고 되어 있어서 100을 바인딩했는데, 0으로 초기화해도 됐을 것 같네요.
감샴다.
| id="product-price-input" | ||
| type="number" | ||
| placeholder="가격" | ||
| value={`${product.price}`} |
There was a problem hiding this comment.
위의 상품명에는 {product.name} 으로 하셨는데 여기엔 백틱과 달러사인을 사용하신 이유가 있나요?
There was a problem hiding this comment.
이건 리팩토링 미스입니당...
제가 처음에 createElement를 구현할 때 무조건 타입에 문자열만 받도록 구현했어서 처음에는 백틱과 달러사인으로 타입변환을 하는식으로 구현했는데 코드 수정 후 리팩토링 과정에서 마저 수정 못한 것 같습니다 ㅠ
| id="product-quantity-input" | ||
| type="number" | ||
| placeholder="수량" | ||
| value={`${product.quantity}`} |
There was a problem hiding this comment.
여기도 백틱과 달러사인을 다시 사용하신 이유가 무엇인지 궁금하네요
| updateProductMutate(product); | ||
| updateMoneyMutate(resultMoney); | ||
| } | ||
| }; |
There was a problem hiding this comment.
이 함수로 구매할 수 있는 상품의 이벤트를 각각 거는 것처럼 보이는데 이벤트 위임을 사용해서 하는게 더 좋아보여요
There was a problem hiding this comment.
이벤트 최적화까지는 생각 못했었는데, 확실히 말씀하신대로하면 더 좋겠네요!
| {products?.map(({ name, price, quantity }) => { | ||
| const deleteProduct = () => { | ||
| deleteProductMutate(name); | ||
| }; |
There was a problem hiding this comment.
이 부분도 위임으로 처리하면 더 좋지 않나 생각해봅니다.
| </thead> | ||
| <tbody> | ||
| {products?.map(({ name, price, quantity }) => { | ||
| const deleteProduct = () => { |
|
바닐라 js 이렇게 깔끔하게 구현하시다니 노력의 흔적이 보이네요 |
4 participants
Complete
How to start
구현사항
위의 4가지 라이브러리를 직접 구현해서 자판기 미션을 수행했습니다.
추후에 각 라이브러리 구현한 방법에 대해서 블로그로 작성 후 PR 수정해두겠습니다.