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주차] 이가빈 미션 제출합니다. #4

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
24 changes: 23 additions & 1 deletion index.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,29 @@
</head>

<body>
<div class="container"></div>
<div class="container">
<img src="./src/toYesterday.svg">
<div class="main">
<p class="leftNum">할 일 n개</p>
<h1 class="date">날짜</h1>
<p class="day">요일</p>

<div class="input">
<form id="inputForm">
<input type="text" class="todoInput" placeholder="To Do 입력 후 Enter">
</form>
</div>

<ul class="todoList">
<li>
<img src="./src/${checkedStatus}.svg" class="toggleComplete">
<span style="color: #000000;">세오스 1주차 과제 하기</span>
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
</li>
</ul>
Comment on lines +24 to +30
Copy link

Choose a reason for hiding this comment

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

현재 HTML 파일에 li 요소들이 정의되어 있는데, 이미 JS 파일에서

todoItem.innerHTML = `
      <img src="./src/${checkedStatus}.svg" class="toggleComplete">
      <span style="color: ${textColor};">${todo.text}</span>
      <img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
    `;

이 부분을 통해 투두리스트를 항목을 동적으로 추가하고 있습니다. 따라서 ul 안에 li 요소를 미리 넣어둔 것은 살짝 불필요하다고 느껴집니다..! JS의 innerHTML에 의해서 미리 정의된 li 요소는 사용되지 않거나 덮어쓰이게 되니까요. 다음과 같이 li 요소를 삭제하고 JS에서 리스트를 동적으로 생성하는 방식으로만 처리하는 것이 어떨까요?

Suggested change
<ul class="todoList">
<li>
<img src="./src/${checkedStatus}.svg" class="toggleComplete">
<span style="color: #000000;">세오스 1주차 과제 하기</span>
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
</li>
</ul>
<ul class="todoList">
<!-- JS에서 동적으로 추가됨 -->
</ul>

</div>
<img src="./src/toTomorrow.svg">
</div>
</body>
<script src="script.js"></script>
</html>
115 changes: 114 additions & 1 deletion script.js
Original file line number Diff line number Diff line change
@@ -1 +1,114 @@
//😍CEOS 20기 프론트엔드 파이팅😍
// 날짜 포맷
function formatDate(date) {
const year = date.getFullYear();
const month = (date.getMonth() + 1).toString();
const day = date.getDate().toString();
return `${year}년 ${month}월 ${day}일`;
}

// 요일 포맷
function formatDay(date) {
const days = ['일요일', '월요일', '화요일', '수요일', '목요일', '금요일', '토요일'];
return days[date.getDay()];
}

function dateDisplay(date) {
document.querySelector('.date').innerHTML = formatDate(date);
document.querySelector('.day').innerHTML = formatDay(date);
}

// 투두 렌더링
function loadTodoList(date) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

loadTodoList 함수를 잘 구현해주셨네요! 함수의 역할에 맞게 명칭을 잘 설정해주신 것 같습니다.

fetch: 데이터를 단순히 가져오는 작업에 집중된 단어로, 데이터를 가져오는 작업만 수행하고, 저장하거나 처리하지 않는 경우에 사용하는 반면,
load는, 데이터를 가져와서 처리하거나 저장하는 작업까지 포함하는 경우에 적합하다고 하더라구요.

가빈님께서 구현해주신 loadTodoList 함수는,

  1. 로컬 스토리지에서 가져온 투두 리스트를 렌더링하고,
  2. 완료 상태를 업데이트까지 가능해서, 함수명과 동작이 일치하는 것 같아 좋은 것 같습니다.

지원님 코드리뷰에도 언급했었는데, 함수명이나 변수명을 짓는데 있어 고민되신다면, 카카오 fe 개발자분의 발표 영상 참고해보시면 좋을 것 같아요!
https://www.youtube.com/watch?v=emGLxi0LvNI&t=556s

const todoList = getTodoList(date);
const todoListContainer = document.querySelector('.todoList');
todoListContainer.innerHTML= '';
Copy link

Choose a reason for hiding this comment

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

현재 코드는 loadTodoList() 함수가 실행될 때마다 innerHTML을 전부 초기화하고 다시 생성하는 방식으로 되어 있습니다. 작은 작업에서는 큰 무리가 없지만, 만약 투두 항목이 많아진다면 렌더링 성능이 저하될 수 있어 보입니다! 모든 투두리스트를 한 번에 삭제하고 추가하는 방식보다, 새로운 투두가 추가되었을 때만 새 li 요소를 DOM에 추가하고 기존 투두가 수정되었을 때는 해당 항목만 업데이트하도록 수정하면 어떨까요? 최대한 기존 DOM을 활용하는 방식으로요..!


todoList.forEach((todo, index) => {
const todoItem = document.createElement('li');
const checkedStatus = todo.completed ? 'checked' : 'unchecked';
const textColor = todo.completed ? '#C0C0C0' : '#000000';
Comment on lines +28 to +29

Choose a reason for hiding this comment

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

이렇게 삼항연산자로 간결하게 표현해주셔서 가독성이 좋네요😊


todoItem.innerHTML = `
<img src="./src/${checkedStatus}.svg" class="toggleComplete">
<span style="color: ${textColor};">${todo.text}</span>
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
Copy link
Collaborator

Choose a reason for hiding this comment

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

innerHTML 방식을 활용해서 할일을 간단하게 dom에 추가해주셨네요!!
가빈님의 코드를 보며, innerHTML를 잘 활용하고 계신다는 생각이 들었어요.

innerHTML은 간단하게 dom 조작이 가능하다는 장점이 있지만, 만약 요소 노드에 기존 자식 노드가 존재했다면, 기존 자식 노드들을 제거하고 할당한 html 마크업을 파싱해서 dom에 반영하기 때문에 주의해야 할 필요가 있을 것 같아요!
또한, 사용자가 입력한 값을 이 방식으로 삽입한느 경우에 xss(크로스 사이트 스크립팅 공격)에 취약해 위험하다고 하네요.
왜냐하면, 사용자가 제공한 입력을 직접 HTML로 삽입하기 때문에, 악성 스크립트가 삽입되어 HTML 구조에 포함될 수 있다고 하니 이에 대해 알아두면 좋을 것 같아요!

https://tecoble.techcourse.co.kr/post/2021-04-26-cross-site-scripting/

`;

// 투두 완료
todoItem.querySelector('.toggleComplete').addEventListener('click', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

각 할 일 항목마다 이벤트 리스너가 반복해서 추가되는 것 같아요!

이번 키 퀘스천이였던, 이벤트 버블링의 개념을 활용해보는 것은 어떨까요?!

이벤트 버블링은 하위 요소에서 발생한 이벤트가 상위 요소로 전파되는 것이므로,
상위 요소가 하위 요소들의 이벤트를 한 번에 처리할 수 있어요.

부모 요소(.todoList)에 한 번만 이벤트 리스너를 추가한 후에,
e.target을 이용해 클릭된 자식 요소를 판별하여 로직을 진행시키는 방식을 제안드려요!!

todo.completed = !todo.completed;
saveTodoList(date, todoList);
loadTodoList(date);
});

// 삭제 버튼
todoItem.addEventListener('mouseover', () => {
todoItem.querySelector('.deleteBtn').style.display = 'inline';
});

todoItem.addEventListener('mouseout', () => {
todoItem.querySelector('.deleteBtn').style.display = 'none';
});

todoItem.querySelector('.deleteBtn').addEventListener('click', () => {
todoList.splice(index, 1);
saveTodoList(date, todoList);
loadTodoList(date);
});

todoListContainer.appendChild(todoItem);
Copy link
Collaborator

Choose a reason for hiding this comment

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

가빈님의 코드를 보면, todoList에 forEach를 돌려 각각의 할일마다, DOM에 할일을 추가하고 있는 것 같아요.
혹시 리플로우와 리페인팅에 대해 들어보셨나요?

'만약 자바스크립트 코드에 DOM이나 CSSOM을 변경하는 DOM API가 사용된 경우 DOM이나 CSSOM이 변경된다. 이때 변경된 DOM과 CSSOM은 다시 렌더 트리로 결합되고 변경된 렌더 트리를 기반으로 레이아
웃과 페인트 과정을 거쳐 브라우저의 화면에 다시 렌더링한다. 이를 리플로우 reflow , 리페인트 repaint 라 한다.
리플로우는 레이아웃 계산을 다시 하는 것을 말하며, 노드 추가/삭제, 요소의 크기/위치 변경, 윈도우 리사이징 등 레이아웃에 영향을 주는 변경이 발생한 경우에 한하여 실행된다. 리페인트는 재결합된 렌더 트리를 기반으로 다시 페인트를 하는 것을 말한다.
따라서 리플로우와 리페인트가 반드시 순차적으로 동시에 실행되는 것은 아니다. 레이아웃에 영향이 없는 변경은 리플로우 없이 리페인트만 실행된다. '
모던 자바스크립트 deep dive 책을 인용해보았어요.

appendChild()는 dom에 직접적으로 요소를 추가하는 메서드이기 때문에, 할일 요소별로 이 메서드를 호출한다면 이로 인해 리플로우와 리페인팅이 빈번히 발생해서 서능이 저하될 수 있어요.
때문에, DocumentFragment라는 임시 컨테이너 객체를 만들어서,
todoItem들을 해당 객체에 미리 추가한 후, 마지막에 DocumentFragment 객체를 appendChild로 한 번만 DOM에 추가하면, DOM 조작을 최소화할 수 있을 것 같아요!

https://developer.mozilla.org/ko/docs/Web/API/DocumentFragment

   function loadTodoList(date) {
const todoList = getTodoList(date);
const todoListContainer = document.querySelector('.todoList');
todoListContainer.innerHTML = '';

// DocumentFragment 생성
const fragment = document.createDocumentFragment();

todoList.forEach((todo, index) => {
	const todoItem = document.createElement('li');
	const checkedStatus = todo.completed ? 'checked' : 'unchecked';
	const textColor = todo.completed ? '#C0C0C0' : '#000000';

	todoItem.innerHTML = `
    <img src="./src/${checkedStatus}.svg" class="toggleComplete">
    <span style="color: ${textColor};">${todo.text}</span>
    <img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
  `;

	// 투두 완료
	todoItem.querySelector('.toggleComplete').addEventListener('click', () => {
		todo.completed = !todo.completed;
		saveTodoList(date, todoList);
		loadTodoList(date);
	});

	// 삭제 버튼
	todoItem.addEventListener('mouseover', () => {
		todoItem.querySelector('.deleteBtn').style.display = 'inline';
	});

	todoItem.addEventListener('mouseout', () => {
		todoItem.querySelector('.deleteBtn').style.display = 'none';
	});

	todoItem.querySelector('.deleteBtn').addEventListener('click', () => {
		todoList.splice(index, 1);
		saveTodoList(date, todoList);
		loadTodoList(date);
	});

	// DocumentFragment에 todoItem 추가
	fragment.appendChild(todoItem);
});

// todoListContainer에 한 번에 추가
todoListContainer.appendChild(fragment);
console.log(todoList);

updateLeftNum(todoList); }
 todoListContainer.appendChild(fragment);

});

updateLeftNum(todoList)
}

// 투두 추가
function addTodoItem(date, todoText) {
if (!todoText) return;

const todoList = getTodoList(date);
const newTodo = { text: todoText, completed: false };
todoList.push(newTodo);
saveTodoList(date, todoList);
loadTodoList(date);
}

document.querySelector('#inputForm').addEventListener('submit', (e) => {
e.preventDefault();
const todoInput = document.querySelector('.todoInput');
addTodoItem(currentDate, todoInput.value);
todoInput.value = '';
});

// 남은 할 일 개수
function updateLeftNum(todoList) {
const leftNum = todoList.filter(todo => !todo.completed).length;
document.querySelector('.leftNum').innerHTML = `할 일 ${leftNum}개`;
}

// 날짜 이동
let currentDate = new Date();
dateDisplay(currentDate);
loadTodoList(currentDate);

document.querySelector('img[src*="toYesterday"]').addEventListener('click', () => {
currentDate.setDate(currentDate.getDate() - 1);
dateDisplay(currentDate);
loadTodoList(currentDate);
});

document.querySelector('img[src*="toTomorrow"]').addEventListener('click', () => {
currentDate.setDate(currentDate.getDate() + 1);
dateDisplay(currentDate);
loadTodoList(currentDate);
});

