[1주차] 신수진 미션 제출합니다. #7
Conversation
|
vercel 배포에서 접근 권한요청이 나옵니다! 해당 프로젝트 배포 settings에 들어가셔서 private -> public으로 바꾸면 됩니당 |
배포링크 수정했습니다! 감사합니당~ |
| #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; | ||
| } |
There was a problem hiding this comment.
입력창 텍스트가 너무 길어지면 줄바꿈이 되지 않고 칸을 뚫고 나가는 현상이 있습니다! 저는 한글이나 영어만 테스트해 보았다가 제출 후 숫자는 줄바꿈이 안 되어서 찾아보았는데 word-break: break-all을 활용하면 모든 문자(숫자 포함)가 공백 상관 없이 줄바꿈이 잘 적용되더라구요!!
| #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; | |
| } |
There was a problem hiding this comment.
너무 좋은 피드백입니다 ㅎㅎㅎ 영어, 한글, 숫자까지 테스트 해보다니요! 😍👍🏻👍🏻
| button{ | ||
| background: white; | ||
| color: black; | ||
| border: none; | ||
| padding: 10px 20px; | ||
| margin-left: 20px; | ||
| border-radius: 20px; | ||
| font-size: 15px; | ||
| font-weight: bold; | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
텍스트 줄바꿈을 적용하면 텍스트 칸의 높이에 따라 삭제 버튼 글자가 줄바꿈 되면서 위아래로 늘어나는 현상이 발생합니다!! 줄바꿈을 방지하는 코드를 함께 넣어 주면 정돈된 UI가 될 것 같습니다😊
| 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; | |
| } |
| const text = input.value.trim(); //양 끝 공백 제거 후 text에 저장 | ||
|
|
||
| if (text !== "") { | ||
| //text기준으로 로컬 스토리지에 삭제되므로, 같은 text면 등록 방지 |
There was a problem hiding this comment.
text 기준으로 변경된 부분만 로컬 스토리지를 업데이트하는 방식이 효율적인 것 같습니다! 😊👍
저는 기존에 로컬 스토리지에서 데이터 변경이 발생할 때마다 전체를 삭제하고 다시 저장하는 방식을 사용했는데, 이 방식이 불필요한 데이터 처리 과정이 많아 비효율적인 것 같아요. 참고해서 코드 최적화에 대해 다시 고민해 봐야 할 것 같습니당
| 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; | ||
| } |
There was a problem hiding this comment.
사소하지만 개인적으로 let todos = JSON.parse(localStorage.getItem("todos")) || [];가 중복으로 호출되고 있는데, 두 함수는 함께 쓰이고 있으니 하나로 합치고 호출도 한 번으로 줄이면 좋을 것 같아요!
| 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; | |
| } |
| <span> | ||
| <input id="todoRecord" type="text" placeholder="할 일 추가" /> | ||
| </span> |
There was a problem hiding this comment.
input을 span으로 감싸신 이유가 따로 있을까요?
input이랑 button을 줄바꿈하지 않고 한줄로 만들어보고자 사용했던 것인데 보니까 필요가 없는 태그 같네요ㅠㅠ
| <div id="header"> | ||
| <h1>Daily To Do List</h1> | ||
| <div class="explain"> | ||
| <p>오늘의 할 일을 기록해보세요</p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
전체적으로 html 파일 내에서 시맨틱 태그가 사용된 것 같지 않아서 div -> header 등 시맨틱 태그를 활용하여 접근성을 높이면 좋을 것 같습니다!
| getDoneCount(); | ||
| getTodoCount(); |
There was a problem hiding this comment.
이 두 개 함수가 계속해서 함께 사용되는 것 같아서 getAllTodoCount 등으로 함수를 만들어 묶어서 사용하면 더 좋을 것 같습니다!
There was a problem hiding this comment.
getDoneCount(),getTodoCount()에서 dom을 조작해서 get으로 시작하는 네이밍은 적절치 않아 보여요.
내부에서 localStorage 접근을 두 번 하게 돼서 서연님이 말씀해주신 것처럼 묶어서 사용하되, update로 시작하는 네이밍으로 수정하면 좀 더 명확할 것 같습니다!
|
|
||
| let todos = JSON.parse(localStorage.getItem("todos")) || []; | ||
| if (doneCount === todos.length) { | ||
| alert("축하합니다! 모든 할 일을 다하셨어요!"); |
There was a problem hiding this comment.
alert 창은 확인 버튼을 누르기 전까지 사용자의 동작을 막아 UX를 저해하기 때문에 별로 좋지 않다고 알고 있습니다. 대신 modal, toast 등을 사용하는 걸 추천드립니다!
| li.append(checkBox); | ||
| li.append(span); | ||
| li.append(deleteBtn); |
There was a problem hiding this comment.
li.append(checkBox, span, deleteBtn);으로 더 간단하게 작성할 수 있을 것 같아요!
| //체크 여부에 따라 스타일 변경 | ||
| if (checked) { | ||
| span.style.textDecoration = "line-through"; | ||
| span.style.color = "gray"; | ||
| }else{ | ||
| span.style.color="white" | ||
| } |
There was a problem hiding this comment.
체크 여부에 따라 className을 주고 css 파일에서 해당 class에 스타일을 지정해주면 더 깔끔할 것 같습니다!
| function getWeekDay() { | ||
| const week = ["일", "월", "화", "수", "목", "금", "토"]; | ||
| const weekDay = week[currentDate.getDay()]; | ||
| return weekDay; | ||
| } | ||
| const weekDay = getWeekDay(); //요일 함수 선언 | ||
| const yyyy_mm_dd = `${year}년 ${month}월 ${day}일 ${weekDay}요일`; | ||
| //오늘의 날짜 띄우기 | ||
| document.getElementById("todayDate").textContent = yyyy_mm_dd; |
There was a problem hiding this comment.
요일을 week 배열로 저장한 것 좋습니다~ 다만 toLocaleDateString 메서드를 사용하면 더 편하게 날짜를 가져올 수 있을 것 같습니다!
https://dev-thinking.tistory.com/15
| if (sameTodo(text)) { | ||
| alert("이미 같은 할 일이 존재합니다!"); | ||
| input.value = ""; //사용자 입력칸 빈칸 리셋 | ||
| input.focus(); //입력창에 포커스 | ||
| } else { | ||
| addToList(text, false); | ||
| saveLocalStorage(text, false); | ||
| input.value = ""; //사용자 입력칸 빈칸 리셋 | ||
| input.focus(); //입력창에 포커스 | ||
| } |
There was a problem hiding this comment.
이 부분 사용자 입력칸 빈칸 리셋과 입력창 포커스가 중복되어 아래처럼 작성하면 더 좋을 것 같네요!
if(text !== "") {
if(sameTodo(text)) {
alert("이미 같은 할 일이 존재합니다!");
} else {
addToList(text, false);
saveLocalStorage(text, false);
}
input.value = "";
input.focus();
} else {
...
}
| input.focus(); //입력창에 포커스 | ||
| } | ||
| } else { | ||
| alert("할 일을 작성해주세요!"); |
| @@ -0,0 +1,39 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
lang 속성을 한국어로 지정해주는게 더 좋지 않을까 싶습니다!
| @@ -0,0 +1,156 @@ | |||
| //현재 날짜와 시간을 가져오기 | |||
|
|
||
| let doneCount = 0; | ||
|
|
||
| function addToList(text, checked) { |
There was a problem hiding this comment.
요 addToList() 함수에 DOM 생성, 스타일링, 이벤트 바인딩, 상태 변경 로직까지 다 포함되어 있는 것 같아 꽤 무거워 보여요 😅😅
addToList() 역할 단위로 쪼개서 코드 가독성도 향상되고, 유지보수 및 확장성이 좋아지도록 책임을 좀 분리해 보면 어떨까요?!
| getTodoCount(); | ||
| getDoneCount(); | ||
|
|
||
| let todos = JSON.parse(localStorage.getItem("todos")) || []; |
There was a problem hiding this comment.
JSON.parse(localStorage.getItem("todos")) || [] -> 요 부분이 거의 모든 함수에서 반복되는데 getTodos() 등의 함수로 추출해서 사용하면 어떨까용? 😆😆 만약 키 이름이 바뀐다면 하나하나 수정해야 하는 번거로움이 발생할 수 있으니 함수에서 통합적으로 관리하는 거죠!
| function sameTodo(text) { | ||
| let todos = JSON.parse(localStorage.getItem("todos")) || []; | ||
| return todos.some((todo) => todo.text === text); | ||
| } |
There was a problem hiding this comment.
지금 script.js는 하나의 파일 안에 모든 기능(날짜 처리, DOM 조작, 이벤트 처리, 로컬스토리지 관리 등)이 전부 다 들어있어서 한눈에 구조 파악이 어려워요. 😢😢
유지보수 측면에서 모듈화를 제안 드리고 싶어요! script를 아예 기능별로 쪼개서 components / utils 같은 폴더에 나눠 관리하는 건 어떨까요?
그럼 구조가 직관적이게 되어 나중에 기능을 추가하더라도 어디를 어떻게 고쳐야 할지 명확해 질 것 같아요 ㅎㅎ
| if (text !== "") { | ||
| //text기준으로 로컬 스토리지에 삭제되므로, 같은 text면 등록 방지 | ||
| if (sameTodo(text)) { | ||
| alert("이미 같은 할 일이 존재합니다!"); |
There was a problem hiding this comment.
UX 측면에서 고민을 많이 하신 게 보입니다! 😍 다음주에 있을 과제에서 모달로 리팩토링 해 보면 어떨까요? ㅎㅎ
| #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; | ||
| } |
There was a problem hiding this comment.
너무 좋은 피드백입니다 ㅎㅎㅎ 영어, 한글, 숫자까지 테스트 해보다니요! 😍👍🏻👍🏻
|
|
||
| /* 체크박스 선택 시 */ | ||
| input[type="checkbox"]:checked { | ||
| background-color: #ed1e85; |
| deleteLocalStorage(text); | ||
| getTodoCount(); | ||
| getDoneCount(); | ||
| alert("삭제 완료"); |
There was a problem hiding this comment.
삭제 완료, 취소까지 알려주는 것 너무 친절한데!!!!!!! 사용자 입장으로 테스트 해 보니 삭제하시겠습니까? 하고 완료를 누른 뒤 alert 창이 한 번 더 떠서 탭으로 또 지워야 하는 게 아주 쪼금!! 불편하더라고요 😢😢 사용자 입장에서 흐름이 매끄러울 수 있도록 과감히 삭제 해 보시는 건 어떨까요?? (저도 과거 과제를 하면서 수진 님처럼 삭제 완료 alert까지 뜨게 했던 것 같은데 그때 들었던 피드백이 아직도 생생합니다...)
| <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> |
There was a problem hiding this comment.
id 네이밍이 kebab-case, camelCase 혼용되어 있는 데 가독성을 위해 맞춰주면 좋을 것 같아요
| return weekDay; | ||
| } | ||
| const weekDay = getWeekDay(); //요일 함수 선언 | ||
| const yyyy_mm_dd = `${year}년 ${month}월 ${day}일 ${weekDay}요일`; |
There was a problem hiding this comment.
변수명도 camelCase로 맞추는게 좋을 것 같아요 .추가로 변수의 의미를 좀 더 명확하게 formattedDate 같은 건 어떨까요 ?
| const weekDay = getWeekDay(); //요일 함수 선언 | ||
| const yyyy_mm_dd = `${year}년 ${month}월 ${day}일 ${weekDay}요일`; | ||
| //오늘의 날짜 띄우기 | ||
| document.getElementById("todayDate").textContent = yyyy_mm_dd; |
There was a problem hiding this comment.
처음에 날짜를 세팅하려는 의도는 파악이 되지만 초기화 해야하는 로직이 많을 땐 분리를 해보는 건 어떨까요 ?
| document.getElementById("todayDate").textContent = yyyy_mm_dd; | |
| const todayDateEl = document.getElementById("todayDate"); | |
| const init = () => { | |
| todayDateEl.textContent = yyyy_mm_dd; | |
| } |
| const input = document.querySelector("input"); | ||
| const text = input.value.trim(); //양 끝 공백 제거 후 text에 저장 |
There was a problem hiding this comment.
이번 과제에서는 input이 하나여도 상관없지만, 이왕 사용자로부터 입력받는 input에 id를 지정했으니까 id로 찾는건 어떨까요??
그리고 변수명도 한 눈에 봤을 때 어떠한 값을 갖고 있는 지 분명했으면 좋겠어요.
| const input = document.querySelector("input"); | |
| const text = input.value.trim(); //양 끝 공백 제거 후 text에 저장 | |
| const inputEl = document.querySelector("#todoRecord"); | |
| const userInput = inputEl.value.trim(); //양 끝 공백 제거 후 text에 저장 |
| getDoneCount(); | ||
| getTodoCount(); |
There was a problem hiding this comment.
getDoneCount(),getTodoCount()에서 dom을 조작해서 get으로 시작하는 네이밍은 적절치 않아 보여요.
내부에서 localStorage 접근을 두 번 하게 돼서 서연님이 말씀해주신 것처럼 묶어서 사용하되, update로 시작하는 네이밍으로 수정하면 좀 더 명확할 것 같습니다!
| getTodoCount(); | ||
| }); | ||
|
|
||
| let doneCount = 0; |
There was a problem hiding this comment.
함수들의 역할을 잘 부여해서 로직을 분리해보면 doneCount 변수가 굳이 필요 없을 것 같습니다
| let todos = JSON.parse(localStorage.getItem("todos")) || []; | ||
| let todoCount = todos.filter((todo) => !todo.checked).length; | ||
|
|
||
| document.querySelector("#todoCount").innerHTML = todoCount; |
There was a problem hiding this comment.
getTodoCount()가 실행할 때마다 dom에 접근해서 변수에 저장해두고 사용하면 좋을 것 같습니다.
| getTodoCount(); | ||
| } | ||
|
|
||
| function doneList(text, checked) { |
There was a problem hiding this comment.
checked 상태를 받아서 해결/미해결을 처리하는 데 doneList라는 함수명도 모호한 것 같습니다. updateTodoStatus 같은 네이밍은 어떨까요?
| } | ||
|
|
||
| function doneList(text, checked) { | ||
| let todos = JSON.parse(localStorage.getItem("todos")) || []; |
배포링크
https://vanilla-todo-21th-sujin.vercel.app/
느낀 점
자바스크립트 하나 만으로 상태 변화를 모두 구현한다는 점에서 코드가 굉장히 복잡해진다는 것을 느낄 수 있었던 과제였던 것 같습니다. 변수와 함수 선언 방식 및 선언 위치, 필요한 함수가 모두 다 있는지 확인해야 한다는 점이 많이 힘들었습니다. (KeyQuestion을 미리 공부하고 코드를 짰다면 많이 도움 됐을 것이란 생각도 듭니다.)
리액트를 사용하지 않고 순수 자바스크립트만으로 코드를 짜니 DOM이라던가, 이벤트 흐름 제어에 대해서 깊게 생각해보게 되면서 자바스크립트에 대한 이해도를 더 높일 수 있는 좋은 기회였습니다.
추가적으로 CSS 작성할 때 사용하는 태그를 무분별하게 class로 표시하기도 하고, id로 사용하기도 하며 이름을 아무렇게나 지었었는데, 이렇게 하니 코드에 대한 가독성이 떨어져서 class혹은 id를 작성하는 규칙이나 작명법에 대해서 더 생각해 봐야 할 것 같습니다.
Key Questions
1️⃣ DOM
Document Object Model의 약자로 자바스크립트에서 웹 페이지 요소를 선택, 수정, 추가 그리고 삭제할 때 사용되는 개념.
이번 과제에서 많이 사용한 메서드로는
document.getElementById(id),document.querySelector(selector),element.appendChild(childElement),element.addEventListener(event, function)가 있다.getElementById와 querySelector차이
getElementById는 id를 통해 엘리먼트를 반환하고, querySelector는 selector의 그룹과 일치하는 document 내 첫 번째 엘리먼트를 반환한다. 쿼리 셀렉터는
id,class,input[name=""]등 다양한 선택자를 사용할 수 있으며, 더 구체적이고 한정적인 엘리먼트를 선택할 수 있다. 또한, querySelectorAll로 해당하는 모든 엘리먼트를 얻어 변수에 할당할 수도 있어 활용도가 높기도 하다.2️⃣ 이벤트 흐름 제어(버블링&캡처링)
버블링
자식요소에서 발생한 이벤트가 바깥 부모 요소로 전파
캡처링
자식요소에서 발생한 이벤트가 부모 요소부터 시작하여 안쪽 자식 요소까지 전달
addEventListner() 함수로 요소 이벤트를 등록하면 버블링 전파 방식으로 이벤트가 흐르게 되어, 자식요소에게 이벤트가 발생하면 부모 요소도 버블링을 통해 이벤트가 전파되어 리스너가 호출된다.
event.stopPropagation()을 사용하면 버블링에 경우 상위요소로 이벤트 전파되는 것을 막을 수 있음.3️⃣ 클로저와 스코프
스코프
참조 대상 식별자를 찾아내기 위한 규칙으로 global scope, local scope, block scope, lexical scope가 있다.
전역 스코프
: 함수 바깥에서 선언된 변수는 어디에서나 접근 가능
지역 스코프
: 함수 내부에서 선언된 변수는 함수 내부에서만 접근 가능
블록 스코프:
: 블록{ } 내부에서 선언된 변수(let, const)는 블록내부에서만 유효
렉시컬 스코프
: 함수가 어디서 호출하는지가 아니라 어디에 선언하였는지에 따라 결정되는 것
클로저
함수가 속한 렉시컬 스코프를 기억하여 함수가 렉시컬 스코프 밖에서 실행될 때도 이 스코프에 접근할 수 있게 하는 기능
var, let, const 모두 호이스팅(끌어올려짐)이 가능하지만 var는 초기값이 undefined로 설정, let과 const는 선언 전에 접근하면 오류난다는 차이가 존재 → 일반적으로 let과 const를 사용하는 것이 안전