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.

Sign up for GitHub

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주차] 신수진 미션 제출합니다. #7

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[1주차] 신수진 미션 제출합니다. #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
32137fa
feat: 오늘 날짜 가져오기
lemoncurdyogurt Mar 12, 2025
5d28351
feat: 할 일 등록 시, 리스트에 저장
lemoncurdyogurt Mar 13, 2025
ec724d2
feat: 로컬스토리지에 데이터 반영 및 가져오기
lemoncurdyogurt Mar 13, 2025
546f16d
feat: 할 일과 끝낸 일 반영
lemoncurdyogurt Mar 13, 2025
3046af5
feat: 할 일 삭제 기능 추가
lemoncurdyogurt Mar 13, 2025
5bd55b3
fix: 할 일 완료 alert 중복 수정
lemoncurdyogurt Mar 13, 2025
ea0a5a8
feat: 삭제확인 문구 추가
lemoncurdyogurt Mar 13, 2025
8865436
feat: 같은 이름의 할 일 추가 중복 방지
lemoncurdyogurt Mar 13, 2025
bd5fccf
text: 테스트 코드 삭제
lemoncurdyogurt Mar 13, 2025
0db8063
style: 전반적이 스타일 작성
lemoncurdyogurt Mar 13, 2025
05f95e1
style: 스타일 전반적인 수정
lemoncurdyogurt Mar 14, 2025
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
39 changes: 39 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<!DOCTYPE html>
<html lang="en">
Copy link

Choose a reason for hiding this comment

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

lang 속성을 한국어로 지정해주는게 더 좋지 않을까 싶습니다!

<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Vanilla Todo</title>
<link rel="stylesheet" href="style.css" />
</head>
<body>
<div id="header">
<h1>Daily To Do List</h1>
<div class="explain">
<p>오늘의 할 일을 기록해보세요</p>
</div>
</div>
Comment on lines +10 to +15
Copy link

Choose a reason for hiding this comment

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

전체적으로 html 파일 내에서 시맨틱 태그가 사용된 것 같지 않아서 div -> header 등 시맨틱 태그를 활용하여 접근성을 높이면 좋을 것 같습니다!


<div id="main-content">
<div id="date-count-container">
<div id="todayDate"></div>
<div id="count">
<p>🔥: <span id="todoCount">0</span>개</p>
<p>✅ : <span id="doneCount">0</span>개</p>
Comment on lines +17 to +22
Copy link

Choose a reason for hiding this comment

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

id 네이밍이 kebab-case, camelCase 혼용되어 있는 데 가독성을 위해 맞춰주면 좋을 것 같아요

</div>
</div>

<form id="todoPost">
<span>
<input id="todoRecord" type="text" placeholder="할 일 추가" />
</span>
Comment on lines +27 to +29
Copy link

Choose a reason for hiding this comment

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

input을 span으로 감싸신 이유가 따로 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

input을 span으로 감싸신 이유가 따로 있을까요?
input이랑 button을 줄바꿈하지 않고 한줄로 만들어보고자 사용했던 것인데 보니까 필요가 없는 태그 같네요ㅠㅠ

<button id="submitBtn">등록</button>
</form>

<div id="todolist">
<ul id="list"></ul>
</div>
</div>
<script src="script.js"></script>
</body>
</html>
156 changes: 156 additions & 0 deletions script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
//현재 날짜와 시간을 가져오기
Copy link

Choose a reason for hiding this comment

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

코드 전반적으로 주석 처리가 친절해서 이해가 훨씬 쉬웠습니다 😍😍

const currentDate = new Date();
const year = currentDate.getFullYear();
const month = currentDate.getMonth() + 1;
const day = currentDate.getDate();
//요일 구하기
function getWeekDay() {
const week = ["일", "월", "화", "수", "목", "금", "토"];
const weekDay = week[currentDate.getDay()];
return weekDay;
}
const weekDay = getWeekDay(); //요일 함수 선언
const yyyy_mm_dd = `${year}년 ${month}월 ${day}일 ${weekDay}요일`;
Copy link

Choose a reason for hiding this comment

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

변수명도 camelCase로 맞추는게 좋을 것 같아요 .추가로 변수의 의미를 좀 더 명확하게 formattedDate 같은 건 어떨까요 ?

//오늘의 날짜 띄우기
document.getElementById("todayDate").textContent = yyyy_mm_dd;
Comment on lines +7 to +15
Copy link

Choose a reason for hiding this comment

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

요일을 week 배열로 저장한 것 좋습니다~ 다만 toLocaleDateString 메서드를 사용하면 더 편하게 날짜를 가져올 수 있을 것 같습니다!
https://dev-thinking.tistory.com/15

Copy link

Choose a reason for hiding this comment

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

처음에 날짜를 세팅하려는 의도는 파악이 되지만 초기화 해야하는 로직이 많을 땐 분리를 해보는 건 어떨까요 ?

Suggested change
document.getElementById("todayDate").textContent = yyyy_mm_dd;
const todayDateEl = document.getElementById("todayDate");
const init = () => {
todayDateEl.textContent = yyyy_mm_dd;
}


//페이지 로드 시에 로컬스토리지에서 가져오기
document.addEventListener("DOMContentLoaded", loadList);

//버튼 클릭 시, 할일 저장
const submitBtn = document.getElementById("submitBtn");
submitBtn.addEventListener("click", (event) => {
event.preventDefault(); //
const input = document.querySelector("input");
const text = input.value.trim(); //양 끝 공백 제거 후 text에 저장
Comment on lines +24 to +25
Copy link

Choose a reason for hiding this comment

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

이번 과제에서는 input이 하나여도 상관없지만, 이왕 사용자로부터 입력받는 input에 id를 지정했으니까 id로 찾는건 어떨까요??
그리고 변수명도 한 눈에 봤을 때 어떠한 값을 갖고 있는 지 분명했으면 좋겠어요.

Suggested change
const input = document.querySelector("input");
const text = input.value.trim(); //양 끝 공백 제거 후 text에 저장
const inputEl = document.querySelector("#todoRecord");
const userInput = inputEl.value.trim(); //양 끝 공백 제거 후 text에 저장


if (text !== "") {
//text기준으로 로컬 스토리지에 삭제되므로, 같은 text면 등록 방지
Copy link

Choose a reason for hiding this comment

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

text 기준으로 변경된 부분만 로컬 스토리지를 업데이트하는 방식이 효율적인 것 같습니다! 😊👍
저는 기존에 로컬 스토리지에서 데이터 변경이 발생할 때마다 전체를 삭제하고 다시 저장하는 방식을 사용했는데, 이 방식이 불필요한 데이터 처리 과정이 많아 비효율적인 것 같아요. 참고해서 코드 최적화에 대해 다시 고민해 봐야 할 것 같습니당

if (sameTodo(text)) {
alert("이미 같은 할 일이 존재합니다!");
Copy link

Choose a reason for hiding this comment

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

UX 측면에서 고민을 많이 하신 게 보입니다! 😍 다음주에 있을 과제에서 모달로 리팩토링 해 보면 어떨까요? ㅎㅎ

input.value = ""; //사용자 입력칸 빈칸 리셋
input.focus(); //입력창에 포커스
} else {
addToList(text, false);
saveLocalStorage(text, false);
input.value = ""; //사용자 입력칸 빈칸 리셋
input.focus(); //입력창에 포커스
}
Comment on lines +29 to +38
Copy link

Choose a reason for hiding this comment

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

이 부분 사용자 입력칸 빈칸 리셋과 입력창 포커스가 중복되어 아래처럼 작성하면 더 좋을 것 같네요!

if(text !== "") {
   if(sameTodo(text)) {
      alert("이미 같은 할 일이 존재합니다!");
   } else {
      addToList(text, false);
      saveLocalStorage(text, false);
   }
   input.value = "";
   input.focus();
} else {
  ...
}

} else {
alert("할 일을 작성해주세요!");
Copy link

Choose a reason for hiding this comment

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

이 뒤에도 input에 focus 해주면 좋을 것 같아요!

}

getDoneCount();
getTodoCount();
Comment on lines +43 to +44
Copy link

Choose a reason for hiding this comment

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

이 두 개 함수가 계속해서 함께 사용되는 것 같아서 getAllTodoCount 등으로 함수를 만들어 묶어서 사용하면 더 좋을 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

getDoneCount(),getTodoCount()에서 dom을 조작해서 get으로 시작하는 네이밍은 적절치 않아 보여요.
내부에서 localStorage 접근을 두 번 하게 돼서 서연님이 말씀해주신 것처럼 묶어서 사용하되, update로 시작하는 네이밍으로 수정하면 좀 더 명확할 것 같습니다!

});