// local storage 관련 코드
function getTodoList(date) {
const storedTodos = localStorage.getItem(date.toDateString());
return storedTodos ? JSON.parse(storedTodos) : [];
}

function saveTodoList(date, todoList) {
localStorage.setItem(date.toDateString(), JSON.stringify(todoList));
}
4 changes: 4 additions & 0 deletions src/checked.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions src/deleteBtn.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions src/toTomorrow.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions src/toYesterday.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions src/unchecked.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
95 changes: 94 additions & 1 deletion style.css
Original file line number Diff line number Diff line change
@@ -1 +1,94 @@
/* 본인의 디자인 감각을 최대한 발휘해주세요! */
@import url("https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/static/pretendard.css");
Copy link

Choose a reason for hiding this comment

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

사실 저는 예전에 @font-face를 통해서 폰트를 직접 호스팅하는 방식으로 코딩했었거든요. 그 때는 사이트가 좀 커졌기 때문에 배포 시 로딩이 오래 걸릴까봐 해당 방법을 택했던 건데, 이 방식에 익숙해진 나머지 전 이번에도 직접 호스팅해서 불러왔어요... 그런데 생각해보니 투두리스트 과제처럼 규모가 작은 프로젝트의 경우에는 가빈 님처럼 CDN 방식으로 가져오는 것이 훨씬 효율적인 것 같네요!! 저도 다음에는 프로젝트의 규모나 배포 속도 등을 전반적으로 고려해서 폰트 불러오는 방식을 다양화해야겠어요 😄


* {
margin: 0;
padding: 0;
box-sizing: border-box;
font-family: "Pretendard";
}

body {
display: flex;
justify-content: center;
align-items: center;
height: 100vh;
background: #F3FAFF;
}

.container {
display: flex;
justify-content: space-between;
align-items: center;
width: 50%;
min-width: 550px;
max-width: 650px;
margin: 0 auto;
}

.main {
text-align:left;
background: white;
padding: 7% 15% 7% 15%;
border-radius: 10px;
box-shadow: 0 4px 8px rgba(0, 0, 0, 0.1);
flex-grow: 1;
margin: 0 20px;
}
Copy link
Member

Choose a reason for hiding this comment

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

image

이런 경우는 거의 없겠지만 todo가 일정 갯수가 넘어가면 화면 잘림 현상이 나타납니다! 아래와 같이 최대 높이를 설정하고 스크롤을 활성화 시키면 좋을 것 같아요

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


.leftNum {
color: #91D1FF;
margin-bottom: 8px;
font-weight: 500;
}

.day {
margin-top: 8px;
font-weight: 500;
}

.todoList {
list-style: none;
padding: 0;
margin-top: 20px;
}

.todoList li {
display: flex;
align-items: center;
padding: 10px 0;
border-bottom: 1px solid #C0C0C0;
margin-top: 5px;
margin-bottom: 5px;
}

span {
margin-left: 5%;
width: 80%;
}

.todoList img {
width: 20px;
height: 20px;
cursor: pointer;
}

.todoInput {
width: 100%;
padding: 10px;
border: 1px solid #91D1FF;
border-radius: 10px;
}

form {
margin-top: 20px;
}

img {
cursor: pointer;
}

img[src*="toYesterday"],
img[src*="toTomorrow"] {
width: 40px;
height: 40px;
}