-
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주차] 이주희 미션 제출합니다. #1
base: main
Are you sure you want to change the base?
Conversation
만들기
모듈화로 인해서 전역함수화가 안됐음
같은 ui달이면 전 달 날짜라도 이번달로 착각하는 문제가 생김
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를 모듈로 분리하고, 이벤트 위임사용이나, 중복 방지, 투두아이템을 배열로 관리하여 로컬스토리지를 사용하는 데이터 구조가 직관적인 게 좋았습니다.
사실 저도 VanillaJS에 대해 자세히 몰라서 어떻게 코드리뷰를 해야 될지 감이 안 잡혔는데 전반적으로 주석이나 로직을 직관적으로 명시해주셔서 흐름을 따라갈 수 있었습니다! 여기서 그냥 제가 좋아하는 습관인데 저는 함수나 특정 로직이 있는 부분에 주석을 달고, 조금 많이 띄워놔서 분리해놓습니다! 그래서 제 코드에 빈공간이 꽤 있는데 그렇게 했을 때 주석과 로직이 한눈에 보여 좋았어서 이런 방법도 있구나~정도로 알아주셨으면 좋겠습니다!
앞으로의 과제 제출이 기대가 되네요 😬. 과제를 진행하면서 개인적으로 추천하는 방향성은 저희가 스터디를 진행할 때 JS -> React -> TS -> Next.js 이런식으로 진행하는데 이 순서는 각 기술이 출시된? 순서라고 보셔도 무방합니다. 그렇다면 출시된 이유가 있겠죠? 왜 React가 나왔고, 왜 TS가 나왔고, 왜 Next.js가 나왔는지 등등 프레임워크, 라이브러리, useMemo와 같은 utils 함수들의 목적과 **'Why?'**라는 질문을 끊임없이 하다보면 개발적으로 많이 얻어가는 게 있을 것 같습니다 👍
과제하느라 고생많으셨습니다! 1등이시니 🥇 금메달드리겠습니다~~2개 드리겠습니다 🥇 붐업
<!-- react 포탈과 비슷하게 활용 --> | ||
<div id="modal-root"></div> | ||
<!-- import export를 활용하기 위해 모듈화 --> | ||
<script type="module" src="modal.js"></script> |
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.
js파일의 사용성을 위해 모듈화해놓은 점 너무 좋은 것 같아요. 사소한 곳에서 재사용성을 챙기는 습관이 눈에 띕니다!
calendar.js
Outdated
document.querySelector(".dates").innerHTML = dates.join(""); | ||
|
||
//버블링을 사용하여 이벤트 위임 | ||
document.querySelector(".dates").addEventListener("click", (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.
각 요소마다 리스너를 등록하지 않고, closest을 활용한 이벤트 위임 로직이 정말 효율적인 것 같습니다.
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.
이벤트리스너관련해서는 이미 고치셨을테지만! 추후 스터디를 진행하면서 React, Next에서도 scroll이나 다른 이벤트리스너를 등록하는 일이 생길텐데 그때마다 이벤트 리스너의 중복과 등록관련해서 최적화를 항상 고려하면 좋습니다! scroll 이벤트 관련해서는 throlttle, debounce 등의 다양한 방식이 있으니 참고하셔도 좋습니다.
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.
사실 밑에서 보셨겠지만!... 이벤트 중복 관련해서 고쳤다라기보다는 모달이 불려나올 때 마다 초기화를 시켜주는 방식으로 해결해서 조금 야매로 해결하긴했습니다 ㅎㅎ...
|
||
renderCalendar(); | ||
|
||
//캘린더 달 이동 네브 함수 |
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 const renderModal = (selectedDate) => { | ||
const modalRoot = document.querySelector("#modal-root"); | ||
// 모달 렌더링 전 기존 모달 삭제 | ||
const existingModal = modalRoot.querySelector(".modal-overlay"); |
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.
기존 모달을 삭제하여 모달중첩을 방지하는 예외처리 좋습니다. 현재 상황에선 문제가 없지만, 다중 모달과 같은 스택형태의 모달이 필요하는 상황에서는 다른 로직을 사용해야 할 수도 있습니다. 당장 필요한 부분은 아니지만 모달중첩과 관련하여 다양한 로직을 생각해보는 것도 도움이 될 것 같아요 😬
nextDates.push(i); | ||
} | ||
|
||
const dates = prevDates.concat(currentDates, nextDates); |
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.
"dates", "modal-overlay"와 같은 클래스, id 문자열을 여러 곳에서 사용하는데 파일 상단에 상수로 분리하거나, DOM element로 잡아두어도 좋을 것 같아요. 저도 자주 쓰이는 문자를 상수로 처리하는 것을 최근에 좋다고 느껴서 한번 고려해보셔도 좋습니다.
예시 : const DATES_SELECTOR = ".dates";
modalOverlay.classList.add("modal-overlay"); | ||
|
||
//모달 생성 | ||
const updateTodo = (selectedDate, todos) => { |
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.
현재 updateTodo에서 모달 생성, 이벤트 등록, 스토리지 갱신, 렌더 등의 여러 기능이 한꺼번에 처리되고 있네요! 기능이 복잡해지는 경우를 대비해서 ToDo가 업데이트되는 과정을 한번 생각해본 뒤 단계별로 함수를 분리하여 관리했을 때 유지보수가 더 용이할 것 같습니다 👍
const newTodo = { text: newTodoText, isDone: false }; | ||
addData(selectedDate, newTodo, todoData); | ||
updateTodo(selectedDate, todoData[selectedDate] || []); | ||
renderCalendar(); |
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.
renderCalendar()가 체크박스 변경이 있을 때, 할 일이 삭제 및 추가 될 때 등등 여러 곳에서 호출되고 있는 것으로 보이는데 아마 실시간 업데이트를 위해서 그런 것 같아요! 지금은 괜찮지만 화면에서 큰 비중을 차지하는 calendar가 자주 렌더링되는 것은 성능적으로 고려해볼만한 것 같습니다. 프로젝트 진행하면서 유저를 위해 신경써야 하는 큰 부분중 하나가 렌더링 최적화이기 때문에 작은 과제에서부터 렌더링 최적화를 마음속에 품고 가시면 훗날 개발 우선순위를 계획할 때 도움이 될 것 같아요
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.
저는 css 내부에서도 주석을 다는 것을 좋아합니다! calendar에 쓰인 class들, modal에 쓰인 class들이 각기 다를텐데 어디에 쓰였는지를 나중에 확장하다보면 classname으로만 판별하기가 어려울 수 있을 것 같아요! 주석을 작성하는 습관은 늘 좋은 것 같습니다. 사실 저도 이래놓고, 다음날 개발할 때 주석을 까먹습니다 😭
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.
기능이 엄청 복잡하고 많아서 놀랐습니다. . . 손이 정말 빠르시네요. . . 👍 제가 주저하던 코드를 작성하시기도 했고 이것저것 코드에 차이가 있어서 재미있게 봤습니다! 고생 많으셨습니다~~!
calendar.js
Outdated
//오늘 날짜 | ||
const date = new Date(); |
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.
아래에 dates 배열과 그 배열의 forEach문에서 date 변수명을 또 사용하고 있어서 여기 있는 date는 today 같은 변수명이 더 좋지 않았을까 싶습니다. 그러면 오늘 날짜라는 주석 없이도 이해가 되기도 하고요!
calendar.js
Outdated
const selectedDate = `${year}/${month}/${eachDate}`; | ||
const isTodo = todoData[selectedDate] && todoData[selectedDate].length > 0; | ||
const isTodoClass = isTodo ? "isTodo" : ""; | ||
const todo = isTodo | ||
? `<p class="todo">할일 ${todoData[selectedDate].length}개 !</p>` | ||
: ""; | ||
const condition = | ||
i >= firstDate && i < lastDateIndex + 1 ? `current` : `other`; | ||
if ( | ||
eachDate === new Date().getDate() && | ||
date.getMonth() === new Date().getMonth() | ||
) { | ||
dates[ | ||
i | ||
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="today ${isTodoClass}" >오늘</span>${todo}</div>`; | ||
} else { | ||
dates[ | ||
i | ||
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="${condition} ${isTodoClass}">${eachDate}일</span>${todo}</div>`; | ||
} | ||
}); | ||
|
||
//캘린더 렌더링 | ||
document.querySelector(".dates").innerHTML = dates.join(""); | ||
|
||
//버블링을 사용하여 이벤트 위임 | ||
document.querySelector(".dates").addEventListener("click", (e) => { | ||
const openModalEl = e.target.closest(".openModalClass"); | ||
if (openModalEl) { | ||
const selectedDate = openModalEl.dataset.date; | ||
renderModal(selectedDate); | ||
} | ||
}); |
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.
사실 실질적인 렌더링 로직은 이 부분인 것 같은데요, 저에게는 앞서서 달력 배열을 만드는 로직이 너무 길게 느껴지기도 합니다. 달력 데이터를 가지는 배열을 만드는 과정은 createCalendar 함수에, 그 배열을 순회하면서 실제 돔 노드를 추가하는 로직은 renderCalendar 함수에 작성하는 등 두 로직의 관심사에 따라 분리해 주는 편이 가독성 측면에서도, 유지보수 측면에서도 더 좋을 수 있을 것 같아요!
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.
추가로 저도 버블링을 활용해 이벤트를 위임한 로직을 보고 많이 놀랐습니다!! 리액트를 사용할 때에도 재사용하는 코드는 이벤트 핸들러까지 컴포넌트에 할당해 와서 요소마다 이벤트 핸들러를 등록하는 행위에 대해 의구심을 가져 본 적이 없었거든요. 혹시 react나 next 같은 프레임워크를 사용하실 때에도 이런 방식으로 버블링을 활용한 코드를 짜셔서 그 흐름이 자연스럽게 이어지신 걸까요?
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.
개별 요소에 이벤트를 넣으니까 이벤트 리스너가 캘린더를 리랜더링하면 자꾸 삭제 되는 문제가 생겼습니당... 그래서 이를 해결하기 위해서 버블링을 공부해서 사용하였고 따로 다른 프로젝트에서 사용해본 적은 없습니다!
modalOverlay.innerHTML = ` | ||
<div class="modal"> | ||
<div class="modal-header"> | ||
<p class="modal-title">${viewDate}의 할 일</p> | ||
<div class="modal-body"> | ||
<ul id="todoList"> | ||
${todos | ||
.map( | ||
(todoItem, index) => | ||
`<li class="todoItem"> | ||
<input type="checkbox" id="todoChk-${index}" ${ | ||
todoItem.isDone ? "checked" : "" | ||
} /> | ||
<label for="todoChk-${index}">${todoItem.text}</label> | ||
<button class="deleteTodo" data-index="${index}">삭제</button> | ||
</li>` | ||
) | ||
.join("")} | ||
</ul> | ||
<input type="text" id="todoInput" placeholder="할 일을 입력하세요" /> | ||
<button id="addTodo">할 일 추가</button> | ||
</div> | ||
</div> | ||
`; |
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.
updateTodo 함수의 주석이 모달 생성이라는 지점에서 관심사의 분리가 잘 일어나지 않음이 보이는 것 같습니다. 저는 주석을 잘 달지 않는 편인데요, 대신에 변수명이 직관적인지, 함수명이 그 동작을 잘 설명하고 있는지를 오래 고민합니다!
이 경우에는 updateTodo라는 이름의 함수가 초기 렌더링까지 담당하고 있다는 점, 할일 추가 및 삭제에서도 사용되는데 실제 변경되지 않은 요소들까지 모두 다시 설정하고 있다는 점을 오래 고민해 볼 것 같습니다.
추가로 함수를 외부에 작성하는 것도 가독성을 높이는 방법이 될 수 있을 것 같습니다! 함수를 작성하는 건 보통 재사용성을 높이거나 캡슐화를 하려는 의도인데, 내부 함수로 작성하는 경우에는 캡슐화의 이점을 활용하지 못하니까요.
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.
제가 사실 주석이랑 변수명에 정~~~말 약합니다 (사실 주석도 이번에 처음 써 본..) 리뷰 보니 다시 한번 그 사실이 느껴지네요...
modalOverlay | ||
.querySelector("#todoInput") | ||
.addEventListener("keypress", (e) => { | ||
if (e.key === "Enter") { | ||
modalOverlay.querySelector("#addTodo").click(); | ||
} | ||
}); | ||
//할 일 삭제 | ||
modalOverlay.querySelectorAll(".deleteTodo").forEach((deleteTodo) => { | ||
deleteTodo.addEventListener("click", (e) => { | ||
const index = e.target.dataset.index; | ||
deleteData(selectedDate, index, todoData); | ||
updateTodo(selectedDate, todoData[selectedDate] || []); | ||
renderCalendar(); | ||
}); | ||
}); | ||
//체크박스 체크시 상태 저장 | ||
modalOverlay | ||
.querySelectorAll("input[type='checkbox']") | ||
.forEach((checkbox, index) => { | ||
checkbox.addEventListener("change", (e) => { | ||
todos[index].isDone = e.target.checked; | ||
saveData(todoData); | ||
}); | ||
}); | ||
//할 일 추가 | ||
modalOverlay.querySelector("#addTodo").addEventListener("click", () => { | ||
const todoInput = modalOverlay.querySelector("#todoInput"); | ||
const newTodoText = todoInput.value; | ||
if (newTodoText.trim() === "") { | ||
alert("할 일을 입력하세요"); | ||
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.
이런 부분에서도 저는 이벤트 리스너에 할당하는 함수를 별도로 정의해서 이벤트 핸들러 함수와 이벤트 리스너 등록 단위로 코드를 분리해 둘 것 같기는 합니다! 그렇게 하면 keypress 이벤트의 경우에 modalOverlay.querySelector("#addTodo").click()
대신 handleAddTodoButtonClick()
과 같이 작성할 수 있기도 하고요.
익명함수를 넘기는지 정의된 함수를 넘기는지는 취향 차이일 것 같으니 이런 사람도 있구나! 정도로 봐 주시면 될 것 같습니다!
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.
안녕하세요 호랑이 조교입니다.
모달까지 구현하시다니.. 앞으로의 과제를 기대하도록 하겠습니다.
함수의 관심사/책임을 좀 더 분리해보고, 중복되는 호출이나 선언은 줄여보면 좋을 것 같아요!
관련해서 코멘트 달았으니 참고만 해주시면 좋을 것 같습니다.
calendar.js
Outdated
document.querySelector(".dates").innerHTML = dates.join(""); | ||
|
||
//버블링을 사용하여 이벤트 위임 | ||
document.querySelector(".dates").addEventListener("click", (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.
const dateEl = document.querySelector(".dates");
로 DOM내에 계속 있다면 render 밖에 선언해도 좋을 것 같아요. 현재는 탐색이 계속 이루어지는 것 같습니다.
calendar.js
Outdated
} | ||
|
||
const dates = prevDates.concat(currentDates, nextDates); | ||
dates.forEach((eachDate, i) => { |
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.
const dateHTML = dates.map ( ~ )
이 더 직관적으로 보일 것 같습니다.
eachDate === new Date().getDate() && | ||
date.getMonth() === new Date().getMonth() |
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.
실시간 시/분을 가져오는 게 아니라면 맨 위에 선언한
//오늘 날짜
const date = new Date();
을 사용해도 되지 않을까요 ??
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.
밑에 함수에서 month가 바뀔때 리랜더 하는 용도로도 사용하고 있어서 여기서 date를 사용하면 3월 11일(오늘) 4월 11일(오늘) 이런식으로 표시가 돼서 사용을 못합니다 ㅜㅜ date는 오늘 날짜라기 보단 캘린더 랜더링을 위한 값? 정도로 봐야할 것 같아요! 주석은 그냥 저 명령어가 가지고 있는 기능만 적었는데 수정을 해야겠습니당..
calendar.js
Outdated
document.querySelector(".dates").addEventListener("click", (e) => { | ||
const openModalEl = e.target.closest(".openModalClass"); | ||
if (openModalEl) { | ||
const selectedDate = openModalEl.dataset.date; | ||
renderModal(selectedDate); | ||
} |
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.
이 이벤트리스너는 rendarCalendar 밖에서 한번만 등록해도 되지 않을까요 ?? 이벤트가 불필요하게 중복으로 추가가 되는 것 같아요
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.
헉 수정수정...👀 감사합니다
@media (max-width: 768px) { | ||
width: 80%; | ||
} |
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.
깨알 반응형 대박
modalOverlay.querySelectorAll(".deleteTodo").forEach((deleteTodo) => { | ||
deleteTodo.addEventListener("click", (e) => { | ||
const index = e.target.dataset.index; | ||
deleteData(selectedDate, index, todoData); | ||
updateTodo(selectedDate, todoData[selectedDate] || []); | ||
renderCalendar(); | ||
}); | ||
}); |
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.
개별 이벤트 리스너를 등록하기보다는, 이벤트 위임을 고려해보는 것도 좋을 것 같습니다.
위임으로 고려를 해보면 modalOverlay.querySelectorAll ~~로 처리되는 할일 추가/할일 삭제를 하나의 콜백함수 내부에서 분기처리를 할 수 있을 것 같아요
storage.js
Outdated
if (localStorage.getItem(StorageKey)) { | ||
return JSON.parse(localStorage.getItem(StorageKey)); |
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.
사소하지만 중복된 호출은 한 번의 호출로 묶어주면 좋을 것 같아요
if (prevLastDay !== 6) { | ||
for (let i = 0; i < prevLastDay + 1; i++) { | ||
prevDates.unshift(prevLast.getDate() - i); | ||
} | ||
} | ||
|
||
for (let i = 1; i < 7 - currentLastDay; i++) { | ||
nextDates.push(i); | ||
} |
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.
여기서의 6,7이나 아래 month 관련 12,11은 매직넘버로 관리해주면 좋을 것 같습니다
간단하게 변경 할 수 있는 부분 만
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.
파일을 구분해서 정리하고, 재사용성이 높은 코드를 작성해주신 점이 좋았습니다 !!
저도 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.
함수를 따로 관리해서 재사용성을 높이신 점이 좋았어요!!
} | ||
|
||
const dates = prevDates.concat(currentDates, nextDates); | ||
const dateHTML = dates.forEach((eachDate, i) => { |
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()는 반환값이 없기 때문에 dateHTML 변수에 할당해도 실제로 사용되지 않고 있습니다! 이 경우 변수를 제거하거나 forEach() 대신 map()을 사용해도 될 거 같아용
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.
저도 바꾸고 나서 이럴꺼면 왜 바꿨지 하면서 후회했습니다...
const prevMonth = () => { | ||
date.setMonth(date.getMonth() - 1); | ||
renderCalendar(); | ||
}; | ||
const nextMonth = () => { | ||
date.setMonth(date.getMonth() + 1); | ||
renderCalendar(); | ||
}; | ||
const goCurrentYearMonth = () => { | ||
date.setMonth(new Date().getMonth()); | ||
renderCalendar(); | ||
}; | ||
//전역 함수화 (모듈화를 했기 때문에) | ||
window.prevMonth = prevMonth; | ||
window.nextMonth = nextMonth; | ||
window.goCurrentYearMonth = goCurrentYearMonth; |
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.
const changeMonth = (diff) => {
date.setMonth(date.getMonth() + diff);
renderCalendar();
};
window.prevMonth = () => changeMonth(-1);
window.nextMonth = () => changeMonth(1);
window.goCurrentYearMonth = () => changeMonth(new Date().getMonth() - date.getMonth());
이런 식으로 반복되는 코드는 묶는 방법도 있을 거 같습니다 !!
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.
캘린더 뷰, 할 일이 존재하는 부분에 하이라이트, 오늘 표시까지 섬세하고 귀여운 TodoList 잘 봤습니다! 부족한 실력으로 코드 리뷰 남겨봅니다😊
@@ -0,0 +1,26 @@ | |||
const StorageKey = "todoData"; |
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.
변하지 않는 값을 담는 상수 변수명은 대문자로 작성하는 코딩 컨벤션이 있는데 따라보면 좋을 거 같습니다. 에어비앤비 코딩 컨벤션 22.15
//캘린더 렌더링 함수 | ||
export const renderCalendar = () => { | ||
//데이터 로드 | ||
const todoData = loadData(); |
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.
loadData()는 localStorage에서 전체 데이터를 꺼내와 JSON 파싱하는 로직인 걸로 알고 있습니다. 자주 쓰이는 데이터이므로 페이지를 로딩할 때 localStorage에서 한 번 읽어서 변수에 저장해두고 이후에는 로컬 상태로 유지해보는 게 어떨까요?
for (let i = 0; i < prevLastDay + 1; i++) { | ||
prevDates.unshift(prevLast.getDate() - i); | ||
} |
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.
unshift는 O(n) 연산이고 for문에 의해 현재 O(n²) 연산인데, 이렇게 반복적으로 unshift를 사용해야 하는 경우에는 push n*O(1) 후 reverse O(n)하는 것이 O(n)으로 최적화에 더 좋다고 합니다.
const dateHTML = dates.forEach((eachDate, i) => { | ||
const firstDate = prevDates.length; | ||
const lastDateIndex = prevDates.length + currentDates.length - 1; | ||
let year = currentYear; | ||
let month = currentMonth + 1; | ||
//전후 달과 현 달 구분 | ||
if (i < firstDate) { | ||
month = currentMonth === 0 ? 12 : currentMonth; | ||
if (currentMonth === 0) { | ||
year = currentYear - 1; | ||
} | ||
} else if (i > lastDateIndex) { | ||
month = currentMonth === 11 ? 1 : currentMonth + 2; | ||
if (currentMonth === 11) { | ||
year = currentYear + 1; | ||
} | ||
} | ||
//날짜별 데이터 할당 | ||
const selectedDate = `${year}/${month}/${eachDate}`; | ||
const isTodo = todoData[selectedDate] && todoData[selectedDate].length > 0; | ||
const isTodoClass = isTodo ? "isTodo" : ""; | ||
const todo = isTodo | ||
? `<p class="todo">할일 ${todoData[selectedDate].length}개 !</p>` | ||
: ""; | ||
const condition = | ||
i >= firstDate && i < lastDateIndex + 1 ? `current` : `other`; | ||
if ( | ||
eachDate === new Date().getDate() && | ||
date.getMonth() === new Date().getMonth() | ||
) { | ||
dates[ | ||
i | ||
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="today ${isTodoClass}" >오늘</span>${todo}</div>`; | ||
} else { | ||
dates[ | ||
i | ||
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="${condition} ${isTodoClass}">${eachDate}일</span>${todo}</div>`; | ||
} | ||
}); |
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.
전 달끼리 그리고 후 달끼리 year과 month가 같을 것이고,
전후 달은 일괄적으로 other
class가 배정되면 되기도 하니까
전후 달과 현 달의 html element를 만드는 로직을 분리해봐도 좋을 거 같아요.
if ( | ||
eachDate === new Date().getDate() && | ||
date.getMonth() === new Date().getMonth() | ||
) { | ||
dates[ | ||
i | ||
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="today ${isTodoClass}" >오늘</span>${todo}</div>`; | ||
} else { |
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.
해당 구간은 오직 '오늘'인지 아닌지를 판별하기 위해 존재하는 것으로 이해했습니다. 그런데 주희님께서는 data-date라는 사용자 정의 속성을 이용하고 계시니까 반복문 밖에서 date.getMonth() === new Date().getMonth()
라면 오늘 날짜의 element를 선택해와서 innerHTML만 바꿔줘도 좋을 거 같아요. const element = document.querySelector('[data-date={오늘 날짜}]');
과제 배포 링크
과제 관련 블로그 정리
느낀점
질문
1. DOM이란?
DOM(Document Object Model) 이란 브라우저가 HTML 문서를 화면에 표시할 때 이를 자바스크립트가 조작할 수 있도록 객체 형태로 변환한 문서 구조 모델을 말합니다. DOM은 트리 구조 형태로 되어 있어서 HTML의 각 요소는 하나의 객체가 되고 자바스크립트는 이를 통해 HTML 문서의 내용을 읽고 추가, 수정, 삭제할 수 있습니다.
이 DOM 덕분에 자바스크립트는 다음과 같은 메서드를 통해 HTML 요소를 선택하거나 조작할 수 있습니다.
- 요소 선택: document.getElementById(), document.querySelector()
- 요소 수정: element.textContent, element.innerHTML
- 이벤트 등록: element.addEventListener()
DOM을 사용함으로써 자바스크립트가 동적인 웹 페이지를 만들 수 있게 됩니다.
2. 이벤트 전파
1 ) 캡처링: 이벤트가 발생하면 최상위 요소에서 하위요소로 이벤트를 전달
- 최상위(window) -> 상위(document) -> 부모 -> 자식 요소
2 ) 버블링: 이벤트가 발생하면 하위 요소에서 시작해 부모 요소로 전파되는 방식 (캡처링과 반대)
- 자바 스크립트에서 사용하는 방식
- 최상위(window) <- 상위(document) <- 부모 <- 자식 요소
- 이번 과제에서 이벤트 위임을 사용
- 이벤트 위임: 여러 개의 하위 요소들에 이벤트를 붙이는 대신 공통된 부모에 리스너를 붙여 하위요소 이벤트를 관리하는 방식
- 이벤트가 버블링되어 올라가는 과정에서 closest를 사용해 특정 부모 요소를 찾아 이벤트를 처리
3. 클로저와 스코프
-보통 이때 많이 쓰임