let doneCount = 0;
Copy link

Choose a reason for hiding this comment

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

함수들의 역할을 잘 부여해서 로직을 분리해보면 doneCount 변수가 굳이 필요 없을 것 같습니다


function addToList(text, checked) {
Copy link

Choose a reason for hiding this comment

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

요 addToList() 함수에 DOM 생성, 스타일링, 이벤트 바인딩, 상태 변경 로직까지 다 포함되어 있는 것 같아 꽤 무거워 보여요 😅😅
addToList() 역할 단위로 쪼개서 코드 가독성도 향상되고, 유지보수 및 확장성이 좋아지도록 책임을 좀 분리해 보면 어떨까요?!

const list = document.querySelector("#list");
let li = document.createElement("li");
let checkBox = document.createElement("input");
checkBox.type = "checkbox";
checkBox.checked = checked;

let span = document.createElement("span");
span.textContent = text;
//삭제 버튼 추가
let deleteBtn = document.createElement("button");
deleteBtn.textContent = "삭제";

//체크 여부에 따라 스타일 변경
if (checked) {
span.style.textDecoration = "line-through";
span.style.color = "gray";
}else{
span.style.color="white"
}
Comment on lines +62 to +68
Copy link

Choose a reason for hiding this comment

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

체크 여부에 따라 className을 주고 css 파일에서 해당 class에 스타일을 지정해주면 더 깔끔할 것 같습니다!


checkBox.addEventListener("change", () => {
doneList(text, checkBox.checked);

span.style.textDecoration = checkBox.checked ? "line-through" : "none";
span.style.color = checkBox.checked ? "gray" : "white";

getTodoCount();
getDoneCount();

let todos = JSON.parse(localStorage.getItem("todos")) || [];
Copy link

Choose a reason for hiding this comment

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

JSON.parse(localStorage.getItem("todos")) || [] -> 요 부분이 거의 모든 함수에서 반복되는데 getTodos() 등의 함수로 추출해서 사용하면 어떨까용? 😆😆 만약 키 이름이 바뀐다면 하나하나 수정해야 하는 번거로움이 발생할 수 있으니 함수에서 통합적으로 관리하는 거죠!

if (doneCount === todos.length) {
alert("축하합니다! 모든 할 일을 다하셨어요!");
Copy link

Choose a reason for hiding this comment

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

alert 창은 확인 버튼을 누르기 전까지 사용자의 동작을 막아 UX를 저해하기 때문에 별로 좋지 않다고 알고 있습니다. 대신 modal, toast 등을 사용하는 걸 추천드립니다!

}
});

//삭제버튼 클릭 시 해당 아이템 삭제
deleteBtn.addEventListener("click", () => {
if (confirm("할 일을 삭제하시겠습니까?")) {
li.remove();
deleteLocalStorage(text);
getTodoCount();
getDoneCount();
alert("삭제 완료");
Copy link

Choose a reason for hiding this comment

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

삭제 완료, 취소까지 알려주는 것 너무 친절한데!!!!!!! 사용자 입장으로 테스트 해 보니 삭제하시겠습니까? 하고 완료를 누른 뒤 alert 창이 한 번 더 떠서 탭으로 또 지워야 하는 게 아주 쪼금!! 불편하더라고요 😢😢 사용자 입장에서 흐름이 매끄러울 수 있도록 과감히 삭제 해 보시는 건 어떨까요?? (저도 과거 과제를 하면서 수진 님처럼 삭제 완료 alert까지 뜨게 했던 것 같은데 그때 들었던 피드백이 아직도 생생합니다...)

} else {
alert("삭제 취소");
}
});

li.append(checkBox);
li.append(span);
li.append(deleteBtn);
Comment on lines +98 to +100
Copy link

Choose a reason for hiding this comment

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

li.append(checkBox, span, deleteBtn);으로 더 간단하게 작성할 수 있을 것 같아요!

list.append(li); // 리스트에 li 추가
}

//로컬스토리지에 저장
function saveLocalStorage(text, checked) {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
todos.push({ text, checked });
localStorage.setItem("todos", JSON.stringify(todos));
}

//로컬스토리지 데이터 가져오기
function loadList() {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
todos.forEach(({ text, checked }) => addToList(text, checked));

getDoneCount();
getTodoCount();
}

