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주차] 최지원 미션 제출합니다. #3

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

jiwonnchoi
Copy link

@jiwonnchoi jiwonnchoi commented Sep 7, 2024

구현 화면 및 기능

배포 링크✏️

필수 요건

  • CSS의 Flexbox를 이용한 레이아웃 구성
  • Todo 추가, 완료 시 체크하여 Did 목록으로 이동, 각 리스트에서 삭제
  • 오늘 날짜 및 현재 시각, Todo 개수 표기
  • Semantic tag를 활용한 HTML 구조

선택 요건

  • localStorage를 이용하여 기존 데이터 불러오기
  • 반응형 적용
  • 입력창 토글 기능
  • 입력창 슬라이드 효과 애니메이션

💭 느낀 점

리액트만 쓰다가 바닐라js를 오랜만에 써보면서 작은 기능 하나에도 비교적 손이 많이 간다는 것을 느꼈고, key question과 함께 생각해보니 주의하여 써야 할 js 개념이 많다는 것을 깨달았습니다.
DOM구조를 고려하여 parentNode에 접근하는데 헤매기도 하였고, 함수의 선언 위치 때문에 코드 실행에서 문제가 생기는데서 어려움을 겪기도 하였습니다. 값이 전달되지 않은 곳을 파악하는데도 여러 번의 시행착오를 거치면서 리액트의 상태관리를 통해 데이터 흐름을 직관적으로 파악하는 것의 편리함을 느꼈습니다.
또한 과제를 하는 내내 중복되는 코드의 효율성에 대한 고민을 많이 한 것 같습니다. html에서는 최대한 깔끔하고 군더더기 없는 구조를, css와 js에서는 공통된 기능은 함수화 해보았습니다. 그럼에도 가독성 혹은 효율성이 떨어진다고 느끼는 부분이 있다면 적극적인 코드리뷰 부탁드리겠습니다. 감사합니다!

🤔 아쉬운 점

  1. 리스트 전체 개수 중에서 한 일의 성취도를 도넛 그래프로 나타내보고 싶어서 css만으로 그리기를 찾아보았으나.. 너무 복잡하여 조금 시도한 끝에 결국 되돌렸습니다.😭라이브러리의 힘을 다시금 느꼈습니다.
  2. 하단의 Total 개수와 Accomplishment 값에서 한 번은 체크 표시한 경우에 동일해야 할 Total 값이 줄어드는 것을 발견했는데, 몇 번 더 재시도해도 해당 현상이 나타나지 않아 무엇이 문제였는지 파악하지 못한 채 문제가 사라진 적이 있었습니다. 관련 함수를 검토하면서 로직이 올바른 것을 확인하고는 넘어갔는데, 혹시 그러한 문제를 다시 발견하신다면 알려주시면 감사하겠습니다.
  3. 중간 중간에 트러블 슈팅과 참고한 자료를 많이 기록해두고 싶었는데 코딩에 집중하다 보니 끝나고 나서는 기억이 많이 휘발되는 것 같습니다.... 다음에는 꼭 남겨두는 습관을 들여서 더 많이 공유할 수 있도록 해야 할 것 같습니다.

🔑 Key Questions

DOM은 무엇인가요?

Document Object Model. XML, HTML 문서의 각 항목을 계층으로 표현하여 생성/변형/삭제를 돕는 인터페이스이다.

브라우저는 서버로부터 리소스들을 받아 렌더링한다. 이때 HTML 소스는 텍스트에 불과하므로 파싱을 통해 브라우저가 이해하는 자료구조 형태로 만든다.
파싱이란 텍스트 문서의 문자열을 토큰으로 분해하고 문법과 구조를 반영해 Tree를 생성하는 과정이다. 이에 따라 HTML이 파싱을 거치면 DOM tree가 된다.

DOM을 구성하는 노드 객체는 종류가 있고 상속 구조를 가지는데, 이러한 노드 타입 중 중요한 네 가지로는 문서노드, 요소 노드, 어트리뷰트 노드, 텍스트 노드가 있다. 상속 구조는 다음과 같다.

JavaScript가 DOM을 조작하는 메서드는 크게 CRUD로 나뉜다.

  • CREATE: createElement
  • READ: querySelector, querySelectorAll, getElementById, getElementByClassName
  • UPDATE: textContent, id, classList, setAttribute
  • DELETE: remove, innerHTML=“”, textContent=””
  • cf) APPEND: appendChild (생성한 노드를 HTML 요소에 연결)

이벤트 흐름 제어(버블링 & 캡처링)이 무엇인가요?

한 HTML 요소에 이벤트가 발생했을 때 HTML의 계층적 구조로 인해 연쇄적 이벤트 흐름이 일어난다. 이 현상을 이벤트 전파(Event Propagation)이라 부르고, 전파 방향에 따라 버블링과 캡처링으로 구분한다.

  • 버블링: 자식 요소에서 발생한 이벤트가 상위 부모 요소로 전파되는 현상
  • 캡처링: 자식 요소에서 발생한 이벤트가 부모 요소부터 시작하여 하위 자식 요소까지 도달하는 현상

버블링은 타킷부터 시작해 document 객체를 만날 때까지 핸들러가 연달이 모두 호출되는데, 원하는 타깃에서만 이벤트를 발생하게 하고 싶을 경우 event.stopPropagation() 을 통해 전파를 막을 수 있다.

event.preventDefault()란?
태그는 form 요소의 값을 전송하면서 해당 페이지를 새로고침 하는 기능을 가지고 있는데 페이지가 새로고침됨을 방지하여 입력값을 처리할 수 있도록 한다. 만약 event.preventDefault() 가 없다면 페이지가 새로고침 되며 입력한 데이터도 서버로 전송되지 않기 때문에 입력창은 초기화되면서 화면에 아무 변화가 없게 된다.


preventDefaultstopPropagation의 차이?
preventDefault 는 태그의 기본 이벤트 발생을 막는 메서드로, submit 태그의 새로고침이나 a태그로 링크 클릭 시의 이동을 막을 때 사용되고, stopPropagation 은 중첩된 html 요소 간에 이벤트 처리를 특정 요소에서만으로 한정할 때 쓰인다.


클로저와 스코프가 무엇인가요?

스코프란 식별자가 유효한 범위로, 전역과 지역 스코프로 나뉜다. 함수의 중첩에 따라 스코프도 계층적 구조를 가지며 이렇게 연결된 것을 스코프 체인이라 한다.

  • 모든 코드 블록(if, for, while, try/catch)은 지역 스코프를 만드는데, 이를 블록 레벨 스코프라 한다. 하지만 var 키워드가 인정하는 지역 스코프는 함수에 의해서 생성된 것만이고, 이를 함수 레벨 스코프라고 한다.

  • 렉시컬 스코프(정적 스코프)란 자바스크립트 엔진이 함수를 어디서 호출했는지가 아니라 함수를 어디에 정의했는지에 따라 상위 스코프를 결정하는 것을 말한다.

자바스크립트의 모든 함수는 자신의 상위 스코프를 기억한다. 클로저는 이러한 렉시컬 스코프를 기억하여 함수가 스코프 밖에서 실행될 때에도 이 스코프에 접근할 수 있게 하는 것이다. 즉, 외부함수보다 중첩함수가 더 오래 유지되는 경우 중첩함수는 이미 생명 주기가 종료한 외부 함수의 변수를 참조할 수 있다.

실행 컨텍스트와 렉시컬 환경의 개념을 구분하여, 함수가 종료하면 실행 컨텍스트가 스택에서 제거되지만 해당 함수의 렉시컬 환경은 여전히 중첩 함수의 내부 슬롯에 의해 참조된다.

클로저의 사용 목적?
상태가 의도치 않게 변경되지 않도록 안전하게 은닉하고 특정 함수에게만 상태 변경을 허용하여 상태를 안전하게 변경하고 유지하기 위해 사용한다. (캡슐화)


📜 참고한 자료

이벤트 버블링, 캡처링
toLocaleString 포맷 적용

Copy link
Member

@ddhelop ddhelop left a comment

Choose a reason for hiding this comment

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

멋진 과제 너무 잘 봤습니다🤩 코드에 주석처리가 되어있어서 코드리뷰하기 정말 편했던 것 같아요.

HTML 구조와 특히 JS DOM을 이용한 코드들이 인상적이였어요! 그래도 과제 취지에 맞게 열심히 해주신 흔적이 보여서 리뷰하면서 그 흔적들을 많이 볼 수 있어서 좋았어요.

첫번째 과제하느라 고생하셨습니다~!🙂‍↕️

Copy link
Member

Choose a reason for hiding this comment

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

아이콘과 로고 같은 경우에는 png, jpeg 보다는 SVG 형식을 쓰는게 좋아요! SVG 형식은 벡터 기반이기 때문에 크기가 조절되어도 이미지 품질이 손상되지 않기 때문에 유연한 크기 조절이 가능한 SVG 형식이 아이콘과 로고 같은 작은 이미지에 적합합니다 :)

image

Copy link
Author

Choose a reason for hiding this comment

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

사실 delete 아이콘이 같은 로직을 통해 생성되는데도 어떤 건 약간 가로로 길쭉한 .. 현상이 있었어서 width, height, 그걸 감싸는 별도의 태그 추가 등 크기 관련 코드만 여러 번 고쳐보았었는데 svg로 바꿔보아야겠다는 생각은 못했네요..! 첨부해주신 표 덕분에 정확히 알아가서 쓸 수 있을 것 같아요☺️

today.innerHTML = now.toLocaleString("ko-KR", options);
};

setInterval(updateTime, 1000); // 1초마다 호출
Copy link
Member

Choose a reason for hiding this comment

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

호출 시 즉시 실행시키면 첫 1초를 기다리지 않아서 이런식으로 구성해도 좋을 것 같습니다.

Suggested change
setInterval(updateTime, 1000); // 1초마다 호출
updateTime();
setInterval(updateTime, 1000);

Copy link
Author

Choose a reason for hiding this comment

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

단순 로딩 문제라 생각했는데 앞으로는 쓰려는 함수의 작동을 정확히 이해하고 써야할 것 같습니다😂 감사합니다!

Comment on lines +35 to +36
let todos = loadFromLocalStorage("todos"); // 할 일 배열
let dones = loadFromLocalStorage("dones"); // 한 일 배열
Copy link
Member

Choose a reason for hiding this comment

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

todos와 dones는 배열을 변경할 필요가 없으므로, const를 사용해도 좋을 것같습니다! let은 값이 재할당되거나 변동될 가능성이 있는 경우에만 사용하는 것이 좋다고 합니다. :)

Copy link
Author

Choose a reason for hiding this comment

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

항목을 삭제하거나 옮기는 deleteTodoItem, todoToDid 등의 함수에서 값을 재할당하는 부분이 있어서 let을 사용하였습니다!

사실 코드 쓸 때는 배열이 계속해서 변동된다는 생각에 곧바로 let을 사용하고선 넘어갔는데, 주신 리뷰보고서 한 번 더 디버깅 해보면서 정확히 어디까진 동작하고 어느 지점부터 재할당으로 인해 반영이 안되는지 파악해볼 수 있었습니다.
또 push()도 배열을 바꾼다고 생각하여 let을 써야겠다는 생각에 한 몫하였는데, 다시 찾아보니 배열을 선언한 것은 포인터이므로 pop(), push()를 통한 접근은 객체가 저장된 공간이 아닌 그 안의 내용이기 때문에 const로 선언해도 무관하다는 내용을 정확히 정리하고 넘어갈 수 있었습니다.

script.js Outdated
Comment on lines 78 to 83
// 체크 버튼 생성하기
const checkBtn = createBtn(
type === "todo" ? "images/empty_checkbox.png" : "images/full_checkbox.png",
"todo-check",
type === "todo" ? todoToDid : didToTodo
);
Copy link
Member

Choose a reason for hiding this comment

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

조건문으로 printItem에서 type에 따라 다른 이미지를 사용하는 구성이 정말 깔끔한 로직인 것 같습니다!

script.js Outdated
// 할 일 추가하기 함수
const addTodoItem = (event) => {
event.preventDefault();
const todoInput = document.querySelector(".input").value.trim(); // 입력값 공백 확인
Copy link
Member

Choose a reason for hiding this comment

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

현재 addTodoItem 함수가 실행될때마다 DOM 접근이 되고 있는데, 이렇게 호출하여 추가하는 방법도 좋지만, 조금 더 디테일을 챙기자면 자주 사용되는 DOM 요소는 한번만 선택하고 캐싱해서 사용해 주시면 불필요한 DOM 접근을 줄일 수 있습니다! :)

Suggested change
const todoInput = document.querySelector(".input").value.trim(); // 입력값 공백 확인
const inputElement = document.querySelector(".input");
const todoListElement = document.querySelector(".todoList");
// 이후 해당 변수를 사용

Copy link
Author

Choose a reason for hiding this comment

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

제 코드만 쓸 때는 간과하고 있었는데 다른 코드를 리뷰해보고나니 확실히 보이는 부분이네요 !! 코드를 짤 때도 반복이 되고 있는지를 의식적으로 확인하도록 하겠습니다!


const deleteTodoItem = (e) => {
todos = deleteItem(e, ".todo-text", todos, "todos", ".todoList");
updateCounts();
Copy link
Member

Choose a reason for hiding this comment

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

updateCounts가 deleteItem 함수 내부에서 이미 호출되고 있으므로, todoToDid 및 didToTodo 함수에서 중복 호출되는 것 같아요. 이러한 중복 호출은 제거해도 괜찮을 것 같습니다 :)

const showMessage = document.querySelector(".show-input"); // 입력창 열고 닫는 버튼 요소

// 애니메이션 적용한 입력창 열고 닫기 함수
const toggleForm = () => {
Copy link
Member

Choose a reason for hiding this comment

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

이런식으로 스타일을 통해서 관리하는 것도 좋지만 입력창의 상태(열림/닫힘)를 명확하게 추적하기 위해 boolean 변수로 상태를 관리하여 여닫는것도 유지보수하기엔 더 편하니까 한번 사용해봐도 좋을 것 같아요 :)_

Copy link
Author

Choose a reason for hiding this comment

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

클래스명은 워낙 지정하기 나름이라 혼동할 수도 있고, html/css에서 변경될 가능성도 있어 확실히 클래스 변경에 의존하는 것보다는 말씀해주신 것이 훨씬 직관적이고 혼란을 줄일 수 있겠다는 생각이 듭니다! 좋은 의견 주셔서 감사합니다☺️

style.css Outdated
}

.input-box.show {
display: flex;
Copy link
Member

Choose a reason for hiding this comment

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

.input-box에서 display: flex를 설정한 후, .input-box.show에도 다시 display: flex를 설정되고 있어요. 이부분은 없애도 좋을 것 같아요

Suggested change
display: flex;

display: flex;
align-items: center;
margin-bottom: 0.938rem;
box-sizing: border-box;
Copy link
Member

Choose a reason for hiding this comment

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

자식 요소들에서 box-sizing: border-box를 사용하는 것도 좋지만, 전역적으로 모든 요소에 box-sizing을 설정하는 것이 더 일관성 있는 레이아웃을 유지할 수 있어서 설정하고 스타일링 하는 것도 좋은 방법이예요

Suggested change
box-sizing: border-box;
* {
box-sizing: border-box;
}

style.css Outdated
Comment on lines 1 to 4
body {
display: flex;
justify-content: center;
}
Copy link
Member

Choose a reason for hiding this comment

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

폰트를 전역적으로 폰트 패밀리를 적용하여 일관성을 유지하는 것도 코드 중독을 방지할 수 있습니다!

Suggested change
body {
display: flex;
justify-content: center;
}
body {
font-family: "Pretendard"
display: flex;
justify-content: center;
}

Copy link
Author

Choose a reason for hiding this comment

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

이번 스타일 코드를 쓰면서 저도 은연중에 무언가 통일감이 없다는 듯한 생각이 들어 공용 클래스로 스타일도 써보는 등의 코드 정리를 계속 했었는데 다음에는 전역적인 스타일링 또한 신경을 써보아야겠습니다.
여러 가지 측면에서 세세하게 리뷰해주셔서 정말 감사합니다. 많은 배움 얻어갑니다!!🥹👍

Comment on lines +50 to +53

countElement.innerText = totalCount;
accomplishmentElement.innerText =
totalCount > 0 ? `${doneCount}/${totalCount}` : "0/0";
Copy link

Choose a reason for hiding this comment

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

삼항조건문으로 count 처리를 하는 게 인상깊습니다! 저도 최근에 알게 된 사실인데 innerText와 textContent로 text를 넣는 차이가 있다는 것을 알았습니다.
innerText -> display에 띄워진 요소만 레이아웃 계산 후 렌더링해서 가져옴(display:none 되어있는 텍스트는 가져오지 않음)
textContent -> 모든 텍스트를 전부 가져옴
그렇기에 특별한 css가 적용되지 않았을 때 textContent가 더 빠를 수 있다고 합니다! 한번 참고해보셔도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

innerHTML과 innerText만 알고 있었는데 말씀해주신 각 개념과 차이점까지 분명하게 정리하고 넘어갑니다👍 감사합니다!

script.js Outdated
Comment on lines 79 to 83
const checkBtn = createBtn(
type === "todo" ? "images/empty_checkbox.png" : "images/full_checkbox.png",
"todo-check",
type === "todo" ? todoToDid : didToTodo
);
Copy link

Choose a reason for hiding this comment

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

check box 디자인을 위해 사진을 활용하는 방법은 생각해보지 못 했던 것 같습니다. 새로운 배움주셔 감사합니다!

Comment on lines +35 to +36
let todos = loadFromLocalStorage("todos"); // 할 일 배열
let dones = loadFromLocalStorage("dones"); // 한 일 배열
Copy link

Choose a reason for hiding this comment

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

로컬스토리지에 key값을 두 개로 분리하여 저장하는 방법을 배워갑니다! 호기심에 다른 방법은 없을까 찾아보다
let tasks = loadFromLocalStorage("tasks") || { todos: [], dones: [] }; // 초기화
로 하나의 키값에 배열만 따로 두어 관리하는 방법도 있음을 알게 되었습니다. 한번 참고해보셔도 좋을 것 같습니다

script.js Outdated
// 할 일 추가하기 함수
const addTodoItem = (event) => {
event.preventDefault();
const todoInput = document.querySelector(".input").value.trim(); // 입력값 공백 확인
Copy link

Choose a reason for hiding this comment

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

입력값이 공백일 때의 제한까지 고려하신 점 좋은 것 같아요. 저는 미처 신경쓰지 못한 부분이라 지원님 코드를 본 이후로 더 세심하게 짚고 넘어갈 수 있을 것 같습니다

script.js Outdated
Comment on lines 116 to 124
const deleteItem = (e, classSelector, array, key, listSelector) => {
const target = e.target.closest("li");
const text = target.querySelector(classSelector).innerText;
array = array.filter((item) => item !== text);
saveToLocalStorage(key, array);
document.querySelector(listSelector).removeChild(target);
updateCounts();
return array;
};
Copy link

Choose a reason for hiding this comment

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

코드리뷰를 하며 e.target.closest이라는 메서드가 있는지 처음 알게 되었습니다. 삭제 로직이 text로 필터링하여 겹치는 text에 해당하는 item을 삭제하는 로직으로 이해했습니다. 혹여나 text가 겹칠 수 있는 부분도 있고, 함수가 받는 인자가 복잡할 수 있는 부분도 있을 것 같아 고유한 id를 item마다 생성하여 html5 data 속성을 이용하여 고유한 Item을 필터링하는 로직도 참고하시면 좋을 것 같습니다!

li.dataset.id = id; // DOM 요소에 data-id='id'

https://inpa.tistory.com/entry/JS-%F0%9F%93%9A-HTML-%EB%8D%B0%EC%9D%B4%ED%84%B0%EC%85%8Bdata-%EC%86%8D%EC%84%B1

script.js Outdated
Comment on lines 168 to 182
form.addEventListener(
"animationend",
() => {
form.style.display = "none";
form.classList.remove("hide");
},
{ once: true }
);
showMessage.innerHTML = "입력창 불러오기";
} else {
form.style.display = "flex";
form.classList.remove("hide");
form.classList.add("show");

showMessage.innerHTML = "입력창 다시닫기";
Copy link

Choose a reason for hiding this comment

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

animationend라는 이벤트 리스너 타입을 활용하여 UI를 신경 쓴 부분이 인상깊습니다. 저는 처음 본 방법이라 신기하고, JS로 어느정도 수준의 UI까지 활용가능할 수 있는지 호기심이 생겼습니다.

showMessage.addEventListener("click", toggleForm);
};

init();
Copy link

Choose a reason for hiding this comment

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

window.onload / document.addEventListener("DOMContentLoaded" / 이벤트 리스너 등록없이 함수 실행

이라는 렌더링 방식에 있어 각각의 사용목적과 장단점이 있음을 이번에 알게 되었습니다. 링크 한번 달겠습니다! 한번 참고하셔도 좋을 것 같습니다. (링크를 줄이는 방법을 몰라 링크가 긴 점 양해부탁드립니다..)

https://mesonia.tistory.com/entry/%ED%8E%98%EC%9D%B4%EC%A7%80-%EB%A1%9C%EB%93%9C-%ED%9B%84-%EC%8B%A4%ED%96%89%ED%95%98%EA%B8%B0-windowonload-documentready-documentready%EB%A5%BC-%EC%88%9C%EC%88%98%EC%9E%90%EB%B0%94%EC%8A%A4%ED%81%AC%EB%A6%BD%ED%8A%B8%EB%A1%9C-DOMContentLoaded


.container {
width: 100%;
max-width: 37.5rem;
Copy link

Choose a reason for hiding this comment

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

rem단위로 css를 사용하시는군요! 혹시 픽셀이나 다른 단위가 아닌 rem으로 했을 때의 장단점이나 이유가 있는지 여쭤보고 싶습니다!

Copy link
Author

@jiwonnchoi jiwonnchoi Sep 8, 2024

Choose a reason for hiding this comment

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

px은 고정된 절대 단위이고, rem은 비율에 맞춰 바뀌는 상대 단위라 반응형에 더 적합하다고 하더라고요! 정확히는 브라우저의 루트 글꼴의 크기설정이 변경될 때 유연하게 반응하는 것이 rem 단위라고 합니다. rem에 대해 정확히 이해할 수 있는 링크가 있어 함께 첨부드려요!
px vs rem
rem이란

index.html Outdated
<link rel="stylesheet" href="style.css" />
<link rel="icon" href="images/full_checkbox.png" />
Copy link

Choose a reason for hiding this comment

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

저도 상단 제목 옆에 예쁜 아이콘 넣고 싶어서 했다가 실패해서 아쉬웠었는데 방법 배워갑니다!

Copy link
Author

Choose a reason for hiding this comment

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

소소한 포인트였는데 발견해주시다니 기쁩니다..😄

사실 파비콘으로는 ico 파일이 많이 쓰인다고 알고 있는데, 제가 가진 png는 ico 변환기를 통해 사용해보니 깨지는 것 같더라구요. 그래도 ico 파일이 여러 사이즈를 저장하고 있어서 더 적절하다고 알고 있습니다!
관련 링크 첨부드리며 파비콘 만들기로 검색해보셔도 많은 정보 얻으실 수 있을 것 같아요!

Copy link
Collaborator

@jinnyleeis jinnyleeis left a comment

Choose a reason for hiding this comment

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

비주얼적인 측면, 코드적인 측면에서 정말 과제 잘 마무리해주신 것 같아 인상깊게 봤습니다!

애니메이션까지 적용하시고, 애니메이션이 잘 작동하게 하기 위해, addeventlistener의 { once: true } 옵션까지 사용하신 부분 인상깊었습니다.

또한, 로컬 스토리지에 저장/삭제 처리 로직에서,
완료 상태의 할 일과, 미완료 상태의 할 일 로직을 중복으로 구현하시지 않고
saveToLocalStorage, loadFromLocalStorage를 통해 재사용가능하게 구현하신 부분도 정말 좋은 것 같아요!

이벤트 리스너가 여러차례 추가되는 부분, 네이밍만 조금 수정하면 더욱 완벽한 코드가 될 수 있을 것 같아요!! 이번 과제 정말 수고 많으셨어요 지원님!!!

script.js Outdated
form.classList.remove("show");
form.classList.add("hide");

form.addEventListener(
Copy link
Collaborator

Choose a reason for hiding this comment

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

toggleForm 함수가 실행될 때마다 animationend 이벤트 핸들러가 새로 추가되도록 코드가 작성되어있는 부분을 수정하면 좋을 것 같아요!

ddEventListener()의 3번째 파라미터로 { once : true } 전달해 주어서, 이벤트가 발생할 때 한 번만 동작하게 되긴 하지만,
toggleForm이 다시 실행될 때마다 새로운 이벤트 리스너가 추가되어 한 번 실행 -> { once : true } 로 인해 삭제 -> 또 다시 토글 발생 -> 새로운 이벤트 리스너 추가 -> { once : true } 로 인해 삭제… 가 반복되는 불필요한 동작이 반복되는 것 같아요!!

때문에 이 방식보다 { once: true } 옵션을 쓰지 않고,
toggleForm 함수 외부에서 이벤트 리스너를 한 번만 등록하고
toggleForm 함수 내부에서는 클래스를 토글하는 동작만 수행하도록 로직을 분리해주는 것이 최적화와 유지보수 측면에서 더 깔끔할 것 같아요!

`
// 애니메이션 종료 후 처리하는 리스너 (한 번만 추가)
const handleAnimationEnd = (e) => {
if (form.classList.contains('hide')) {
form.style.display = 'none';
form.classList.remove('hide');
}
};

// 단순히 클래스만 토글하도록!!

const toggleForm = () => {
if (form.classList.contains('show')) {
form.classList.remove('show');
form.classList.add('hide');
showMessage.innerHTML = '입력창 불러오기';
} else {
form.style.display = 'flex';
form.classList.remove('hide');
form.classList.add('show');
showMessage.innerHTML = '입력창 다시닫기';
}
};

// 이벤트 리스너 초기에 1번만 등록!!
form.addEventListener('animationend', handleAnimationEnd);
`

Copy link
Author

Choose a reason for hiding this comment

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

수정방향 코드까지 너무 감사드립니다!! 처음에 클래스 토글까지 구현한 상태에서 애니메이션을 추가하여 한번에 쓰려하다 보니 불필요한 동작이 일어나게 되는 것까지 고려하지 못한 것 같습니다. ㅜㅜ 한 함수에서 담당하는 기능이 최소화되도록 분리하여 쓰는 습관을 길러야겠다는 생각이 많이 듭니다.

script.js Outdated
todos.forEach((todo) => printTodo(todo));
dones.forEach((done) => printDidItem(done));
todos.forEach((todo) => printItem(todo, "todo"));
dones.forEach((done) => printItem(done, "did"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

완료된 항목을 나타내기 위한 표현으로 did와 done이 혼용되어있어서 하나로 통일하는 것이 좋지 않을까 해요!!
자칫 다른 사람이 보기에, done과 did가 다른 역할을 수행한다는 착각을 불러일으킬 수 있기 때문에,
did-text -> completed-text , dones -> completed 이런식으로 모든 관련 함수, 변수, 클래스에서 완료된 항목을 의미하는 용어를 일관되게 수정하는 것을 제안드려요!!

script.js Outdated
};

// 할 일에서 한 일로 옮기기
// 할 일에서 한 일로 이동 함수
const todoToDid = (e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

함수 네이밍을 할 때, 함수 이름만으로 동작을 예측할 수 있도록 명확한 역할을 표현하는 이름을 사용하는 것이 좋다고 해요!
deleteTodoItem은 할일을 삭제한다는 의미가 명확하게 드러나는 함수명인 반면,
todoToDid, didToTodo는 to가 작업의 방향성을 나타내긴 하지만, 할 일 항목을 완료 상태로 변경, 완료된 항목을 다시 미완료 상태로 변경한다는 의미를 한번에 전달하기에는 약간 광범위한 표현이라는 생각이 들어요! (todo가 정확히 미완료 할일만을 가리키는거라기보단 할일 목록을 가리킬 수도 있어 표현이 약간 광범위해보임 )
때문에, todoToDid보다는 함수의 역할이 명확하게 드러나도록 completeTodo
didToTodo 보다는 restoreTodo 처럼 함수명을 수정해보는 것을 제안드려봅니다!

https://www.youtube.com/watch?v=emGLxi0LvNI&t=556s
명확한 네이밍을 통해 코드 가독성을 개선하는 것에 대한 카카오 FE개발팀 발표 영상 첨부해드려요!!
어떻게 변수명/함수명 등을 지어야 할지 기준을 세우는데 도움을 줄 수 있는 좋은 영상인 것 같아서 첨부합니다~~
11분 18초 '2. 보다 구체적인 단어로 바꾸기' 섹션부터 참고하시면 좋을 것 같아요!(앞부분도 유익해서 시간되시면 다 보시는것도 추천드려용ㅎㅎ )

또한, 보통 did는 동작을 가리키는 반면에 done은 상태를 가리키는 표현이기 때문에, did보단 done을 사용하는 것이 적절해보여요!!

Copy link
Author

Choose a reason for hiding this comment

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

네이밍은 매번 고민하던 부분이었는데 이번을 계기로 좀더 명확한 기준을 확립해보겠습니다! 영상 추천해주셔서 감사합니다 ☺️

script.js Outdated
img.setAttribute("src", src);
btn.appendChild(img);
btn.setAttribute("class", className);
btn.addEventListener("click", clickHandler);
Copy link
Collaborator

@jinnyleeis jinnyleeis Sep 8, 2024

Choose a reason for hiding this comment

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

각 버튼에 대해 개별적으로 이벤트 리스너를 추가하는 방식으로 작성해주셨는데,
버튼마다 하나하나 이벤트 리스너를 등록하기 보단, 이번에 키 퀘스천 주제였던 '이벤트 버블링'을 활용해 보는 것이 어떨까 제안드려요!
이벤트 버블링은 아시다시피 하위 요소에서 발생한 이벤트가 상위 요소로 전파되는 것이므로,
이를 통해 상위 요소가 하위 요소들의 이벤트를 한 번에 처리할 수 있어요.

때문에, 상위 요소에 이벤트 리스너를 등록하고, 실제 클릭된 하위 요소(버튼)는 event.target을 통해 받아와서 적절한 로직을 취하는 것을 제안드려요!

이 방식을 취하면, 버튼 클릭 처리 로직과 / 버튼 생성 createBtn , 항목 출력하기 printItem 로직을 분리할 수 있어서, 나중에 유지보수할때도 좋고, 코드의 가독성도 좋아질 것 같아요.

먼저 .todoList, .didList에 한 번만 이벤트 리스너를 등록한 후에,
didToTodo 함수에서 closest() 메서드를 활용해주신 것처럼, closest() 메서드를 통해 클릭된 하위 요소가 체크 버튼인지 삭제 버튼인지 확인 후에 이에 따른 적절한 완료처리/삭제처리 로직을 실행해주는 것이 좋을 것 같아요!

`수정 코드 첨부합니다!!
// 이벤트 위임을 통한 버튼 클릭 처리
document.querySelector('.todoList').addEventListener('click', (e) => {
if (e.target.closest('.todo-check')) {
todoToDid(e);
} else if (e.target.closest('.todo-del')) {
deleteTodoItem(e);
}
});

document.querySelector('.didList').addEventListener('click', (e) => {
if (e.target.closest('.todo-check')) {
didToTodo(e);
} else if (e.target.closest('.todo-del')) {
deleteDidItem(e);
}
});

// 버튼 생성하기 함수 (이벤트 리스너 없이 생성)
const createBtn = (src, className) => {
const btn = document.createElement("button");
const img = document.createElement("img");
img.setAttribute("src", src);
btn.appendChild(img);
btn.setAttribute("class", className);
return btn;
};

// 항목 출력하기 함수 (버튼에 이벤트 리스너 추가x)
const printItem = (text, type) => {
const list = document.querySelector(.${type}List);
const item = document.createElement("li");
const itemContent = document.createElement("div");
const itemText = document.createElement("span");
itemText.innerText = text;
itemText.className = ${type}-text;

// 체크 버튼 생성
const checkBtn = createBtn(
type === "todo" ? "images/empty_checkbox.png" : "images/full_checkbox.png",
"todo-check"
);

// 삭제 버튼 생성
const deleteBtn = createBtn("images/delete_btn.png", "todo-del");

itemContent.className = "todo-item";
itemContent.appendChild(checkBtn);
itemContent.appendChild(itemText);
itemContent.appendChild(deleteBtn);

item.appendChild(itemContent);
list.appendChild(item);
};
`

Copy link
Author

@jiwonnchoi jiwonnchoi Sep 9, 2024

Choose a reason for hiding this comment

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

감사합니다. 수정코드 방향으로 고쳐보며 다시 한번 이벤트 위임의 개념을 다져봐야 할 것 같습니다🥹 디테일하게 리뷰해주셔서 더 많이 고민해보는 계기도 되었고 정말 많이 배워갑니다🙇‍♀️!!

style.css Outdated
}

.submitBtn {
color: #85b6ff;
Copy link
Collaborator

@jinnyleeis jinnyleeis Sep 8, 2024

Choose a reason for hiding this comment

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

#85b6ff,#458fff 이 두 색상을 계속해서 사용해주셨는데,
재사용되는 색상을 CSS 전역 변수로 정의해서 이용하는 방법을 제안드려요!

공식 문서에 나와있듯이, :root 클래스에 선언해서 HTML 문서 전체에서 이를 활용할 수 있도록 할 수 있으니 한 번 참고해보시면 좋을 것 같습니다!
https://developer.mozilla.org/ko/docs/Web/CSS/Using_CSS_custom_properties

:root {
--primary-color: #85b6ff;
--secondary-color: #458fff;
}

...
.today {
color: var(--primary-color);
font-family: "Pretendard";
font-weight: 300;
font-size: 0.938rem;
}
...

<section class="did">
<h2 class="ment-style">What I did</h2>
<ul class="didList"></ul>
</section>
</main>
<footer>
Copy link
Collaborator

Choose a reason for hiding this comment

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

footer main 등등 시맨틱 태그들을 적절히 잘 활용해주셨네요!! 문서의 구조를 보다 더 명확하게 파악할 수 있는 것 같습니다 ㅎㅎ

@jsomnium
Copy link

jsomnium commented Sep 9, 2024

지원님 잘 봤습니다! 과제 하느라 고생 많으셨어요. 😄
투두리스트 디자인도 그렇고, 깔끔하게 잘 구현하셨네요.
코드 깔끔하게 잘 작성하시는 것 같아서 부럽습니다. 과제 고생많으셨습니다!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants