[Mission001] todo crud by pci2676 · Pull Request #40 · eastjun-dev/frontend

pci2676 · 2020-04-20T04:46:02Z

하다보니 어느 파일에 어떤 메서드가 위치해야될지 점점 헷갈려졌어요...
많이 이상하겠지만 리뷰 잘부탁드립니다...ㅎㅎ

새로운 todo item 등록기능

eastjun-dev

@pci2676
비밥!!!!!! 정말 열심히 코드를 짠게 보여서 너무너무너무 감동입니다!!!!! 👍👍👍
리뷰가 늦어진 점 정말 너무 미안하구요 ㅠㅠ 😭
자바스크립트 코드를 보니, 대부분 객체들의 prototype을 활용해주었더라구요. 그런데 prototype에 선언된 메서드들에서 dom을 활용한 굉장히 구체적인 로직들이 들어있어서 여러 인스턴스를 생성했을 때 원치 않는 동작들이 발생할 수도 있어 보여요. 예를들어 View 객체에서

function View() {
  const todoList = document.querySelector("#todo-list");
  const todoCount = document.querySelector(".count");
  const filters = document.querySelector(".filters");
}

위와 같이 구현되어있는데요, 한 페이지에 준의 투두리스트와 비밥의 투두리스트를 만드는 인스턴스를 2개 만들어서 조작한다고 했을 때 원치 않는 동작이 발생할 수 있을 것 같아요. 그래서 프로토타입을 통해 적극적으로 활용한다면,

function View($listElement, $countElement, $filters) {
}

new View($listElement, $countElement, $filters)

위와 같이 파라미터로 커스텀한 태그들을 받아서 사용하면 좋을 것 같아요~

그리고 Prototype 형태를 적극 활용한다면 자바스크립트의 최신문법인 class를 활용해보는 것도 좋을 것 같아요 :)

처음 하는거라 쉽지 않았을텐데 너무 고생 많았어요!!!!

eastjun-dev · 2020-05-12T09:14:59Z

mission001/pci2676/js/app.js

+}
+
+function initialize(app) {
+    console.log(app);


꼭 필요한 경우가 아니라면 console.log는 지우는게 좋아요 :)

제거했습니다!

eastjun-dev · 2020-05-12T09:16:40Z

mission001/pci2676/js/todo/Controller.js

+    }
+
+    Controller.prototype.toggleClick = (event) => {
+        const target = event.target.offsetParent;


엘리먼트를 담는 변수의 경우 보통 const $target과 같이 일반 변수와 구분할 수 있게 선언해주는 경우가 많습니다 :)
강제적인건 아니지만 일반적인 컨벤션이니 참고해주세요 :)

$가 붙으니까 구분하기 더 편해졌네요!

eastjun-dev · 2020-05-12T09:20:12Z

mission001/pci2676/js/todo/Template.js

@@ -0,0 +1,20 @@
+const Template = {
+    getNewItem: (entity) => {
+        const li = document.createElement('li');


자바스크립트에서 dom api를 이용해 element를 생성할 수도 있지만, 가독성 측면에서 마크업 구조를 확인하기 어렵지는 않을까요?
지금 형태의 경우에는

const template = { getNewItem() { return ` <li id="todo-${entity.id}" class="${entity.status}"> <div class="view"> <input class="toggle" type="checkbox"> <label class="label">${entity.value}</label> <button class="destroy"></button> </div> <input class="edit" value=${entity.value}> </li>`; } };

이렇게 작성해도 같은 동작을 할 것 같아요 :)

참고로 오브젝트 내부에 메서드 형태로 함수를 선언할 때는 화살표 함수를 사용하지 않는게 좋습니다.
링크를 참고해주시면 좋을 것 같아요 :)

아하... 컨텍스트가 다른곳을 바라보게 되는군요..!
감사합니다!

eastjun-dev · 2020-05-12T09:22:01Z

mission001/pci2676/js/todo/View.js

+
+    View.prototype.addNewItem = (entity, eventListener) => {
+        const item = Template.getNewItem(entity);
+        item.querySelector('.toggle').onclick = eventListener.toggleClick;


모든 todoItem의 toggle, destory, label, edit 등에 이벤트를 바인딩하면 한 페이지에서 너무 많은 이벤트가 바인딩 될 수 있기 때문에, 이벤트 위임을 사용해보면 좋을 것 같아요 :)
왜 이벤트 위임을 해야 하는가?

현재 domElement.onclick으로 클릭 이벤트를 바인딩하고 있는데요. 좀 더 일반적으로는 addEventListener 방식을 이용합니다. 왜냐하면 한 엘리먼트에 여러 이벤트를 바인딩할 수 있고, 이벤트 제거도 명시적으로 메서드로 제공하기 때문이에요 :)

document.getElementById('clickMe').addEventListener('click', onClick); // 이벤트 연결 document.getElementById('clickMe').addEventListener('click', onClick2); // 또 하나의 이벤트 연결 document.getElementById('clickMe').removeEventListener('click', onClick); // 연결할 이벤트 중 하나 제거

메모리 누수 현상이 심각하게 발생할수 있네요!
이벤트 위임방식으로 변경했습니다!

eastjun-dev · 2020-05-12T09:25:09Z

mission001/pci2676/js/todo/Controller.js

+    }
+
+    Controller.prototype.toggleClick = (event) => {
+        const target = event.target.offsetParent;


엘리먼트를 찾기 위해 offsetParnet를 이용하고 있는데, closest가 아닌 offsetParent를 사용해야 하는 이유는 무엇일까요?
event.target을 포함한 offsetParent를 찾는 과정에 css가 변경되면서 position에 대한 값이 달라진다면 원치 않는 동작을 일으킬 수 있을 것 같아요 😭
offsetParent

엘리먼트의 style.display가 "none"으로 설정되면 null을 반환한다.

위 부분이 offsetParent의 문제점이 될수 있을것 같아요!
closest로 하는게 안전하겠네요!

eastjun-dev · 2020-05-12T12:03:15Z

mission001/pci2676/js/app.js

+    const view = new View();
+    const storage = new Storage();
+    const service = new Service(storage);
+    app.Controller = new Controller(service, view);


view, storage, service와는 다르게 controller만 app 내부의 객체로 선언한 이유가 있을까요?

생각해보니 내부 객체로 선언할 필요가 없네요..!!
괜히 노출만 되는것 같아요!

eastjun-dev · 2020-05-12T12:04:52Z

mission001/pci2676/js/utils/utils.js

@@ -0,0 +1,20 @@
+const Utils = {


Utils객체를 사용하는곳이 없는데 만든 이유가 있을까요?

이전에 쓰려다가 안쓴 파일이었네요..!!
지웠습니다!

eastjun-dev · 2020-05-12T12:12:16Z

mission001/pci2676/js/todo/Controller.js

+    Controller.prototype.editContent = (event) => {
+        const li = event.target.offsetParent;
+
+        if (isEsc(event)) {


isEsc와 같은 validation 체크해주는 함수들을 따로 분리한 validator 객체가 있으면 어떨까요? Controller에 선언하기 보다는 따로 객체로 분리되어 여러곳에서 재사용할 수 있으면 좋을 것 같아요 :)

validators.js 파일을 하나 만들어서 한쪽에서 처리하게 해줬습니다!

eastjun-dev · 2020-05-12T12:20:00Z

mission001/pci2676/js/todo/View.js

+        const target = document.querySelector('.selected').classList.item(0);
+        if (target === 'all') {
+            for (let li of todoList.getElementsByTagName('li')) {
+                li.style.display = '';


자바스크립트에서 엘리먼트의 스타일을 직접 조작하는 것은 지양하는 패턴이에요
css 파일에

.d-none { display: none; }

과 같은 스타일 클래스를 추가하고, 해당 엘리먼트에 d-none이라는 클래스를 추가하거나 빼는 방식으로 스타일을 적용하면 좋을 것 같아요 :)

해당 방식으로 바꿨습니다!

eastjun-dev · 2020-05-12T12:21:01Z

mission001/pci2676/js/todo/View.js

+            }
+        } else {
+            for (let li of todoList.getElementsByTagName('li')) {
+                if (li.className === target) {


li.style.display = li.className === target ? "" : "none"

중첩된 부분이 많은데, 이렇게 간략하게 표현할 수도 있을 것 같아요 :)

classList에 add 하는 방식으로 변경하니까 ''과 같은 빈칸을 넣으면 문법 오류가 발생해서

아래와 같은 방식으로 변경했습니다!

for (let $li of $todoList.getElementsByTagName('li')) { if ($li.className !== filter) { $li.classList.add('d-none') } }

pci2676 · 2020-05-26T16:55:12Z

준!
피드백 정말 감사합니다!
피드백 덕에 모호했던 몇가지 개념들이 정립된 것 같아요! ㅎㅎ

pci2676 added 9 commits February 17, 2020 17:03

feat : inputItem.js 작성

127295d

새로운 todo item 등록기능

feat : inputItem.js 작성

f4f58a5

새로운 todo item 등록기능

프로젝트 초기화

7eb64e4

feat : add new todo item

d06f250

feat : toggle event

28106c3

feat : remove event

650b36d

feat : 수정 기능 완성

3f73b52

feat : 아이템 성격별로 갯수새기

9bd50f1

feat : 필터별로 보이게 하기

bf87e51

pci2676 requested a review from eastjun-dev April 20, 2020 04:46

eastjun-dev requested changes May 13, 2020

View reviewed changes

refactor : 피드백 반영

90b29e6

Conversation

pci2676 commented Apr 20, 2020

Uh oh!

eastjun-dev 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!

Choose a reason for hiding this comment

Uh oh!

pci2676 commented May 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants