-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: main
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
todoInput.addEventListener("keydown", handleEnterKeyDown); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
투두를 한글을 입력하고 enter를 눌러서 처리하는 로직에서 enter가 중복으로 처리되는 경우가 있는 것 같습니당.
keydown 은 작동방식이 브라우저마다 차이가 있어 keypress 가 상대적으로 안전하다고 알고 있는데 이런 부분 참고 해보시면 좋을 것 같습니다! 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
허걱. 좋은 자료 감사합니다!! 사실 이전 프로젝트에서 첨부해 주신 블로그와 동일한 이슈가 있었는데 정확한 원인을 파악하지 못했었거든요 ㅠㅠ 덕분에 좋은 정보 알아갑니다!
There was a problem hiding this comment.
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
한글 입력 후 엔터 시 중복 이벤트 발생
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 방금 보고 후다닥 달려왔어요 MDN에서도 이제 권장하지 않는다네요 ㅜㅜ 위에 분 말씀 처럼 isComposing을 통해서 해결할 수 있다고 합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 아영님! 1주차 과제하시느라 수고 많으셨습니다🤍
직관적인 변수 및 함수 네이밍, 필요한 부분에 추가적으로 덧붙여주신 주석으로 인해 코드리뷰가 한결 편리했어요! 체크박스와 인풋 간의 관계에 label을 활용하여 구조적으로도 깔끔하게 구현하신 점이 인상깊네요👍
이번 바닐라 JS 과제의 핵심이라고도 할 수 있는 버블링과 이벤트 위임에 대해 코드적으로는 어떻게 적용할 수 있는지 고민해보며 좀 더 효율적인 이벤트 감지 작동을 고민해보셔도 좋을 것 같습니다. 또 선택 구현사항이었지만 로컬스토리지에 저장하게 되었을 때 항목들을 배열로 관리하는 로직에서도 또 고민해볼 지점들이 많이 생기기 때문에 코드리뷰를 진행하시면서 한 번 더 생각해보면 더욱 좋을 것 같습니다ㅎㅎ
빠르게 과제 마치시고 다른 pr에도 먼저 코드리뷰 남기는 적극성도 넘 멋져요 진짜 짱짱 최고입니당🥹🫶
deleteButtons.forEach((deletebutton) => | ||
deletebutton.addEventListener("click", handleDeleteButtonClick) | ||
); | ||
|
||
checkboxes.forEach((checkbox) => | ||
checkbox.addEventListener("change", handleCheckboxChange) | ||
); |
There was a problem hiding this comment.
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);
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key question에 버블링이 있어서 한번 활용해 봐야겠다 생각했지만. . . 적절한 쓰임을 찾지 못하고 적용하지 못했는데요, 주희 님 코드를 보며 이렇게 사용할 수 있구나 배웠습니다 ㅎㅎ 지원 님도 좋은 피드백 감사합니다!
function submitNewTodo() { | ||
if (todoInput.value === "") return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
입력값이 없을 때 submit 되지 않도록 처리해주신 점 좋아요! 다만, 공백(스페이스)만 입력되었을 때도 submit 되지 않도록 추가적인 처리를 해주면 더욱 좋을 것 같습니당
deleteButton.addEventListener("click", handleDeleteButtonClick); | ||
deleteButton.addEventListener("click", handleUncheckedDeleteButtonClick); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하나의 deleteButton에 두 가지 핸들러를 따로 두고 있어서 한 번의 클릭에도 불필요한 중복 연산이 발생할 수 있겠네요🥲 체크가 이미 된 항목인지의 여부에 따라 조건문으로 분기처리해서 하나의 핸들러로 처리하면 효율적일 것 같습니다!
function handleDeleteButtonClick(e) { | ||
const label = e.target.previousElementSibling; | ||
|
||
// 완료된 투두 삭제 | ||
if (label.classList.contains(CHECKED_CLASS)) { | ||
const li = e.target.parentElement; | ||
li.remove(); | ||
} | ||
} |
There was a problem hiding this comment.
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 로직
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addEventListener가 여러 개의 이벤트 리스너를 중첩해서 등록할 수 있다는 사실을 새로 학습해서 한번 써먹어 봐야겠다는 생각에 너무 사로잡혔나 봅니다. . . 말씀해 주신 대로 isChecked에 대한 연산이 불필요하게 두 번 동작하겠네요 조건을 변수로 선언해서 가독성을 높이는 방법도 좋은 방법 같습니다!
const deleteButtons = document.querySelectorAll(".delete-button"); | ||
const checkboxes = document.querySelectorAll(".todo-item input"); | ||
|
||
const CHECKED_CLASS = "checked-todo-content"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모듈화하는 방법을 찾아볼 생각을 못 하고 다만 분할된 script일지라도 동일한 컨텍스트에서 실행된다는 점을 이용해 export import 없이 다른 파일의 변수나 함수를 사용하는 꼼수를 부려 봤습니다 ㅎㅎ💦💦 말씀해 주신 대로 모듈화하여 스코프를 분리하면 더 안정적인 코드가 될 것 같네요!
} | ||
|
||
.confirm-button { | ||
background-color: indianred; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
darkslategrey도 그렇고 컬러명을 정말 잘 활용하시네요 🤩!
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; |
There was a problem hiding this comment.
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를 생성하는 방식을 고민해보셔도 좋을 것 같습니다~~
border-bottom: 1.5px solid darkslategrey; | ||
|
||
display: grid; | ||
grid-template-columns: auto 1fr 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리드 레이아웃을 활용해서 아이템 내 요소 간 배치를 정리한 부분 넘 깔끔하네요🥹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1주차 과제하시느라 고생많으셨습니다! 전반적으로 코드가 깔끔하고, 함수명이나 클래스명이 각 용도에 맞게 작성되어 직관적인 코드라고 생각됩니다. 지금도 코드가 간결하고, 가독성이 좋지만 주석을 더 활용하는 것도 좋을 것 같아요. 전역으로 상수를 관리하는 것, 이벤트 핸들러 중복 등을 고려해서 리액트 2주차 리팩토링이 진행되면 좋을 것 같습니다! 저도 바닐라JS를 잘 모르는데 부원분들 코드리뷰를 하면서 공부가 되어 너무 좋습니다 😬 다들 실력이 너무 좋네용.
@@ -0,0 +1,78 @@ | |||
const todoInput = document.querySelector('input[type="text"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전역으로 반복되는 요소를 한번씩 호출하여 참조하는 거 너무 좋습니다 👍
deleteButton.innerText = "삭제"; | ||
|
||
checkbox.addEventListener("change", handleCheckboxChange); | ||
deleteButton.addEventListener("click", handleDeleteButtonClick); |
There was a problem hiding this comment.
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);
}
}
` 요런 느낌..?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지원 님 코멘트를 보고 단순히 handleDeleteButtonClick 내부에서 handleUncheckedDeleteButtonClick 함수를 호출할 생각을 했었는데 영준 님 코드처럼 openModal을 호출하는 편이 더 좋겠네요!
const cancelButton = document.createElement("button"); | ||
const confirmButton = document.createElement("button"); | ||
|
||
modalWrapper.classList.add("modal-wrapper"); |
There was a problem hiding this comment.
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)); }
요런 느낌으로! 저는 코드 짤 때 한번에 처리할 수 있거나 분리가 가능한 것은 유틸함수로 빼서 사용하려고 노력하는중입니다 ._.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동일한 element에 여러 class를 할당하는 로직이 반복될 때에는 확실히 보여주신 코드처럼 함수를 작성해도 좋을 것 같네요!
background-color: darkslategrey; | ||
} | ||
|
||
.todo-item label { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가로 스크롤이 가능하게 해주셨네요! 유니크한 방법인 것 같습니다. 사용자 입장의 방식을 조금 고려하면 word-wrap으로 줄바꿈이 되게하는 것도 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ui적으로 투두마다 동일한 줄 간격을 유지하고 싶은 욕심에 줄바꿈되지 않도록 했습니다 ㅎㅎ... 원래는 줄이 길어지는 경우 새 li 태그를 추가하고 싶었는데 구현이 너무 복잡해지더라고요. 바닐라js에서는 구현이 복잡해지면 쉽게 타협해버리는 것 같습니다. . . 💦💦 리액트로 마이그레이션하면서 구현해 보겠습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 코드 감사합니다! 제가 코드 리뷰를 늦게 시작해서 잘 짜신 코드라고 생각되기도 하고 이미 다른 분들이 전반적으로 말씀해주신 것 같아서 크게 말씀드릴 건 없는 것 같습니다 ㅜㅜ
궁금한 점이 하나 있는데, script 폴더 내에 js 파일 기능별로 쪼개서 구현하신 거 같은데 혹시 어떤 기준을 사용하셨는지 여쭤봐도 될까요? 저는 개인적으로 함수 따로 / constant 따로 / 실제 DOM 구현 따로 이렇게 진행을 했어서, 어떤 기준으로 진행하셨는지 궁금합니다!
추후 기회가 되신다면 local storage 까지 적용하신 코드도 궁금하네요 좋은 코드 감사합니다 :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모달까지 구현하신 점이 인상적이었고, 전반적으로 로직이 깔끔한 것 같다고 생각합니다!
다른 분들이 작성해주신 리뷰에 좋은 피드백을 읽으면서 저도 모르는 부분을 알게 됐고, 코드 리뷰도 즐거웠습니다!
고생하셨어요 :)
label.classList.add(CHECKED_CLASS); | ||
} else { | ||
label.classList.remove(CHECKED_CLASS); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스가 하나인 경우에는 toggle
메소드를 사용해서 조건문을 더 간결하게 표현할 수 있을 것 같아요!
배포 링크
https://vanilla-todo-21th-4ynn0nwdc-gustns-projects.vercel.app/
production은 default 브랜치만 되는 것 같아서 preview로 배포했습니다!! 혹시 이 방식이 잘못됐다면 알려주세요!
미션을 하며 . . .
바닐라 js를 공부한 지 1년이 되었을 뿐인데 이렇게나 까마득하네요. . . 옛날 코드들도 찾아보고 즐거웠습니다 👍
지난 해 초에 혼자서 바닐라 js 공부를 할 때는
getElementById
와addEventListener
를 사용했었는데요, 그 해 3월에 시작한 스터디에서는querySelector
와 이벤트 핸들러 메서드에 할당하는 방식을 거의 강제하다시피 했었습니다. 그때는 js 공부를 시작한 것도 몇 달 안 돼서 그저 하라는 대로만 했었는데 이제서야 뭐가 뭔지 눈에도 좀 들어오고 찾아보고 비교할 여유가 생겼네요!공유하면 좋을 내용 같아 간략히 정리해 보겠습니닷
getElementById
vsquerySelector
getElementById
가 성능이 더 좋습니다. (querySelector
가 약 1.2~1.3배 느리다고 하네용)querySelector
는 다양한 css 선택자를 허용합니다..input-item input
과 같이 속성이나 자손 선택자를 사용하는 게 좋더라고요!getElementByClass
와querySelectorAll
도 유사 배열을 반환한다는 점은 같지만 실제 타입이 달라서 생기는 차이점들이 있으니 찾아보셔도 좋을 것 같습니다!이벤트 핸들러 메서드 vs
addEventListener
element.onclick() = () => {...}
와 같이 이벤트 핸들러 메서드에 직접 함수를 할당한 경우를 이벤트 핸들러 메서드라고 불러보겠습니다.addEventListener
는 중복으로 할당됩니다. 즉, 할당한 이벤트 핸들러가 모두 동작합니다.addEventListener
는 메서드에 할당하는 방식과 달리 세 번째 인자를 통해 캡처링이나 버블링 시에 동작하도록 설정할 수 있습니다.addEventListener
를 사용하는 방식으로 리팩토링했습니다! 브라우저 호환성 등 다른 차이들도 있으니 한번쯤 찾아보시면 좋겠습니다!과제 내내 리액트적 사고가 머리를 떠나지 않아서 투두 리스트를 배열로 관리하고 싶은데 엄두를 못 내겠더라고요. . . 현재 제 생각은요, 사용자 입력에 따라 배열을 업데이트하고 그 배열을 순회하며 화면을 렌더링하는 리액트와 같은 동작을 바닐라 js로는 시작도 못하고 주저하는 건, 리액트의 버추얼 돔이 바닐라에는 없다는 것을 알기 때문이 아니려나요. . . 버추얼 돔 없이 건너가기엔 너무 먼 강인 거죠. . .
비록 저는 그랬지만!! 다른 여러분들은 또 어떻게 코드를 짜셨을지 기대가 많이 됩니다!!
Key Questions
1. DOM은 무엇일까요?
mdn에서는 이렇게 정의하고 있습니다. 문서, 다르게 얘기하면 브라우저를 구성하는 노드(태그)들의 프로퍼티, 메서드, 이벤트를 객체 형식으로 담아서 프로그래밍 언어를 사용해 접근할 수 있도록 표현한 방식이다, 정도로 저는 정리해 보겠습니다.
2. 이벤트 흐름 제어(버블링 & 캡처링)이 무엇인가요?
버블링
e.stopPropagation()
을 추가하면 playing만 alert되겠죠?캡처링
3. 클로저와 스코프가 무엇인가요?
클로저
스코프
var
는 지역변수,let
과count
는 블록변수입니다. 이에 따라서 아래와 같은 흥미로운 예제가 존재하는데요,var
로 선언된 변수이기 때문에 i는 전체 함수에서 유효하고, 결국 setTimeout에서 클로저를 통해 받아오는 i의 값은 함수가 종료된 시점의 3이 됩니다.let
으로 선언하는 경우 각각의 i는 각각의 블록에서 유효하기 때문에 setTimeout에서 클로저를 통해 받아오는 i의 값도 1, 2, 3이 된다고 합니다!