[1주차] 주효정 미션 제출합니다. by jhj2713 · Pull Request #5 · CEOS-Developers/vanilla-todo-15th

jhj2713 · 2022-03-18T00:11:55Z

안녕하세요. CEOS 15기 프론트 주효정입니다. 생각보다 React 없이 코드 짜는 게 낯설고 어렵더라구요:sweat_smile: 새로 시작하는 마음으로 즐겁게 공부하면서 코드 짰습니다!

배포 링크

https://vanilla-todo-15th-d1j4r277m-jhj2713.vercel.app/

어려운 부분

주석
주석을 신경 안 쓰는 몹쓸 습관이 있는데 고쳐보려고 열심히 주석을 써봤는데요... 다시보니 너무 지저분하게 쓴 것 같네요... 더 신경써야 할 것 같습니다:disappointed_relieved:
코드 가독성
뭔가 부족한데,, 마음에 들지 않는데,, 어디가 문제인지 아직 잘 모르겠습니다:joy: 코드 보면서 더 생각해봐야 할 것 같습니다!!!

Key Questions

DOM은 무엇인가요?
Document Object Model의 약자로 문서의 구조화된 표현을 제공하며 프로그래밍 언어가 객체 구조에 접근할 수 있는 방법을 제공합니다.
HTML (tag) Element를 JavaScript로 생성하는 방법은 어떤 것이 있고, 어떤 방법이 가장 적합할까요?
createElement를 사용해서 태그를 추가할 수 있습니다.
Semantic tag에는 어떤 것이 있으며, 이를 사용하는 이유는 무엇일까요?
Semantic tag에는 머릿말을 의미하는 header, 내용을 의미하는 article, 바닥글을 의미하는 footer, 문서의 구역을 정의하는 section 등 다양한 태그가 있습니다.
Semantic tag는 브라우저 뿐만 아니라 개발자가 제목이나 본문의 위치를 한 눈에 이해할 수 있게 해주기 때문에 코드의 가독성을 높이고 의미를 명확하게 하는 용도로 사용할 수 있습니다.
Flexbox Layout은 무엇이며, 어떻게 사용하나요?
Flexbox Layout은 요소의 크기가 동적으로 변할 때 요소를 배치해줄 수 있는 layout입니다. 요소들을 감싸는 container에 display 속성을 주어 Flexbox를 정의할 수 있습니다. 추가로 flex-direction으로 주 축을 결정하고, justify-content로 주 축의 정렬 방법을 설정할 수 있습니다.
JavaScript가 다른 언어들에 비해 주목할 만한 점에는 어떤 것들이 있나요?
컴파일이 필요없어 다른 언어에 비해 시간이 절약됩니다. 그리고 자바스크립트를 사용해 프론트엔드 뿐만 아니라 백엔드에서도 개발할 수 있어 다양하게 사용될 수 있습니다.
코드에서 주석을 다는 바람직한 방법은 무엇일까요?
보는 사람이 이해할 수 있도록 알아보기 쉽게 작성해야 합니다!

감사합니다. 부족한 부분 많이많이 말씀해주세요!!!:smile:

Add input box shadow and fix list style

itsnowkim

효정님 안녕하세요, 프론트엔드 운영진 김현재입니다.

우선 코드를 보기 앞서 커밋 메시지부터 살펴보았는데요,
커밋 메시지를 상당히 잘 작성해주셔서 감동적이었습니다.

fix, style, feat 표시하신 점, 해당 커밋이 어떤 기능을 하는지
기능별로 커밋을 구분하신 점이 추후에 유지보수를 할 때나
다른 팀원들과 협업할 때 큰 힘을 발휘할 것이라고 생각이 듭니다.

전체적으로 코드가 깔끔하고, 함수 작성도 너무 잘 해주셔서
말씀드릴게 딱히 없네요..! 정말 수고하셨고, 저보다 뛰어나신 것 같아서
제가 감히 제안드리기가 어렵네요ㅎㅎ

앞으로 있을 스터디에서도
지금처럼만 해주신다면 정말 감동적일 것 같습니다!

고생하셨습니다~

itsnowkim · 2022-03-18T05:47:51Z

index.html

