Skip to content

[윤장원]-vending-machine#4

Open
jangwonyoon wants to merge 18 commits into
FC-InnerCircle-ICD1:mainfrom
jangwonyoon:jangwon
Open

[윤장원]-vending-machine#4
jangwonyoon wants to merge 18 commits into
FC-InnerCircle-ICD1:mainfrom
jangwonyoon:jangwon

Conversation

@jangwonyoon
Copy link
Copy Markdown

No description provided.

@jangwonyoon jangwonyoon marked this pull request as ready for review July 25, 2024 09:59
Comment thread index.html
<div
class="vending-machine__calc-container flex flex-col gap-2 box-border p-4"
>
<div class="flex gap-2">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

사용자가 입력하고 제출하는 형태의 UI여서
form태그 사용하셔도 좋을거같습니다.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Form 태그를 사용해야겠네요. 감사합니다.

Comment thread src/js/constants/contants.js Outdated
@@ -0,0 +1,41 @@
export const Items = [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

상수는 혹시 대문자로 시작하기로 규칙을 세우신 걸까요? 그렇다면 아래에 있는 btnStyle 도 같은 규칙으로 통일해주는 편이 좋을 것 같습니다. 다만, 첫 문자만 대문자로 사용하는 단어는 JS에서는 사실 클래스 이름에 주로 사용되는 규칙이라서 다른 방식을 고려하면 좋습니다. 널리 통용되는 방식 중 하나로는 대문자와 언더스코어(_) 문자만 사용하하는 게 있습니다.
https://javascript.info/variables#uppercase-constants

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Constants 전체 대문자로 한다는걸 놓친거 같습니다. 언더스코어는 명명은 몰랐는데 인사이트 감사합니다

Comment thread src/js/handlers/eventHandlers.js Outdated
Comment thread src/js/utils/util.js Outdated
Comment on lines +49 to +54
export const returnMoney = (
amount,
totalBalanceView,
transactionLog,
transactionLogContainer,
) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

반환하는 함수가 로그 배열과 로그 컨테이너 엘리먼트의 참조까지 인수로 받는 건 너무 번거로워 보입니다. 다른 함수도 마찬가지고요. 이 부분을 어떻게 개선할 수 있을까요?

Copy link
Copy Markdown
Author

@jangwonyoon jangwonyoon Jul 27, 2024

Choose a reason for hiding this comment

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

transactionLog, transactionLogContainer 인수 부분을 addLog에서만 사용하게 해서 정리해 다시 커밋 했습니다.

Comment thread src/js/handlers/index.js Outdated
Comment thread src/js/utils/util.js Outdated
@@ -0,0 +1,125 @@
// 돈 단위 3자리 계산 함수
export const formatCurrency = (amount) => {
return amount.toString().replace(/\B(?=(\d{3})+(?!\d))/g, ',');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Intl 객체의 numberFormat을 사용해서 이 부분을 리팩토링 해볼 수 있을까요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Intl 객체를 잘 몰랐는데 이번기회에 한번 사용해보겠습니다.

Comment thread src/js/utils/util.js Outdated
Comment on lines +7 to +13
export const convertCurrency = (amount) => {
if (amount < 1000) {
return parseInt(amount, 10);
}

return parseInt(amount.replace(/,/g, ''), 10);
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

코드로 봐서는 명확하지 않은데 amount는 숫자 타입인가요, 문자열 타입인가요?
주석에 있는 함수 설명이 더 명확해야 할 것 같습니다.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

주석을 통해 더 명확하게 작성해야할거 같습니다. 헷갈리는 부분이 있네요. 감사합니다. amount는 number입니다.

Comment thread src/js/utils/util.js
export const findMinPrice = (items) => {
if (items.length === 0) return 0;

return Math.min(...items.map((item) => item.price));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

오 저도 min price를 상수로 빼지말고 찾아서 해야겠네요! 👍🏻👍🏻👍🏻👍🏻👍🏻👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants