Skip to content

[10기 조현학] TodoList with CRUD #210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: hyeonhak
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .idea/.gitignore

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions .idea/js-todo-list-step1.iml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/misc.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions .idea/modules.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/vcs.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

94 changes: 93 additions & 1 deletion index.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ <h1>TODOS</h1>
<span class="todo-count">총 <strong>0</strong> 개</span>
<ul class="filters">
<li>
<a class="all selected" href="/#">전체보기</a>
<a class="all selected" href="#">전체보기</a>
</li>
<li>
<a class="active" href="#active">해야할 일</a>
Expand All @@ -32,7 +32,99 @@ <h1>TODOS</h1>
</li>
</ul>
</div>

</main>
</div>
<script>

Choose a reason for hiding this comment

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

script를 분리하여 js파일로 따로 관리하는게 좋지 않을 까요 ? 🤔

const newItem = (title) => {
const newItemList = document.createElement('li');
const newItemDiv = document.createElement('div');
const newItemLabel = document.createElement('label');
const newItemButton = document.createElement('button');
const newItemCheckbox = document.createElement('input');
const newItemInput = document.createElement('input');

Copy link

Choose a reason for hiding this comment

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

코드를 한줄로 길게 적어주셨지만, 굉장히 정리가 잘되어있는것 같아요! 다른분들 코드를 보면서 파일나누어서 관리하는 것과 함수를 나누어서 관리하는것을 익히시면 실력이 금방 늘어나실것같아요!

newItemDiv.className = 'view';
newItemLabel.className = 'label';
newItemLabel.innerText = title;
newItemButton.className = 'destroy';
newItemCheckbox.className = 'toggle';
newItemCheckbox.type = 'checkbox';
newItemInput.className = 'edit';
Comment on lines +47 to +53

Choose a reason for hiding this comment

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

className에 초기화할때 하드코딩이 아닌 const변수를 선언한 이후에 하는것이 어떤가요? 나중에 수정할때 더 편할것 같아서요

newItemInput.setAttribute('value', title);
newItemList.appendChild(newItemDiv);
newItemList.appendChild(newItemInput);
newItemDiv.appendChild(newItemCheckbox);
newItemDiv.appendChild(newItemLabel);
newItemDiv.appendChild(newItemButton);

newItemCheckbox.addEventListener('click', event => {

Choose a reason for hiding this comment

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

작성하신 방법 코드도 직관적여서 좋은 거같습니다 ㅎㅎㅎ, 반면에 콜백함수를 따로 분리하여 관리하는 게 좋을 거 같다는 생각을 해보았습니다.

newItemCheckbox.addEventListener('click', (event) => completedToggle(event) );

function completedToggle(event){
          if (event.target.value) {
            event.target.setAttribute('checked', true);
            newItemList.className = 'completed';
          } else {
            event.target.removeAttribute('checked');
          }
}

if (event.target.value) {
event.target.setAttribute('checked', true);
newItemList.className = 'completed';
} else {
event.target.removeAttribute('checked');
}
});

newItemButton.addEventListener('click', () => {
newItemList.remove();
const currentCount = document.getElementsByClassName('todo-count')[0].childNodes[1].textContent;
Copy link

Choose a reason for hiding this comment

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

getElementsByClassName으로 요소를 가져오다보니까 배열이 리턴되어서 [0]으로 조회하신것같아요! 그보단 querySelector를 사용하시면 좀 더 깔끔하게 요소를 가져오실것같아요 ㅎㅎ

document.getElementsByClassName('todo-count')[0].childNodes[1].innerText = `${Number(currentCount) - 1}`;
});

newItemList.addEventListener('keydown', (e) => {
if (e.key === 'Escape') {
Comment on lines +76 to +77

Choose a reason for hiding this comment

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

e로 줄이는것보다 풀네임으로 작성해주시는것이 파악하기 더 쉬울것 같습니다

newItemList.removeAttribute('class');
} else if (e.key === 'Enter'){
newItemLabel.innerText = newItemInput.value;
newItemList.removeAttribute('class');
}
});

newItemList.addEventListener('dblclick', () => {
console.log(newItemList);

Choose a reason for hiding this comment

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

콘솔 출력 내용은 지워주셔도 될 것 같습니다!

newItemList.className = 'editing';
});

return newItemList;
}

document.getElementById('new-todo-title').addEventListener('keypress', (event) => {
if (event.key === 'Enter') {

Choose a reason for hiding this comment

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

공통 피드백 적혀있는 글에서 실패코드를 먼저 작성 해주는 게 좋다는 글 보았습니다.

if (event.key !== 'Enter')  return 

이벤트 키가 enter가 아닐 시 return 이러한 방법으로 바꾸는게 좋지 않을 까라는 생각을 해보았습니다.

현학님 코드를 보면서 저또한 배울 수 있어서 감사합니다 !

document.getElementById('todo-list').appendChild(newItem(event.target.value));
const currentCount = document.getElementsByClassName('todo-count')[0].childNodes[1].textContent;
document.getElementsByClassName('todo-count')[0].childNodes[1].innerText = `${Number(currentCount) + 1}`;
}
});

document.querySelector('.filters').addEventListener('click', (event) => {
const tab = event.currentTarget.querySelectorAll('a');
const list = document.getElementById('todo-list').querySelectorAll('li');
tab.forEach((element) => {
element.classList.remove('selected');
})
event.target.classList.add('selected');

list.forEach((element) => {
if (event.target.classList.contains('active')) {
if (element.classList.contains('completed')) {
element.style.display = 'none';
} else {
element.style.display = 'block';
}
} else if (event.target.classList.contains('completed')) {
if (element.classList.contains('completed')) {
element.style.display = 'block';
} else {
element.style.display = 'none';
}
} else {
element.style.display = 'block';
}
Comment on lines +110 to +124

Choose a reason for hiding this comment

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

if문 안에 다시 if문이 나아서 depth가 2인것 같습니다. 안에 if를 함수로 빼는것은 어떤가요?
그리고 112부터 114가, 118부터 121가 동일한 동작이면서 block과 none만 반대로 바뀌는것 같은데 함수로 만들고 true, false로 관리하는것은 어떤가요?

})
})

</script>
</body>
</html>