function doneList(text, checked) {
Copy link

Choose a reason for hiding this comment

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

checked 상태를 받아서 해결/미해결을 처리하는 데 doneList라는 함수명도 모호한 것 같습니다. updateTodoStatus 같은 네이밍은 어떨까요?

let todos = JSON.parse(localStorage.getItem("todos")) || [];
Copy link

Choose a reason for hiding this comment

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

재할당이 일어나지 않아서 const로 선언해도 될 것 같습니다

todos = todos.map((todo) =>
todo.text === text ? { ...todo, checked: checked } : todo
);

localStorage.setItem("todos", JSON.stringify(todos));

getDoneCount();
getTodoCount();
}

function getTodoCount() {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
let todoCount = todos.filter((todo) => !todo.checked).length;

document.querySelector("#todoCount").innerHTML = todoCount;
Copy link

Choose a reason for hiding this comment

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

getTodoCount()가 실행할 때마다 dom에 접근해서 변수에 저장해두고 사용하면 좋을 것 같습니다.

}

function getDoneCount() {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
doneCount = todos.filter((todo) => todo.checked).length;

document.querySelector("#doneCount").innerHTML = doneCount;
}
Comment on lines +132 to +144
Copy link

Choose a reason for hiding this comment

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

사소하지만 개인적으로 let todos = JSON.parse(localStorage.getItem("todos")) || [];가 중복으로 호출되고 있는데, 두 함수는 함께 쓰이고 있으니 하나로 합치고 호출도 한 번으로 줄이면 좋을 것 같아요!

Suggested change
function getTodoCount() {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
let todoCount = todos.filter((todo) => !todo.checked).length;
document.querySelector("#todoCount").innerHTML = todoCount;
}
function getDoneCount() {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
doneCount = todos.filter((todo) => todo.checked).length;
document.querySelector("#doneCount").innerHTML = doneCount;
}
function updateCounts() {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
let todoCount = todos.filter((todo) => !todo.checked).length;
let doneCount = todos.filter((todo) => todo.checked).length;
document.querySelector("#todoCount").innerHTML = todoCount;
document.querySelector("#doneCount").innerHTML = doneCount;
}


function deleteLocalStorage(text) {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
todos = todos.filter((todo) => todo.text !== text);
localStorage.setItem("todos", JSON.stringify(todos));
}

//로컬스토리지에서 이미 있는 일인지 확인
function sameTodo(text) {
let todos = JSON.parse(localStorage.getItem("todos")) || [];
return todos.some((todo) => todo.text === text);
}
Copy link

Choose a reason for hiding this comment

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

지금 script.js는 하나의 파일 안에 모든 기능(날짜 처리, DOM 조작, 이벤트 처리, 로컬스토리지 관리 등)이 전부 다 들어있어서 한눈에 구조 파악이 어려워요. 😢😢

유지보수 측면에서 모듈화를 제안 드리고 싶어요! script를 아예 기능별로 쪼개서 components / utils 같은 폴더에 나눠 관리하는 건 어떨까요?

그럼 구조가 직관적이게 되어 나중에 기능을 추가하더라도 어디를 어떻게 고쳐야 할지 명확해 질 것 같아요 ㅎㅎ

134 changes: 134 additions & 0 deletions style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
@import url("https://cdn.jsdelivr.net/gh/orioncactus/pretendard/dist/web/static/pretendard.css");
body {
font-family: "Pretendard Variable", Pretendard, -apple-system, BlinkMacSystemFont, system-ui, Roboto, "Helvetica Neue", "Segoe UI", "Apple SD Gothic Neo", "Noto Sans KR", "Malgun Gothic", "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", sans-serif;
margin: 0;
padding: 0;
display: flex;
flex-direction: column;
align-items: center;
background-color: #000000ef;
}

#header {
text-align: center;
font-size: 24px;
Copy link

Choose a reason for hiding this comment

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

css에서 전체적으로 px이 사용된 것 같은데 rem을 사용하는 것을 추천드려요! 아래 글 참고하셔도 좋을 것 같습니다~
https://yozm.wishket.com/magazine/detail/1410/

font-weight: bold;
color:white;
margin-bottom: 0px;
}

#main-content {
display: flex;
flex-direction: column;
justify-content: center;
text-align: center;
margin: 20px auto;
width: 90%;
max-width: 700px;
}

#date-count-container {
color: white;
display: flex;
flex-direction: row;
align-items: center;
justify-content: space-between;
width: 90%;
max-width: 700px;
margin: 0 auto;
}
#todayDate{
font-size: 17px;
}
#count {
font-size: 15px;
display: flex;
gap: 15px;
}

#list {
list-style: none;
}

#list li {
font-size: 15px;
font-weight: bold;
background: #2c2c2c;
padding: 12px;
border-radius: 8px;
box-shadow: 0px 2px 4px rgba(0, 0, 0, 0.1);
margin-bottom: 10px;
display: flex;
justify-content: space-between;
align-items: center;
}
Comment on lines +53 to +64
Copy link

Choose a reason for hiding this comment

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

입력창 텍스트가 너무 길어지면 줄바꿈이 되지 않고 칸을 뚫고 나가는 현상이 있습니다! 저는 한글이나 영어만 테스트해 보았다가 제출 후 숫자는 줄바꿈이 안 되어서 찾아보았는데 word-break: break-all을 활용하면 모든 문자(숫자 포함)가 공백 상관 없이 줄바꿈이 잘 적용되더라구요!!

Suggested change
#list li {
font-size: 15px;
font-weight: bold;
background: #2c2c2c;
padding: 12px;
border-radius: 8px;
box-shadow: 0px 2px 4px rgba(0, 0, 0, 0.1);
margin-bottom: 10px;
display: flex;
justify-content: space-between;
align-items: center;
}
#list li {
font-size: 15px;
font-weight: bold;
background: #2c2c2c;
padding: 12px;
border-radius: 8px;
box-shadow: 0px 2px 4px rgba(0, 0, 0, 0.1);
margin-bottom: 10px;
display: flex;
justify-content: space-between;
align-items: center;
word-break: break-all;
}

Copy link

Choose a reason for hiding this comment

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

너무 좋은 피드백입니다 ㅎㅎㅎ 영어, 한글, 숫자까지 테스트 해보다니요! 😍👍🏻👍🏻


h1 {
margin-bottom: 5px;
}

.explain {
font-size: 18px;
font-weight: bold;
margin-top: 20px;
}

input[type="checkbox"] {
appearance: none;
width: 20px;
height: 20px;
border-radius: 50px;
border: 2px;
background-color: white;
position: relative;
cursor: pointer;
box-shadow: 0px 0px 10px rgba(255, 0, 128, 0.3);
}

/* 체크박스 선택 시 */
input[type="checkbox"]:checked {
background-color: #ed1e85;
Copy link

Choose a reason for hiding this comment

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

색이 너무 예쁜 것 가타욤... 😍

border-color: #f34da0;
box-shadow: 0px 0px 10px rgba(255, 0, 128, 0.7);
}


input[type="checkbox"]:checked::before {
content: "";
position: absolute;
top: 50%;
left: 50%;
width: 10px;
height: 3px;
border-left: 2px solid white;
border-bottom: 2px solid white;
transform: translate(-50%, -50%) rotate(-45deg)
}

#todoPost{
margin-top: 20px;
margin-bottom: 15px;
position:flex;
flex-direction: row;
align-items: center;
justify-content: space-between;
}


button{
background: white;
color: black;
border: none;
padding: 10px 20px;
margin-left: 20px;
border-radius: 20px;
font-size: 15px;
font-weight: bold;
cursor: pointer;
}
Comment on lines +118 to +128
Copy link

Choose a reason for hiding this comment

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

image
텍스트 줄바꿈을 적용하면 텍스트 칸의 높이에 따라 삭제 버튼 글자가 줄바꿈 되면서 위아래로 늘어나는 현상이 발생합니다!! 줄바꿈을 방지하는 코드를 함께 넣어 주면 정돈된 UI가 될 것 같습니다😊

Suggested change
button{
background: white;
color: black;
border: none;
padding: 10px 20px;
margin-left: 20px;
border-radius: 20px;
font-size: 15px;
font-weight: bold;
cursor: pointer;
}
button{
background: white;
color: black;
border: none;
padding: 10px 20px;
margin-left: 20px;
border-radius: 20px;
font-size: 15px;
font-weight: bold;
cursor: pointer;
white-space: nowrap;
}


#todoRecord{
width: 60%;
padding: 12px;
border-radius: 8px;
}