Skip to content
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

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

Open
wants to merge 1 commit into
base: hyeonhak
Choose a base branch
from

Conversation

HyeonHak
Copy link

안녕하세요 10기 조현학입니다.
기본 요구사항을 구현했고, 심화 요구사항은 공부가 더 필요할 것 같아 구현하지 못했습니다.
기본 요구사항도 깔끔하거나, 그렇진 않지만.. 열심히했습니다 ㅎㅎ..

🎯 요구사항
todo list에 todoItem을 키보드로 입력하여 추가하기
todo list의 체크박스를 클릭하여 complete 상태로 변경 (li tag 에 completed class 추가, input 태그에 checked 속성 추가)
todo list의 x버튼을 이용해서 해당 엘리먼트를 삭제
todo list를 더블클릭했을 때 input 모드로 변경 (li tag 에 editing class 추가) 단 이때 수정을 완료하지 않은 상태에서 esc키를 누르면 수정되지 않은 채로 다시 view 모드로 복귀
todo list의 item갯수를 count한 갯수를 리스트의 하단에 보여주기
todo list의 상태값을 확인하여, 해야할 일과, 완료한 일을 클릭하면 해당 상태의 아이템만 보여주기
🎯🎯 심화 요구사항
localStorage에 데이터를 저장하여, TodoItem의 CRUD를 반영하기. 따라서 새로고침하여도 저장된 데이터를 확인할 수 있어야 함

</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파일로 따로 관리하는게 좋지 않을 까요 ? 🤔

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');
          }
}

}

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 이러한 방법으로 바꾸는게 좋지 않을 까라는 생각을 해보았습니다.

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

Copy link

@HongJungKim-dev HongJungKim-dev left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다!

<orderEntry type="inheritedJdk" />
<orderEntry type="sourceFolder" forTests="false" />
</component>
</module>

Choose a reason for hiding this comment

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

IEEE가 책정한 POSIX에서 Line을 책정할때 한 줄 씩 읽으면서 파일 끝에 EOF가 없으면 문제가 있었다고합니다. 개행하는것이 나을것으로 보입니다.
참고자료

Comment on lines +1 to +6
<?xml version="1.0" encoding="UTF-8"?>
<project version="4">
<component name="ProjectRootManager">
<output url="file://$PROJECT_DIR$/out" />
</component>
</project>

Choose a reason for hiding this comment

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

xml로 작성한 이유가 있으실까요?

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Comment on lines +110 to +124
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';
}

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로 관리하는것은 어떤가요?

});

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.

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

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.

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


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를 사용하시면 좀 더 깔끔하게 요소를 가져오실것같아요 ㅎㅎ

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