주산들 - 자판기 온보딩 프로젝트 by DEV4N4 · Pull Request #17 · FC-InnerCircle-ICD2/fe-onboarding-project

DEV4N4 · 2024-11-25T11:16:50Z

1차 리뷰 수정 사항 😃

거의 모든 경우에 엄격한 연산자를 사용하도록 수정
checkBatchimEnding 함수의 44032, 55203에 대한 설명 추가
변할 가능성이 없는 변수는 const로 선언
동작하지 않는 min="0" 삭제
params 설명 {event} 대신 {MouseEvent} 로 변경 (타입 힌트를 더 잘 얻기 위함)
pre 태그로 로그 영역을 구분
변하지 않을 요소들 전역으로 빼서 정리
엘리먼트를 가져오고, 문자열을 파싱해서 현재값을 가져오고, 값을 설정하고, 로그를 추가하는 코드가 반복되는 현상 개선

taggon · 2024-11-28T13:35:34Z

+
+  if (uni < 44032 || uni > 55203) return null;
+
+  return (uni - 44032) % 28 != 0;


가능하면 거의 모든 경우에 != 대신 !==와 같은 엄격한 연산자를 사용하는 버릇을 들이는 게 좋습니다.

넵!! 앞으로 더 신경쓰겠습니다 😃

taggon · 2024-11-28T13:35:51Z

+  if (typeof word !== "string") return null;
+
+  let lastLetter = word[word.length - 1];
+  let uni = lastLetter.charCodeAt(0);
+
+  if (uni < 44032 || uni > 55203) return null;


반환하는 자료형이 설명과 다릅니다.

44032, 55203에 대한 설명이 없습니다. 한국어의 범위를 찾는다는 건 알겠지만, 가능하다면 이 부분에 변수를 새로 선언하거나 주석을 추가하여 위 숫자가 임의의 값처럼 보이지 않도록 하는 게 중요합니다.

const startOfKorean = 44032; // 한국어 첫 글자 '가'의 코드 const endOfKorean = 55203; // 한국어 마지막 글자 '힣'의 코드

넵!! 평소에 주석을 잘 달려고 노력하는 편인데 깜빡했네요 ㅠㅠ 신경쓰겠습니다!
반환하는 자료형도 수정하도록 하겠습니다!

taggon · 2024-11-28T13:57:50Z

+  let lastLetter = word[word.length - 1];
+  let uni = lastLetter.charCodeAt(0);


논란의 여지가 있는 선택이기는 하지만, 변수 선언은 기본을 const라고 생각하는 게 좋습니다. 의도치 않은 실수를 예방할 수 있다는 점, 성능의 이득이 있을 가능성이 있다는 점 등의 이유가 있습니다. 코드의 가독성 측면에서도 변할 가능성이 있는 변수와 변할 가능성이 없는 변수는 구분하여 선언해두면 코드를 읽을 때 조금 더 문맥을 이해하고 읽게 됩니다.

변경될 가능성이 있을 때는 let 그렇지 않을 때는 기본적으로 const를 사용해보세요.

넵!! 기본적으로 const를 사용하는 습관을 들이도록 하겠습니다!! ㅎㅎ

taggon · 2024-11-28T14:04:05Z

+          <div class="insert_return_space">
+            <input
+              type="text"
+              data-type="number"


type="number"를 사용하지 않는 이유가 있을까요?
심지어 min="0"은 type="number"일 때만 유효할텐데요.

앗 원래 number로 했었는데 숫자값 중간에 콤마 찍는 로직을 넣으면서 이것저것 해보다가 실수한것 같습니다 😂 수정하겠습니다!!

taggon · 2024-11-28T14:05:22Z

+    const cursorPosition = target.selectionStart;
+
+    // 원본 값에서 숫자만 추출
+    const originalValue = target.value;


type="number"였으면 valueAsNumber 속성을 사용할 수 있습니다.

오!! 알아두겠습니다! 감사합니다!!

taggon · 2024-11-28T14:07:26Z

