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

[1주차] 김류원 미션 제출합니다. #10

Open
wants to merge 7 commits into
base: master
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
60 changes: 0 additions & 60 deletions README.md

This file was deleted.

1 change: 1 addition & 0 deletions icon/NotCheck.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions icon/check.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions icon/checkComplete.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 17 additions & 1 deletion index.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,23 @@
</head>

<body>
<div class="container"></div>
<header>✓ To Do</header>
<section class="todoContainer">
<h1 class="title">TO DO LIST</h1>
<p id="todayDate">9월 6일 금요일</p>

<!-- input container -->
<div class="WriteForm">
Copy link

Choose a reason for hiding this comment

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

WriteForm이나 TodoInput, check_icon에서 네이밍 컨벤션이 다른 부분이 보이는데, 혹시 의도하신걸까요?!

<form id="todoForm" class="writeTodoForm">
<img src="./icon/check.svg" class="check_icon">
<input type="text" class="TodoInput" placeholder="Add a To Do">
<button class="addBtn">Add</button>
</form>
</div>

<!-- todo list -->
<ul class="todoList"></ul>
</section>
</body>
<script src="script.js"></script>
</html>
120 changes: 119 additions & 1 deletion script.js
Original file line number Diff line number Diff line change
@@ -1 +1,119 @@
//😍CEOS 20기 프론트엔드 파이팅😍
const addBtn = document.querySelector('.addBtn');
const TodoInput = document.querySelector('.TodoInput');
Copy link

Choose a reason for hiding this comment

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

여기서도 TodoInput이나 Todo처럼 camelCase를 사용하지 않는 부분이 있어서, 따로 기준을 가지고 계신지 궁금해요!

const todoList = document.querySelector('.todoList');
const todoForm = document.getElementById('todoForm');

let todos = [];

// localStorage에서 todos 불러오기
function loadTodos() {
const savedTodos = localStorage.getItem('todos');
if (savedTodos) {
todos = JSON.parse(savedTodos);
todos.forEach(todo => createTodoElement(todo.text, todo.completed));
}
}

// localStorage에 todos 저장
function saveTodos() {
localStorage.setItem('todos', JSON.stringify(todos));
}

function addTodo(e) {
e.preventDefault();

const Todo = TodoInput.value.trim();

if (Todo) {
createTodoElement(Todo, false);
todos.push({ text: Todo, completed: false });
saveTodos();
TodoInput.value = '';
} else {
alert('To Do를 입력하세요');
}
Comment on lines +27 to +34

Choose a reason for hiding this comment

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

이렇게 항목이 존재하지 않을 때, 에러 핸들링를 해주신건 사용자입장에서 정말 좋을거같아요!
저는 해당 부분을 놓쳤는데, 배워갑니다 ㅎㅎ

}

todoForm.addEventListener('submit', addTodo);
Copy link

Choose a reason for hiding this comment

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

submit 이벤트를 이용해주셔서 enter로도 todo를 추가할 수 있어서 편리했어요👍👍👍


// 투두 추가 함수
function createTodoElement(Todo, isCompleted) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

createTodoElement() 함수 내에
할 일 추가, 완료 토글, 삭제 로직이 모여 있어서 , 나중에 수정할 부분이 생기면, 복잡할 수 있겠다는 생각이 들어요!
때문에 각각의 로직별로 함수로 적절히 분리하면 가독성과 유지보수에 더 좋을 것 같아요!

투두 텍스트 생성 / 토글 생성 / 삭제 버튼 생성 로직을 별도의 함수로 빼낸 후,
createTodoElement() 함수 내에서 이들을 호출하는 식으로 수정해보시는 것을 추천드려요!!

const listItem = document.createElement('li');
listItem.classList.add('animate-slide-down');

if (isCompleted) {
listItem.classList.add('completed');
}

// 완료 토글 아이콘
const toggleIcon = document.createElement('img');
toggleIcon.src = isCompleted ? './icon/checkComplete.svg' : 'https://raw.githubusercontent.com/ryu-won/vanilla-todo-20th/40e5a4dcd0113eadd85034cc953aa3fa97de4527/icon/NotCheck.svg';
toggleIcon.alt = isCompleted ? 'Toggle Complete' : 'Toggle unComplete';
Copy link

Choose a reason for hiding this comment

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

alt까지 챙겨주시는 꼼꼼함 너무 좋아요🫶🫶 저는 까먹기도 하거든요....😅

toggleIcon.classList.add('toggle-icon');

// 투두 텍스트
const todoText = document.createElement('span');
todoText.textContent = Todo;
Copy link

Choose a reason for hiding this comment

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

여기에서 textContent를 써주신 부분이 좋았어요👍👍 저는 innerText를 썼는데, 검색해보니 innerText는 스타일 재계산이 일어나지만, textContent는 스타일 재계산이 일어나지 않으므로 간단한 텍스트 업데이트에는 textContent가 더 성능이 좋다고 합니다! 저두 textContent로 바꿔야겠어요🫶🫶

Copy link
Collaborator

Choose a reason for hiding this comment

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

변수명을 쓸 때, Todo보다 lowercamelcase 컨벤션에 맞추어 todo로 작성하는것이 좋을 것 같아요!

if (isCompleted) {
todoText.classList.add('completed-text');
}

toggleIcon.addEventListener('click', () => {
listItem.classList.toggle('completed');
todoText.classList.toggle('completed-text');
const isNowCompleted = listItem.classList.contains('completed');
if (isNowCompleted) {
toggleIcon.src = './icon/checkComplete.svg';
toggleIcon.alt = 'Toggle Complete';
} else {
toggleIcon.src = 'https://raw.githubusercontent.com/ryu-won/vanilla-todo-20th/40e5a4dcd0113eadd85034cc953aa3fa97de4527/icon/NotCheck.svg';
toggleIcon.alt = 'Toggle unComplete';
Comment on lines +69 to +70

Choose a reason for hiding this comment

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

이미지 소스를 가져올 때 주소가 하드코딩 되어있는데 나중에 쉽게 관리할 수 있도록 아래와 같이 상수로 분리하는 방법도 있을 것 같습니다!

const  NOT_CHECK = 'https://raw.githubusercontent.com/.../NotCheck.svg';

이렇게 하면 해당 이미지를 중복적으로 사용할 때, 이미지 경로를 변경 시 코드 전체에서 한 번만 수정하면 되고, 에러 찾기도 더 쉽지 않을까 생각합니다 😊

}
// localStorage 업데이트
const index = todos.findIndex(item => item.text === Todo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 류원님의 투두 리스트에서 동일한 내용을 가진 항목을 추가하고, 완료하면 첫번째 항목의 completed 값만 true로 변경되고 두번째 항목의 completed 값은 변경되지 않는 것을 확인할 수 있어요.
image

지금 코드를 보면, 항목을 고유하게 식별할 수 있는 값이 없고, 텍스트만을 기준으로 항목을 찾고 업데이트하는 것으로 보여요. 때문에, 텍스트가 동일한 두 항목이 있을 경우 제대로 구분되지 않고 한 항목만 업데이트되는 문제가 발생하는 것 같아요.
배열 메서드 findIndex()에 대한 공식 문서 설명을 보시면, '주어진 판별 함수를 만족하는 배열의 첫 번째 요소에 대한 인덱스를 반환'이라고 되어 있어요. 때문에, 첫 번째 요소 이후의 항목은 무시가 되는 것 같아요.
https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex

이러한 문제를 해결하려면, 각각의 항목을 텍스트 내용에 상관없이 고유하게 식별할 수 있어야 하는데, text를 구분 기준으로 삼기 보다, 고유한 id를 추가해서 Id를 기준으로 식별하는 것이 좋아보여요!

    todos.push({ text: Todo, completed: false });

->
todos.push({ id: Date.now(), text: Todo, completed: false });

...
const index = todos.findIndex(item => item.id === todoId);

이런식으로요!

https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex

if (index !== -1) {
todos[index].completed = isNowCompleted;
saveTodos();
}
});

// 삭제 버튼
const deleteBtn = document.createElement('button');
deleteBtn.textContent = 'Del';
deleteBtn.classList.add('delete-btn');
deleteBtn.addEventListener('click', () => {
listItem.classList.add('animate-fade-out');
setTimeout(() => {
todoList.removeChild(listItem);
// localStorage에서 삭제
todos = todos.filter(item => item.text !== Todo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

할일 항목을 삭제할때도, 텍스트 기준으로 비교되어서
예를 들어 ㅇㅇㅇㅇㅇ라는 항목이 5개가 있는데 하나를 삭제했을 경우, 새로고침하면 5개의 항목이 다 삭제되는 것 같아요!!
이 경우에도 text 기준이 아닌, 항목별로 고유한 Id를 통해 필터링해서, 로직을 처리해주는 것이 적합할 것 같아요!
아까 배열 메서드 중 findIndex는 값과 일치하는 첫 번째 요소를 반환했는데,
filter 메서드는 주어진 배열에서 제공된 함수에 의해 구현된 테스트를 통과한 요소들을 담은 새로운 배열을 반환한다는 차이점도 알아두면 좋을 것 같아요!
https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Array/filter

saveTodos();
}, 300);
});

listItem.appendChild(toggleIcon);
listItem.appendChild(todoText);
listItem.appendChild(deleteBtn);
Comment on lines +94 to +96
Copy link

Choose a reason for hiding this comment

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

혹시 listItem.append(toggleIcon, todoText, deleteBtn);처럼 element를 한번에 추가할 수 있다는 걸 알고 계신가용?! 저도 이번 과제에서 처음 알았는데 유용한 것 같아서 공유드립니다!
https://developer.mozilla.org/en-US/docs/Web/API/Element/append 에서 더 자세히 확인하실 수 있어요🫶🫶

todoList.appendChild(listItem);
}

// 날짜 표시 함수
function formatDateKorean(date) {
const days = ['일요일', '월요일', '화요일', '수요일', '목요일', '금요일', '토요일'];
const months = ['1월', '2월', '3월', '4월', '5월', '6월', '7월', '8월', '9월', '10월', '11월', '12월'];
Comment on lines +102 to +103
Copy link

Choose a reason for hiding this comment

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

이렇게 상수를 선언해 주신 점 좋습니다👍
그런데 JS에서 이런 상수를 선언할 때는 UPPER_SNAKE_CASE로 선언하는 방법이 더 널리 쓰인다고 알고 있어요! 한번 찾아보시면 좋을 것 같아요!!


const month = months[date.getMonth()];
const day = date.getDate();
const dayOfWeek = days[date.getDay()];

return `${month} ${day}일 ${dayOfWeek}`;
}
Comment on lines +101 to +110

Choose a reason for hiding this comment

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

month표현도 day처럼 표현한다면, months라는 변수를 사용하지 않아도 되니까 더 간결하고 일관된 코드가 될 수 있을 것 같습니다!


// 페이지 로드 시 실행
document.addEventListener('DOMContentLoaded', () => {
const todayDateElement = document.getElementById('todayDate');
const today = new Date();
todayDateElement.textContent = formatDateKorean(today);

loadTodos(); // localStorage에서 todos 불러오기
});
Loading