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주차] 송아영 미션 제출합니다. #2

Open
wants to merge 16 commits into
base: main
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
32 changes: 32 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<!DOCTYPE html>
<html lang="ko">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>To Do List</title>
<link rel="stylesheet" href="style.css" />
</head>
<body>
<div class="todo-wrapper">
<header>
<h1>To Do List</h1>
</header>

<main>
<div>
<div class="todo-input">
<input type="text" placeholder="할 일을 입력하세요..." />
<button id="append-button">추가</button>
</div>

<ul class="todo-list"></ul>
</div>
<div class="side"></div>
</main>
</div>

<script src="script/date.js"></script>
<script src="script/todo.js"></script>
<script src="script/modal.js"></script>
</body>
</html>
13 changes: 13 additions & 0 deletions script/date.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const header = document.querySelector("header");
const date = document.createElement("div");
const now = new Date();

const options = {
year: "numeric",
month: "long",
day: "numeric",
weekday: "long",
};

date.innerText = now.toLocaleDateString("ko-KR", options);
header.appendChild(date);
66 changes: 66 additions & 0 deletions script/modal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
function handleUncheckedDeleteButtonClick(e) {
const label = e.target.previousElementSibling;

// 완료되지 않은 투두 삭제 시 확인 모달
if (!label.classList.contains(CHECKED_CLASS)) {
openModal(label);
}
}

function openModal(label) {
const modalWrapper = document.createElement("div");
const modal = document.createElement("div");
const content = document.createElement("div");
const buttonWrapper = document.createElement("div");
const cancelButton = document.createElement("button");
const confirmButton = document.createElement("button");

modalWrapper.classList.add("modal-wrapper");
Copy link
Collaborator

Choose a reason for hiding this comment

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

class 설정을 한번에 처리하는 util함수도 고려해볼만 할 것 같아요!

function addClasses(element, classNames) { classNames.forEach(cls => element.classList.add(cls)); }

요런 느낌으로! 저는 코드 짤 때 한번에 처리할 수 있거나 분리가 가능한 것은 유틸함수로 빼서 사용하려고 노력하는중입니다 ._.

Copy link
Author

Choose a reason for hiding this comment

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

동일한 element에 여러 class를 할당하는 로직이 반복될 때에는 확실히 보여주신 코드처럼 함수를 작성해도 좋을 것 같네요!

modal.classList.add("modal");
content.classList.add("modal-content");
buttonWrapper.classList.add("button-wrapper");
cancelButton.classList.add("cancel-button");
confirmButton.classList.add("confirm-button");

content.innerText = "완료되지 않은 투두입니다.\n정말로 삭제할까요?";
cancelButton.innerText = "취소";
confirmButton.innerText = "삭제";

modalWrapper.addEventListener("click", handleOutsideClick);
cancelButton.addEventListener("click", closeModal);
confirmButton.addEventListener("click", () =>
handleConfirmButtonClick(label)
);

modalWrapper.appendChild(modal);
modal.appendChild(content);
modal.appendChild(buttonWrapper);
buttonWrapper.appendChild(cancelButton);
buttonWrapper.appendChild(confirmButton);

document.body.appendChild(modalWrapper);

// 엔터 입력으로 확인 버튼이 눌릴 수 있도록 포커싱
confirmButton.focus();
}

function closeModal() {
const modalWrapper = document.querySelector(".modal-wrapper");
modalWrapper.remove();
}

function handleOutsideClick(e) {
if (e.target === e.currentTarget) {
closeModal();
}
}

function handleConfirmButtonClick(label) {
const li = label.parentElement;
li.remove();
closeModal();
}

deleteButtons.forEach((deletebutton) =>
deletebutton.addEventListener("click", handleUncheckedDeleteButtonClick)
);
78 changes: 78 additions & 0 deletions script/todo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
const todoInput = document.querySelector('input[type="text"]');
Copy link
Collaborator

Choose a reason for hiding this comment

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

전역으로 반복되는 요소를 한번씩 호출하여 참조하는 거 너무 좋습니다 👍

const appendButton = document.querySelector("#append-button");
const deleteButtons = document.querySelectorAll(".delete-button");
const checkboxes = document.querySelectorAll(".todo-item input");

const CHECKED_CLASS = "checked-todo-content";

Choose a reason for hiding this comment

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

다른 파일에서도 사용되고 있는 상수이니 export 해서 모듈화해주어도 좋을 것 같아용
@BeanMouse 님 과제에서 잘 활용해준 예시가 있네요 ✨
https://github.com/CEOS-Developers/vanilla-todo-21th/pull/1/files

Copy link
Author

Choose a reason for hiding this comment

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

모듈화하는 방법을 찾아볼 생각을 못 하고 다만 분할된 script일지라도 동일한 컨텍스트에서 실행된다는 점을 이용해 export import 없이 다른 파일의 변수나 함수를 사용하는 꼼수를 부려 봤습니다 ㅎㅎ💦💦 말씀해 주신 대로 모듈화하여 스코프를 분리하면 더 안정적인 코드가 될 것 같네요!


function handleEnterKeyDown(e) {
if (e.key === "Enter") {
submitNewTodo();
}
}

function submitNewTodo() {
if (todoInput.value === "") return;
Comment on lines +14 to +15

Choose a reason for hiding this comment

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

입력값이 없을 때 submit 되지 않도록 처리해주신 점 좋아요! 다만, 공백(스페이스)만 입력되었을 때도 submit 되지 않도록 추가적인 처리를 해주면 더욱 좋을 것 같습니당


const ul = document.querySelector(".todo-list");
const nth = ul.childElementCount;

const newTodo = document.createElement("li");
const checkbox = document.createElement("input");
const content = document.createElement("label");
const deleteButton = document.createElement("button");

newTodo.classList.add("todo-item");
deleteButton.classList.add("delete-button");

checkbox.type = "checkbox";
checkbox.id = `todo-${nth}`;
content.htmlFor = `todo-${nth}`;
content.innerText = todoInput.value;
Comment on lines +20 to +31

Choose a reason for hiding this comment

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

label 요소의 htmlFor 특징을 활용하여 id값을 가진 요소로의 연결을 구현해주신 부분이 인상깊네요👍👍
다만 이번 과제에서는 추가적으로 구현하지 않으셨지만 만약 localStorage에 저장하는 경우에는 id를 todo-${nth}로 정의했을 때 삭제/추가 과정에서 중복 id가 발생할 가능성이 생기겠다는 생각이 듭니다. 이 부분을 고려하여 고유한 id를 생성하는 방식을 고민해보셔도 좋을 것 같습니다~~

deleteButton.innerText = "삭제";

checkbox.addEventListener("change", handleCheckboxChange);
deleteButton.addEventListener("click", handleDeleteButtonClick);
Copy link
Collaborator

Choose a reason for hiding this comment

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

두번의 click 이벤트가 등록되고 있는데 구현의도는 명확하지만 두 개의 핸들러를 실행시키는 것보다 handleDeleteButtonClick 하나의 핸들러 안에서 분기하면 더 좋을 것 같아요

`function handleDeleteButtonClick(e) {
const label = e.target.previousElementSibling;
const li = e.target.parentElement;

if (label.classList.contains(CHECKED_CLASS)) {
li.remove();
} else {
openModal(label, li);
}
}
` 요런 느낌..?!

Copy link
Author

Choose a reason for hiding this comment

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

지원 님 코멘트를 보고 단순히 handleDeleteButtonClick 내부에서 handleUncheckedDeleteButtonClick 함수를 호출할 생각을 했었는데 영준 님 코드처럼 openModal을 호출하는 편이 더 좋겠네요!

deleteButton.addEventListener("click", handleUncheckedDeleteButtonClick);
Comment on lines +35 to +36

Choose a reason for hiding this comment

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

하나의 deleteButton에 두 가지 핸들러를 따로 두고 있어서 한 번의 클릭에도 불필요한 중복 연산이 발생할 수 있겠네요🥲 체크가 이미 된 항목인지의 여부에 따라 조건문으로 분기처리해서 하나의 핸들러로 처리하면 효율적일 것 같습니다!


newTodo.appendChild(checkbox);
newTodo.appendChild(content);
newTodo.appendChild(deleteButton);

ul.appendChild(newTodo);

todoInput.value = "";
}

function handleDeleteButtonClick(e) {
const label = e.target.previousElementSibling;

// 완료된 투두 삭제
if (label.classList.contains(CHECKED_CLASS)) {
const li = e.target.parentElement;
li.remove();
}
}
Comment on lines +47 to +55

Choose a reason for hiding this comment

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

위에서 말한 분기처리는 이렇게요! + 조건은 isChecked처럼 변수화해서 가독성도 챙겨주기

function handleDeleteButtonClick(e) {
  const label = ~
  const isChecked = label.classList.contains(CHECKED_CLASS);

  if (isChecked) {
    ... // handleDeleteButtonClick 로직
  } else {
    ... // handleUncheckedDeleteButtonClick 로직
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

addEventListener가 여러 개의 이벤트 리스너를 중첩해서 등록할 수 있다는 사실을 새로 학습해서 한번 써먹어 봐야겠다는 생각에 너무 사로잡혔나 봅니다. . . 말씀해 주신 대로 isChecked에 대한 연산이 불필요하게 두 번 동작하겠네요 조건을 변수로 선언해서 가독성을 높이는 방법도 좋은 방법 같습니다!


function handleCheckboxChange(e) {
const checkboxId = e.target.id;
const label = document.querySelector(`label[for="${checkboxId}"]`);

if (e.target.checked) {
label.classList.add(CHECKED_CLASS);
} else {
label.classList.remove(CHECKED_CLASS);
}

Choose a reason for hiding this comment

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

클래스가 하나인 경우에는 toggle 메소드를 사용해서 조건문을 더 간결하게 표현할 수 있을 것 같아요!

}

todoInput.addEventListener("keydown", handleEnterKeyDown);
Copy link

@BeanMouse BeanMouse Mar 13, 2025

Choose a reason for hiding this comment

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

투두를 한글을 입력하고 enter를 눌러서 처리하는 로직에서 enter가 중복으로 처리되는 경우가 있는 것 같습니당.
keydown 은 작동방식이 브라우저마다 차이가 있어 keypress 가 상대적으로 안전하다고 알고 있는데 이런 부분 참고 해보시면 좋을 것 같습니다! 👀

코드 실행 차이
비슷한 오류 관련 블로그

Copy link
Author

Choose a reason for hiding this comment

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

허걱. 좋은 자료 감사합니다!! 사실 이전 프로젝트에서 첨부해 주신 블로그와 동일한 이슈가 있었는데 정확한 원인을 파악하지 못했었거든요 ㅠㅠ 덕분에 좋은 정보 알아갑니다!

Choose a reason for hiding this comment

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

keypress는 deprecated 되어 사용이 권장되지 않으니 isComposing이라는 속성과 keydown을 함께 사용하는 방식을 추천드려요!
keypress
isComposing
한글 입력 후 엔터 시 중복 이벤트 발생

Copy link

@BeanMouse BeanMouse Mar 17, 2025

Choose a reason for hiding this comment

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

저도 방금 보고 후다닥 달려왔어요 MDN에서도 이제 권장하지 않는다네요 ㅜㅜ 위에 분 말씀 처럼 isComposing을 통해서 해결할 수 있다고 합니다


appendButton.addEventListener("click", submitNewTodo);

deleteButtons.forEach((deletebutton) =>
deletebutton.addEventListener("click", handleDeleteButtonClick)
);

checkboxes.forEach((checkbox) =>
checkbox.addEventListener("change", handleCheckboxChange)
);
Comment on lines +72 to +78

Choose a reason for hiding this comment

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

forEach로 매번 삭제버튼, 체크박스마다 이벤트 리스너를 개별 등록하는 방식은 리스트가 많아지면 비효율적일 수 있으므로 이벤트 위임을 적용해보면 어떨까요?

todo-list 하나만 클릭 이벤트를 등록해서 e.target을 통해 실제 클릭된 요소를 파악하여 분기 처리하는 방식을 추천드립니다!

const todoList=document.querySelector(".todo-list");

todoList.addEventListener("click", (e)=>{
  if(e.target.classList.contains("delete-button")){
    handleDeleteButtonClick(e);
  }
  if(e.target.matches('input[type="checkbox"]')) {
    handleCheckboxChange(e);
  }
});

Copy link
Author

Choose a reason for hiding this comment

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

key question에 버블링이 있어서 한번 활용해 봐야겠다 생각했지만. . . 적절한 쓰임을 찾지 못하고 적용하지 못했는데요, 주희 님 코드를 보며 이렇게 사용할 수 있구나 배웠습니다 ㅎㅎ 지원 님도 좋은 피드백 감사합니다!

170 changes: 170 additions & 0 deletions style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
body {
background-color: rgb(240, 245, 240);
color: darkslategrey;

display: flex;
flex-direction: column;
gap: 40px;
justify-content: center;
align-items: center;
}

header {
width: 100%;
border-bottom: 2px solid darkslategrey;
padding-block: 10px;
}

h1 {
font-size: 44px;
margin-right: auto;
margin-block: 2px;
}

main {
width: 100%;
height: 100%;
display: grid;
grid-template-columns: 1fr 150px;
overflow-y: scroll;
scrollbar-width: none;
}

li {
list-style: none;
}

button {
width: 50px;
height: 31px;
margin-left: auto;
border: none;
border-radius: 2px;
color: darkslategrey;
cursor: pointer;
}

.todo-wrapper {
background-color: white;
width: 640px;
height: 800px;
padding: 40px;

display: flex;
flex-direction: column;
justify-content: center;
align-items: center;

border: 2px solid darkslategrey;
box-shadow: 0 2px 8px 0 #00000020;
}

.todo-input {
width: 100%;
display: grid;
grid-template-columns: 1fr 50px;
padding: 5px 8px;
box-sizing: border-box;
border-bottom: 1.5px solid darkslategrey;
}

.todo-input input {
padding: 8px 4px;
border: none;
outline: none;
}

.todo-list {
padding: 0 8px;
margin: 0;
color: black;
}

.todo-item {
width: 100%;
height: 31px;
padding: 5px 0;
border-bottom: 1.5px solid darkslategrey;

display: grid;
grid-template-columns: auto 1fr 50px;

Choose a reason for hiding this comment

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

그리드 레이아웃을 활용해서 아이템 내 요소 간 배치를 정리한 부분 넘 깔끔하네요🥹

gap: 4px;
align-items: center;

font-size: small;
}

.todo-item input {
appearance: none;
width: 16px;
height: 16px;
border: 1.5px solid darkslategrey;
border-radius: 2px;
cursor: pointer;
}

.todo-item input:checked {
background-color: darkslategrey;
}

.todo-item label {
Copy link
Collaborator

Choose a reason for hiding this comment

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

가로 스크롤이 가능하게 해주셨네요! 유니크한 방법인 것 같습니다. 사용자 입장의 방식을 조금 고려하면 word-wrap으로 줄바꿈이 되게하는 것도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

ui적으로 투두마다 동일한 줄 간격을 유지하고 싶은 욕심에 줄바꿈되지 않도록 했습니다 ㅎㅎ... 원래는 줄이 길어지는 경우 새 li 태그를 추가하고 싶었는데 구현이 너무 복잡해지더라고요. 바닐라js에서는 구현이 복잡해지면 쉽게 타협해버리는 것 같습니다. . . 💦💦 리액트로 마이그레이션하면서 구현해 보겠습니다!!

padding: 7px 0;
cursor: pointer;
white-space: nowrap;
overflow: scroll;
scrollbar-width: none;
}

.checked-todo-content {
color: rgb(150, 150, 150);
text-decoration: line-through;
}

.side {
border-left: 2px solid darkslategrey;
}

.modal-wrapper {
position: fixed;
top: 0;
left: 0;
width: 100%;
height: 100%;
background-color: #00000050;

display: flex;
justify-content: center;
align-items: center;
}

.modal {
width: 300px;
height: 150px;
padding: 20px;
box-sizing: border-box;

border-radius: 8px;
background-color: white;

display: flex;
flex-direction: column;
justify-content: space-between;
align-items: center;

font-size: small;
text-align: center;
}

.modal-content {
margin: auto 0;
}

.button-wrapper {
display: flex;
gap: 8px;
}

.confirm-button {
background-color: indianred;

Choose a reason for hiding this comment

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

darkslategrey도 그렇고 컬러명을 정말 잘 활용하시네요 🤩!

color: white;
}