+      <div class="center-contents">
+        <p class="list-title">TO DO&nbsp<span class="todo-count"></span></p>
+        <ul class="todo-list"></ul>
+      </div>
+      <div class="bottom-contents">
+        <p class="list-title">DONE&nbsp<span class="done-count"></span></p>
+        <ul class="done-list"></ul>
+      </div>


구조를 깔끔하게 잡고 시작하신 점 아주 좋습니다!

추가로 제안드리자면, form 태그로 감싸 enter 입력을 받거나,
onkeypress event를 활용하여 enter를 눌렀을 때에도 등록이 되게 한다면 더 좋을 것 같습니다!

enter는 생각을 못했네요... 조언 감사합니다!!

itsnowkim · 2022-03-18T05:48:23Z

script.js

+
+// 화면에 todo item을 추가
+const paintTodoItem = (text) => {
+  const todoId = new Date().valueOf();


unique한 id를 위해 Date object를 사용하신 점도 좋네요

아이디어 인상깊었습니다!!

itsnowkim · 2022-03-18T05:52:24Z

script.js

+  // todo item 텍스트 추가
+  newTodoText.innerText = text;
+  newTodoText.style.cursor = "pointer";
+  newTodoText.style.fontSize = "15px";
+  newTodoText.style.paddingLeft = "5px";
+  newTodoText.addEventListener("dblclick", toggleTodoToDone);
+
+  // todo item 삭제 버튼 추가
+  todoDel.setAttribute("src", "img/bin.png");
+  todoDel.style.width = "14px";
+  todoDel.style.paddingLeft = "5px";
+  todoDel.style.cursor = "pointer";
+  todoDel.addEventListener("click", deleteTodoItem);
+
+  // li에 item 추가
+  newTodo.setAttribute("id", todoId);
+  newTodo.style.marginBottom = "13px";
+  newTodo.appendChild(newTodoText);
+  newTodo.appendChild(todoDel);


주석과 줄바꿈으로 가독성 고려해주신 점 정말 마음에 드네요

개인적으로 제안드리는 점은 공통적으로 반복되는 속성은 css로 따로 빼고, class로 적용시킬 수도 있을 것 같습니다!
다만 크게 문제되는 점은 아니고 제 생각입니다ㅎㅎ

css를 따로 빼는 방법은 생각 못해봤는데 그 방법도 좋을 것 같아요!!

저도 JS내에서 불필요한 스타일 수정은 가능한 지양하는게 좋다고 들었습니다!

itsnowkim · 2022-03-18T05:55:35Z

script.js

+  // todo item 개수 갱신
+  countTodoItem();
+
+  // localStorage에 갱신된 todoList 저장
+  saveTodoLocalStorage();


정말 로직이 깔끔하네요,..

itsnowkim · 2022-03-18T06:10:05Z

script.js

+
+// 화면에 todo done item을 추가
+const paintDoneItem = (text) => {
+  const todoId = new Date().valueOf();


unique id를 사용하는 아이디어는 정말 좋습니다!
그러나 toggle이 일어날 때마다 id가 새롭게 update되는데,
그 때문에 매번 filter() 를 통해 target을 찾아야 합니다.

todoList 배열의 index를 id로 생각하여 click이 일어날 때(toggle을 하려고 할 때)
해당 index를 이용하여 element에 접근하고, ES6 문법을 사용하여
[...todoList, selected] 이런식으로 배열 뒤에 추가해주는 방법은 어떨까요??

우선 filter를 통해 찾는 O(n)에서 O(1)로 줄일 수 있어서 성능상으로 좋아질 것 같습니다,,
(저도 안해봤습니다만,, 될 것 같아요..?)

오오 좋은 것 같아요! 계속 filter가 사용되면 확실히 성능이 떨어질 것 같네요.. 말씀해주신 대로 시도해보겠습니다!!:smile:

itsnowkim · 2022-03-18T06:29:09Z

script.js

+const countDoneItem = () => {
+  const count = document.querySelector(".done-count");
+  count.innerText = "(" + doneList.length + ")";
+};


앞서 말씀드린대로 바꾸신다면, count 할 때 array length로 바로 구할 수 있을 것 같습니다!

넵 시도해보겠습니다!

itsnowkim · 2022-03-18T06:30:43Z

style.css

+  display: flex;
+  justify-content: center;
+  align-items: center;
+  font-family: "Noto Sans KR", sans-serif;


font 설정 좋아요~

itsnowkim · 2022-03-18T06:31:00Z

style.css

+.todo-input:focus {
+  outline: none;
+  box-shadow: 0px 0px 5px lightgrey;
+}


이런 디테일도 좋습니다~

itsnowkim · 2022-03-18T06:31:34Z

style.css

+  border: none;
+  font-size: 30px;
+  margin-left: 10px;
+  cursor: pointer;


button 같은 경우 global로 cursor: pointer 로 주는 것이 더 편할 것 같아요!

참고해서 고쳐보도록 하겠습니다!

itsnowkim · 2022-03-18T06:31:55Z

style.css

+.todo-list::-webkit-scrollbar,
+.done-list::-webkit-scrollbar {
+  width: 5px;
+}
+.todo-list::-webkit-scrollbar-thumb,
+.done-list::-webkit-scrollbar-thumb {
+  background-color: rgba(0, 0, 0, 0.3);
+  border-radius: 15px;
+}


오호 스크롤바까지!

스크롤바 대박이네요. 중요한데, 전혀 생각도 못하고 있었어요.

S-J-Kim

안녕하세요 효정님

프론트엔드 멘토 김선종입니다.

코드가 전반적으로 구조가 잘 짜여져 있고, 적절한 주석이 있어 코드를 리뷰하기가 편했습니다. 이미 운영진들이 좋은 리뷰를 남겼기에 제가 크게 드릴말씀은 없었고, 참고하면 좋을 만한 몇가지를 코멘트로 남겨보았습니다.

과제 하느라 고생하셨습니다

S-J-Kim · 2022-03-19T11:17:57Z

script.js

+const addTodoItem = () => {
+  const todoContent = document.querySelector(".todo-input").value;
+  if (todoContent) paintTodoItem(todoContent);
+};


Suggested change

const addTodoItem = () => {

const todoContent = document.querySelector(".todo-input").value;

if (todoContent) paintTodoItem(todoContent);

};

const addTodoItem = (event) => {

const todoContent = event.target.value;

if (todoContent) paintTodoItem(todoContent);

};

이벤트 핸들러 함수는 이벤트 객채에서 값을 가져오는 것이 일반적입니다.

그렇군요! 다음부터 참고해서 코드짜도록 하겠습니다!!

S-J-Kim · 2022-03-19T11:21:17Z

script.js

+    id: todoId,
+    text,
+  };
+  todoList.push(todoObj);


Suggested change

todoList.push(todoObj);

todoList = [...todoList, todoObj]

객체에 변형을 가하는 것 보단 새롭게 할당하는 방식으로 객체를 변경하는 것이 좋습니다.

S-J-Kim · 2022-03-19T11:25:14Z

script.js

+const saveTodoLocalStorage = () => {
+  localStorage.setItem("todoList", JSON.stringify(todoList));
+};
+
+// todo done list를 localStorage에 저장
+const saveDoneLocalStorage = () => {
+  localStorage.setItem("doneList", JSON.stringify(doneList));
+};


const saveLocalStorage = (itemType) => { localStorage.setItem(itemType, JSON.stringify(itemType)); } const saveTodoLocalStorage = () => saveLocalStorage('todoList') const saveDoneLocalStorage = () => saveLocalStorage('doneList')

이런식으로 함수를 조금 더 일반화 해서 사용하면 좋겠죠?

넵 감사합니다!

9yujin

안녕하세요~ 1주차 리뷰파트너 한규진입니다! !

주석과 깔끔한 코드에 신경쓰신 부분이 그대로 드러나는것 같습니다. 덕분에 편하게 코드를 읽을 수 있었고 많이 배워갑니다!! 현재 님이 좋은 리뷰 많이 달아주셔서 제 부족한 실력으로는 더 드리고 싶은 말씀이 보이지 않네요ㅠㅠ 그럼 스터디 시간에 뵙겠습니다😁

9yujin · 2022-03-19T15:07:41Z

script.js

+// todo item 개수 출력
+const countTodoItem = () => {
+  const count = document.querySelector(".todo-count");
+  count.innerText = "(" + todoList.length + ")";


템플릿 리터럴을 이용해서 표현할 수 있다고 합니다!

Suggested change

count.innerText = "(" + todoList.length + ")";

count.innerText = `(${todoList.length})`;

아하 알려주셔서 감사합니다~!

9yujin · 2022-03-19T15:09:38Z

script.js

+};
+
+// 화면에 todo done item을 추가
+const paintDoneItem = (text) => {


paintDoneItem과 paintTodoItem 함수에 중복되는 부분이 많으니 하나의 함수로 만들고 완료 여부를 매개변수로 받아와 활용하는 방식은 어떨까요??

중복되는 부분 최대한 제거했어야하는데 못했네요😂 수정해보도록 하겠습니다!

9yujin · 2022-03-19T15:30:22Z

style.css

+  background: linear-gradient(to bottom left, #22c1c3, #fdbb2d);
+}
+body {
+  height: 98%;


height를 98%로 해주신 이유가 궁금합니다!!

height가 100% 였을 때 흰 container의 위치가 마음에 안들어서 어쩌다보니 저런 애매한 수치를 줬던 것 같네요..!

9yujin · 2022-03-19T15:32:41Z

script.js

+
+// 화면에 todo item을 추가
+const paintTodoItem = (text) => {
+  const todoId = new Date().valueOf();


아이디어 인상깊었습니다!!

corinthionia

안녕하세요 효정 님 🙌🏻

우선 효정 님의 깔끔한 코드와 상세한 주석에 코드 리뷰하기 정말 편했습니다! 나중에 협업 과정에서 효정 님의 큰 장점으로 다가올 것 같아요 😊 앞서 다른 분들께서 좋은 코멘트를 많이 달아 주셔서 저는 몇 가지 제안 정도만 달아드리니 참고해 주세요!

과제하시느라 고생 많으셨고 스터디 시간에 뵙겠습니다~

corinthionia · 2022-03-19T16:56:28Z

index.html

+            class="todo-input-button"
+            type="button"
+            value="+"
+            onclick="addTodoItem()"


엘리먼트에 인라인으로 이벤트를 바인딩하는 것은 유지보수성을 악화시켜서 지양하는 것이 좋습니다. script.js 파일 내에서 addEventListener()를 사용해 보세요!

미처 몰랐던 부분이네요! 알려주셔서 감사합니다!!

corinthionia · 2022-03-19T17:00:34Z

script.js

+  newTodoText.style.textDecorationLine = "line-through";
+  newTodoText.style.color = "lightGrey";
+  newTodoText.style.paddingLeft = "5px";
+  newTodoText.addEventListener("dblclick", toggleDoneToTodo);


저도 개인적으로는 현재 js 파일과 css 파일이 구분되어 있으니 스타일 관련 코드는 css에 작성해 주시는 게 더 깔끔할 것 같습니다!

corinthionia · 2022-03-19T17:01:53Z

script.js

+// todo item 개수 출력
+const countDoneItem = () => {
+  const count = document.querySelector(".done-count");
+  count.innerText = "(" + doneList.length + ")";


이 부분도 템플릿 리터럴을 사용해 보시면 좋을 것 같아요

Suggested change

count.innerText = "(" + doneList.length + ")";

count.innerText = `(${doneList.length})`;

넵 앞으로 템플릿 리터럴 사용하도록 해야겠어요!!

jdaeheon

안녕하세요, 코드리뷰를 맡게된 15기 정대헌 입니다!
함수로 잘 정리해주시고 주석도 잘 적어주셔서 코드 보는데 큰 도움이 되었습니다.
이미 많은 분들이 건설적인 댓글 들을 많이 남겨주셔서, 제가 드릴 말씀이 없는 것 같아요.
좋은 코드 공유해주셔서 감사합니다!

jdaeheon · 2022-03-20T02:08:06Z

index.html

+        <div class="todo-input-items">
+          <input class="todo-input" placeholder="할 일을 입력하세요" />
+          <input
+            class="todo-input-button"


<form> 태그안에 <button> 태그를 넣어 POST 요청을 보내는 것도 가능합니다!

<form> <input name="name" type="text"/> <button type="submit"> Send </button> </form>

아하 버튼으로 설정할 생각은 못했네요! 감사합니다~!

jdaeheon · 2022-03-20T02:13:10Z

script.js

+
+// 화면에 todo item을 추가
+const paintTodoItem = (text) => {
+  const todoId = new Date().valueOf();


getTime도 ✨유니크✨한 아이디를 얻기 위해 사용가능합니다. 둘이 동일하다고 하더라구요
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getTime

Suggested change

const todoId = new Date().valueOf();

const todoId = new Date().getTime();

링크까지 공유해주시다니.. 알려주셔서 감사합니다😄

jdaeheon · 2022-03-20T02:15:15Z

script.js

+  // todo item 텍스트 추가
+  newTodoText.innerText = text;
+  newTodoText.style.cursor = "pointer";
+  newTodoText.style.fontSize = "15px";
+  newTodoText.style.paddingLeft = "5px";
+  newTodoText.addEventListener("dblclick", toggleTodoToDone);
+
+  // todo item 삭제 버튼 추가
+  todoDel.setAttribute("src", "img/bin.png");
+  todoDel.style.width = "14px";
+  todoDel.style.paddingLeft = "5px";
+  todoDel.style.cursor = "pointer";
+  todoDel.addEventListener("click", deleteTodoItem);
+
+  // li에 item 추가
+  newTodo.setAttribute("id", todoId);
+  newTodo.style.marginBottom = "13px";
+  newTodo.appendChild(newTodoText);
+  newTodo.appendChild(todoDel);


저도 JS내에서 불필요한 스타일 수정은 가능한 지양하는게 좋다고 들었습니다!

jdaeheon · 2022-03-20T02:17:33Z

script.js

+  const doneStorage = localStorage.getItem("doneList");
+
+  // localStorage에 저장된 list가 있는지 확인
+  if (todoStorage) {


paint란 함수 이름 개인적으로 좋은 것 같아요 ㅎㅎㅎ

jdaeheon · 2022-03-20T02:19:18Z

style.css

+.todo-list::-webkit-scrollbar,
+.done-list::-webkit-scrollbar {
+  width: 5px;
+}
+.todo-list::-webkit-scrollbar-thumb,
+.done-list::-webkit-scrollbar-thumb {
+  background-color: rgba(0, 0, 0, 0.3);
+  border-radius: 15px;
+}


스크롤바 대박이네요. 중요한데, 전혀 생각도 못하고 있었어요.

jhj2713 added 14 commits March 14, 2022 08:51

style: change background color

3cfd63f

style: add container box

10c509c

feat: add addItem function

6a2c065

feat: add deleteItem function

40bc329

feat: add localStorage saving

b6f3390

style: change container size

971dbf2

feat: add addTodoDoneItem function

b177f5a

feat: add deleteDoneItem function

9ce809d

style: change scroll bar

ad0f258

feat: add countItem function

6d7f1d2

style: change UI

f4ef6ca

fix: change ID creation and prevent empty text

8f5cf2a

fix: change count function location

fbb3239

style: change UI

9ae8665

Add input box shadow and fix list style

itsnowkim reviewed Mar 18, 2022

View reviewed changes

fix: add form tag

02d72fb

S-J-Kim reviewed Mar 19, 2022

View reviewed changes

9yujin reviewed Mar 19, 2022

View reviewed changes

corinthionia reviewed Mar 19, 2022

View reviewed changes

jdaeheon reviewed Mar 20, 2022

View reviewed changes

jhj2713 added 3 commits March 20, 2022 20:58

fix: delete style in javascript

6c588f2

fix: delete inline event

7907835

fix: generalize saveStorage function

4bf64a9

	count.innerText = "(" + todoList.length + ")";
	count.innerText = `(${todoList.length})`;

	count.innerText = "(" + doneList.length + ")";
	count.innerText = `(${doneList.length})`;

	const todoId = new Date().valueOf();
	const todoId = new Date().getTime();

Conversation

jhj2713 commented Mar 18, 2022

배포 링크

어려운 부분

Key Questions

Uh oh!

itsnowkim left a comment

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!

S-J-Kim left a comment

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!

9yujin left a comment

Choose a reason for hiding this comment

Uh oh!

9yujin Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

9yujin Mar 19, 2022 •

edited

Loading

corinthionia left a comment •

edited

Loading

jdaeheon left a comment •

edited

Loading