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 1 commit
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
Binary file modified .DS_Store
Binary file not shown.
58 changes: 49 additions & 9 deletions script.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,33 @@
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.querySelector('.writeTodoForm');
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);
createTodoElement(Todo, false);
todos.push({ text: Todo, completed: false });
saveTodos();
TodoInput.value = '';
} else {
alert('To Do를 입력하세요');
Expand All @@ -19,37 +37,57 @@ function addTodo(e) {
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) {
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 = './icon/notCheck.svg';
toggleIcon.alt = 'Toggle unComplete';
toggleIcon.src = isCompleted ? './icon/checkComplete.svg' : './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');
if (listItem.classList.contains('completed')) {
const isNowCompleted = listItem.classList.contains('completed');
if (isNowCompleted) {
toggleIcon.src = './icon/checkComplete.svg';
toggleIcon.alt = 'Toggle Complete';
} else {
toggleIcon.src = './icon/notCheck.svg';
toggleIcon.alt = 'Toggle unComplete';
}
// 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 = '삭제';
deleteBtn.textContent = 'Del';
deleteBtn.classList.add('delete-btn');
deleteBtn.addEventListener('click', () => {
todoList.removeChild(listItem);
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);
});

// HTML 구조에 맞게 요소 추가
Copy link
Collaborator

Choose a reason for hiding this comment

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

다혜님이 말씀해주신것처럼, appendchild 메서드로 요소를 하나씩 추가하는 것보다 한번에 추가하는 방법을 사용하는게 최적화 측면에서도 좋아보여요!
appendChild가 호출될 때마다 브라우저가 DOM을 업데이트하고, 리플로우와 리페인팅 발생 가능성이 있어요. 때문에, 성능이 저하가 될 수 있어 가급적 dom 조작을 최소화하는 방식을 취하는 것이 좋아보여요.

가빈께 남긴 코드리뷰중에, 리플로우와 리페인팅에 대한 설명을 참고해보시면 좋을 것 같아요!
#4 (comment)

Expand All @@ -71,9 +109,11 @@ function formatDateKorean(date) {
return `${month} ${day}일 ${dayOfWeek}`;
}

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

loadTodos(); // localStorage에서 todos 불러오기
});
109 changes: 98 additions & 11 deletions style.css
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
/*라이브러리*/
@import url("https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/static/pretendard.css");
html{
font-family: "Pretendard";
}
* {
margin: 0;
padding: 0;
Expand Down Expand Up @@ -56,35 +60,118 @@
margin-top: 20px;
display: flex;
flex-direction: column;
background-color: #c3ccff;
background-color: #d6ddff;
}
.check_icon {
width: 20px;
height: 20px;
margin: 0 15px 0 0px;
fill: #788bff;


}
.writeTodoForm {
width: 100%;
height: 52px;
display: flex;
flex-direction: row;
align-items: center;
padding: 0 15px 0 15px;
}
.writeTodoForm > .TodoInput {
width: 100%;
height: 50px;
outline: none;
border: none;
color: #788bff;
background-color: transparent;
font-weight: 700;
font-size: 0.8rem;
}
.writeTodoForm > .TodoInput::placeholder{
color: #788bff;
}
.writeTodoForm > .addBtn, .delete-btn {
cursor: pointer;
background-color: #788bff;
width: 36px;
height: 22px;
border-radius: 10px;
font-size: 0.75rem;
color: white;
padding: 2px;
border: none;
transition: background-color .3s, color .3s;
}
.writeTodoForm > .addBtn:hover, .delete-btn:hover{
Copy link

Choose a reason for hiding this comment

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

hover까지 처리해주신거 너무 좋아요👍👍

background-color: white;
color: #788bff;
}
.todoList{
width:100%;
}
Copy link

Choose a reason for hiding this comment

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

스크린샷 2024-09-09 오후 3 12 08
overflow 처리가 안되어 있어서 todolist 목록이 많아졌을 때,
컨테이너 외부로 넘치고 있어요!
max-height, overflow 속성 적용하면 더 좋을 것 같아요 🤩

Suggested change
}
max-height: 70vh;
overflow-y: auto;
}

overflow docs
😃😃

.todoList li {
width: 100%;
Copy link

Choose a reason for hiding this comment

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

.todoList 자체에 이미 width: 100%가 설정되어 있어서,
li에 적용된 width: 100%는 지워주셔도 좋을 것 같아요 😇👋

border-radius: 10px;
margin-top: 20px;
display: flex;
flex-direction: row;
align-items: center;
background-color: #d6ddff;
height: 50px;
padding: 0 15px 0 15px;
color: #788bff;
font-weight: 700;
font-size: 0.8rem;
Copy link

Choose a reason for hiding this comment

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

다른 값들은 px 단위로, font-size에는 rem 단위로 작성하신 것 같아요!
사실 이번 과제에서는 큰 차이가 없겠지만.. 폰트 외의 다른 값도 rem으로 단위를 통일시켜 주시는게 좋아요! 프로젝트 볼륨이 커질수록 반응형 레이아웃 관리가 더 수월할 거예요! 😸
rem velog

}

.completed-text {
.todoList .completed-text {
text-decoration: line-through;
color: #888;
color: #7d808e;
}

.toggle-icon, .delete-icon {
.todoList .toggle-icon, .delete-icon {
width: 20px;
height: 20px;
cursor: pointer;
}

.toggle-icon {
.todoList .toggle-icon {
margin-right: 10px;
}

.delete-icon {
.todoList .delete-icon {
margin-left: auto;
}

span {
.todoList span {
flex-grow: 1;
}
}

@keyframes slideDownFadeIn {
from {
opacity: 0;
transform: translateY(-20px);
}
to {
opacity: 1;
transform: translateY(0);
}
}
.animate-slide-down {
animation: slideDownFadeIn 0.2s ease-out forwards;
}
/* */

/* 할일 삭제 애니메이션 */
@keyframes fadeOutScaleDown {
from {
opacity: 1;
transform: scale(1);
}
to {
opacity: 0;
transform: scale(0.9);
transform: translateY(+10px);
}
}
.animate-fade-out {
animation: fadeOutScaleDown 0.2s ease-out forwards;
}