+
+/**
+ * item button에서 mouseup 했을 때 실행되는 함수
+ * @param {event} event


{event} 대신 {MouseEvent}를 사용하면 에디터에서 TypeScript 타입 힌트를 조금 더 잘 받을 수 있습니다.

수정하겠습니다! 감사합니다 😃

taggon · 2024-11-28T14:09:11Z

+    const calculatedValue = currentMoney - event.currentTarget.value;
+    moneyInputOriginValue = calculatedValue;
+    moneyInput.value = calculatedValue.toLocaleString();
+    const adj = checkBatchimEnding(event.currentTarget.name) ? "을" : "를";


작은 i18n 관련 팁인데 조사 처리의 어려움이 느껴질 때는 변하지 않을 부분을 추가하면 됩니다. 예를 들어, 여기서는 ${event.currentTarget.name} 상품을 구매했습니다.라고 해도 되겠네요. :)

앗 안그래도 그런 생각은 했었는데 최대한 요구사항이랑 비슷하게 구현하려다보니 이렇게 된 것 같습니다 😁
참고하겠습니다!

taggon · 2024-11-28T14:11:03Z

+    moneyInputOriginValue = calculatedValue;
+    moneyInput.value = calculatedValue.toLocaleString();
+    const adj = checkBatchimEnding(event.currentTarget.name) ? "을" : "를";
+    logArea.innerHTML += `${event.currentTarget.name + adj} 구매했습니다.<br/>`;


이렇게 br로 줄바꿈을 할 거면 pre 태그로 로그 영역을 구분하고 줄바꿈을 사용하는 편이 더 낫지 않았을까요?

오오 pre 태그를 사용하도록 하겠습니다!

taggon · 2024-11-28T14:11:56Z

+  const insertInput = document.querySelector("#insertMoney");
+  const moneyInput = document.querySelector("#currentMoney");


입력 버튼은 전역에 있는데 똑같이 변하지 않을 input 버튼들은 함수 안에 있는 이유가 있을까요?

음.. 함수 안에서 사용할 element들이니 내부에 선언해두는게 코드를 읽고 이해하기 편하지 않을까 생각해서 그렇게 했었습니다.
그런데 같은 선언이 반복되기도 하고 어차피 변하지 않을 요소들이니 전역으로 빼서 정리하는 쪽으로 수정하도록 하겠습니다!!

taggon · 2024-11-28T14:13:58Z

+    const currentMoney = Number(moneyInput.value.replace(/[^0-9.]/g, ""));
+    moneyInput.value = (currentMoney + insertMoney).toLocaleString();


moneyInput의 엘리먼트를 가져오고, 문자열을 파싱해서 현재값을 가져오고, 값을 설정하고, 로그를 추가하는 코드가 반복되는 데 줄일 수 있는 방법은 없을까요?

줄이는 쪽으로 리팩토링 해보겠습니다! 더 생각해보겠습니다

taggon · 2024-12-08T13:49:04Z

+  // 계산 및 log 출력
+  const currentMoney = moneyInputProxy.value;
+  const insertedMoney = event.currentTarget.value;
+  if (currentMoney - insertedMoney > 0) {


이 부분에 버그가 있습니다. 금액이 딱 맞을 때 제품을 못 사요...

Chore: vite 세팅

a97d27f

DEV4N4 added the WIP 아직 작업 중입니다 label Nov 25, 2024

DEV4N4 added this to the Structure milestone Nov 25, 2024

DEV4N4 and others added 18 commits November 26, 2024 09:29

Rename: css, image 폴더 생성

d53c317

feat: reset.css 생성

2cf23c3

feat: ui 기초 작업 완료

b31401d

rename: 사용하지 않는 파일 삭제, 파일 이동

57c3128

style: css 수정

335ea45

remove: 사용하지 않는 파일 삭제 (style.css)

1d52b99

refactor: 반복되는 html 코드 js로 랜더링

502c749

feat: reset.css 추가 (body, html 초기화)

d1ae81f

design: body_wrapper 추가, button hover, active css 추가

60d2a0a

Update README.md

63d2463

rename: CSS 명 변경, js 구현을 위한 id 추가

7bff059

feat: 기본적인 로직 구현

56c7236

Merge branch 'main' of https://github.com/sandeulju/fe-onboarding-pro…

bc4ee71

…ject

fix: click event를 mousedown, mouseleave, mouseup 으로 분리

020877b

design: 투입된 돈 보다 item의 가격이 높을 경우에 띄워줄 text에 넣을 class 추가

347ecd0

feat: input 숫자값 쉼표로 바꿔주는 로직 추가

d1b3b05

feat: log 추가시 자동으로 스크롤 내려주는 기능 구현

4875f9d

comment: 관련 없는 함수 분리, 주석 추가

e6d97fe

DEV4N4 added Needs Review 리뷰받을 준비가 되면 추가해주세요 and removed WIP 아직 작업 중입니다 labels Nov 28, 2024

taggon reviewed Nov 28, 2024

View reviewed changes

taggon added Reviewed 리뷰를 마친 상태입니다 and removed Needs Review 리뷰받을 준비가 되면 추가해주세요 labels Nov 28, 2024

DEV4N4 added WIP 아직 작업 중입니다 and removed Reviewed 리뷰를 마친 상태입니다 labels Nov 30, 2024

DEV4N4 added 2 commits December 1, 2024 19:43

fix: 리뷰 반영하여 코드 수정

8bbc930

refactor: proxy를 이용해 쉼표 포맷팅을 관리하도록 리팩토링

0a45caa

fix: item button에서 mouseup 했을 때 종종 쉼표가 사라지는 버그 수정

a06b775

DEV4N4 added Needs Review 리뷰받을 준비가 되면 추가해주세요 WIP 아직 작업 중입니다 and removed WIP 아직 작업 중입니다 Needs Review 리뷰받을 준비가 되면 추가해주세요 labels Dec 1, 2024

taggon reviewed Dec 8, 2024

View reviewed changes


		if (uni < 44032 \|\| uni > 55203) return null;

		return (uni - 44032) % 28 != 0;

		let lastLetter = word[word.length - 1];
		let uni = lastLetter.charCodeAt(0);

		const insertInput = document.querySelector("#insertMoney");
		const moneyInput = document.querySelector("#currentMoney");

		const currentMoney = Number(moneyInput.value.replace(/[^0-9.]/g, ""));
		moneyInput.value = (currentMoney + insertMoney).toLocaleString();

Uh oh!

Conversation

DEV4N4 commented Nov 25, 2024 • edited by hyoungqu23 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DEV4N4 commented Nov 25, 2024 •

edited by hyoungqu23

Loading