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주차] 이주희 미션 제출합니다. #1

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Binary file added asset/fonts/Cafe24Supermagic-Bold-v1.0.otf
Binary file not shown.
Binary file added asset/fonts/Cafe24Supermagic-Bold-v1.0.ttf
Binary file not shown.
Binary file added asset/fonts/Cafe24Supermagic-Regular-v1.0.otf
Binary file not shown.
Binary file added asset/fonts/Cafe24Supermagic-Regular-v1.0.ttf
Binary file not shown.
115 changes: 115 additions & 0 deletions calendar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import { loadData } from "./storage.js";
import { renderModal } from "./modal.js";
//캘린더에서 사용할 데이터 날짜
const date = new Date();
//캘린더 렌더링 함수
export const renderCalendar = () => {
//데이터 로드
const todoData = loadData();
Copy link

Choose a reason for hiding this comment

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

loadData()는 localStorage에서 전체 데이터를 꺼내와 JSON 파싱하는 로직인 걸로 알고 있습니다. 자주 쓰이는 데이터이므로 페이지를 로딩할 때 localStorage에서 한 번 읽어서 변수에 저장해두고 이후에는 로컬 상태로 유지해보는 게 어떨까요?


const currentYear = date.getFullYear();
const currentMonth = date.getMonth();

//현 캘린더 년월 표시
document.querySelector(".currentYearMonth").textContent = `${currentYear}년 ${
currentMonth + 1
}월`;

//이전 달 마지막 날, 현재 달 마지막 날 날짜 및 요일
const prevLast = new Date(currentYear, currentMonth, 0);
const currentLast = new Date(currentYear, currentMonth + 1, 0);

const prevLastDay = prevLast.getDay();
const currentLastDay = currentLast.getDay();

//이전 달, 현재 달, 다음 달 날짜 배열 생성
const prevDates = [];
const currentDates = Array.from(
{ length: currentLast.getDate() },
(_, i) => i + 1
);
const nextDates = [];

//현재 달 캘린더에서 보이는 전후 달 날짜 배열 생성
if (prevLastDay !== 6) {
for (let i = 0; i < prevLastDay + 1; i++) {
prevDates.unshift(prevLast.getDate() - i);
}
Comment on lines +35 to +37
Copy link

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)으로 최적화에 더 좋다고 합니다.

}

for (let i = 1; i < 7 - currentLastDay; i++) {
nextDates.push(i);
}
Comment on lines +34 to +42

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은 매직넘버로 관리해주면 좋을 것 같습니다


const dates = prevDates.concat(currentDates, nextDates);
Copy link
Collaborator

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";

const dateHTML = dates.forEach((eachDate, i) => {

Choose a reason for hiding this comment

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

forEach()는 반환값이 없기 때문에 dateHTML 변수에 할당해도 실제로 사용되지 않고 있습니다! 이 경우 변수를 제거하거나 forEach() 대신 map()을 사용해도 될 거 같아용

Copy link
Author

Choose a reason for hiding this comment

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

저도 바꾸고 나서 이럴꺼면 왜 바꿨지 하면서 후회했습니다...

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()
Comment on lines +72 to +73

Choose a reason for hiding this comment

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

실시간 시/분을 가져오는 게 아니라면 맨 위에 선언한

//오늘 날짜
const date = new Date();

을 사용해도 되지 않을까요 ??

Copy link
Author

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는 오늘 날짜라기 보단 캘린더 랜더링을 위한 값? 정도로 봐야할 것 같아요! 주석은 그냥 저 명령어가 가지고 있는 기능만 적었는데 수정을 해야겠습니당..

) {
dates[
i
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="today ${isTodoClass}" >오늘</span>${todo}</div>`;
} else {
Comment on lines +71 to +78
Copy link

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={오늘 날짜}]');

dates[
i
] = `<div class="date openModalClass " data-date="${selectedDate}"><span class="${condition} ${isTodoClass}">${eachDate}일</span>${todo}</div>`;
}
});
Comment on lines +45 to +83
Copy link

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를 만드는 로직을 분리해봐도 좋을 거 같아요.


//캘린더 렌더링
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);
}
});
renderCalendar();

//캘린더 달 이동 네브 함수
Copy link
Collaborator

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;
Comment on lines +100 to +115

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());

이런 식으로 반복되는 코드는 묶는 방법도 있을 거 같습니다 !!

42 changes: 42 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<!DOCTYPE html>
<html lang="ko">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>todoList</title>
<link rel="stylesheet" href="style.css" />
</head>
<body>
<div class="container">
<p class="main-title">투두 리스트</p>
<main class="calendar">
<nav class="header">
<button class="nav" id="prev" onClick="prevMonth()">이전</button>
<button
class="currentYearMonth"
onClick="goCurrentYearMonth()"
></button>
<button class="nav" id="next" onClick="nextMonth()">다음</button>
</nav>
<div class="main">
<div class="days">
<div class="day">일</div>
<div class="day">월</div>
<div class="day">화</div>
<div class="day">수</div>
<div class="day">목</div>
<div class="day">금</div>
<div class="day">토</div>
</div>
<div class="dates"></div>
</div>
</main>
<!-- react 포탈과 비슷하게 활용 -->
<div id="modal-root"></div>
<!-- import export를 활용하기 위해 모듈화 -->
<script type="module" src="modal.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

js파일의 사용성을 위해 모듈화해놓은 점 너무 좋은 것 같아요. 사소한 곳에서 재사용성을 챙기는 습관이 눈에 띕니다!

<script type="module" src="calendar.js"></script>
<script src="script.js"></script>
</div>
</body>
</html>
104 changes: 104 additions & 0 deletions modal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { renderCalendar } from "./calendar.js";
import { loadData, addData, deleteData, saveData } from "./storage.js";

// 선택 날짜 해당 모달 렌더링 함수
export const renderModal = (selectedDate) => {
const modalRoot = document.querySelector("#modal-root");
// 모달 렌더링 전 기존 모달 삭제
const existingModal = modalRoot.querySelector(".modal-overlay");
Copy link
Collaborator

Choose a reason for hiding this comment

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

기존 모달을 삭제하여 모달중첩을 방지하는 예외처리 좋습니다. 현재 상황에선 문제가 없지만, 다중 모달과 같은 스택형태의 모달이 필요하는 상황에서는 다른 로직을 사용해야 할 수도 있습니다. 당장 필요한 부분은 아니지만 모달중첩과 관련하여 다양한 로직을 생각해보는 것도 도움이 될 것 같아요 😬

if (existingModal) {
modalRoot.removeChild(existingModal);
}
//데이터 로드
const todoData = loadData();
const todos = todoData[selectedDate] || [];

//배경 레이어 생성
const modalOverlay = document.createElement("div");
modalOverlay.classList.add("modal-overlay");

//모달 생성
const updateTodo = (selectedDate, todos) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 updateTodo에서 모달 생성, 이벤트 등록, 스토리지 갱신, 렌더 등의 여러 기능이 한꺼번에 처리되고 있네요! 기능이 복잡해지는 경우를 대비해서 ToDo가 업데이트되는 과정을 한번 생각해본 뒤 단계별로 함수를 분리하여 관리했을 때 유지보수가 더 용이할 것 같습니다 👍

const clikedDate = new Date(selectedDate);
const viewDate =
clikedDate.getDate() === new Date().getDate() &&
clikedDate.getMonth() === new Date().getMonth()
? "오늘"
: `${clikedDate.getMonth() + 1}월 ${clikedDate.getDate()}일`;
//모달 내용
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>
`;
Comment on lines +29 to +52
Copy link

Choose a reason for hiding this comment

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

updateTodo 함수의 주석이 모달 생성이라는 지점에서 관심사의 분리가 잘 일어나지 않음이 보이는 것 같습니다. 저는 주석을 잘 달지 않는 편인데요, 대신에 변수명이 직관적인지, 함수명이 그 동작을 잘 설명하고 있는지를 오래 고민합니다!

이 경우에는 updateTodo라는 이름의 함수가 초기 렌더링까지 담당하고 있다는 점, 할일 추가 및 삭제에서도 사용되는데 실제 변경되지 않은 요소들까지 모두 다시 설정하고 있다는 점을 오래 고민해 볼 것 같습니다.

추가로 함수를 외부에 작성하는 것도 가독성을 높이는 방법이 될 수 있을 것 같습니다! 함수를 작성하는 건 보통 재사용성을 높이거나 캡슐화를 하려는 의도인데, 내부 함수로 작성하는 경우에는 캡슐화의 이점을 활용하지 못하니까요.

Copy link
Author

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();
});
});
Comment on lines +63 to +70

Choose a reason for hiding this comment

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

개별 이벤트 리스너를 등록하기보다는, 이벤트 위임을 고려해보는 것도 좋을 것 같습니다.

위임으로 고려를 해보면 modalOverlay.querySelectorAll ~~로 처리되는 할일 추가/할일 삭제를 하나의 콜백함수 내부에서 분기처리를 할 수 있을 것 같아요

//체크박스 체크시 상태 저장
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;
}
Comment on lines +55 to +87
Copy link

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()과 같이 작성할 수 있기도 하고요.

익명함수를 넘기는지 정의된 함수를 넘기는지는 취향 차이일 것 같으니 이런 사람도 있구나! 정도로 봐 주시면 될 것 같습니다!

const newTodo = { text: newTodoText, isDone: false };
addData(selectedDate, newTodo, todoData);
updateTodo(selectedDate, todoData[selectedDate] || []);
renderCalendar();
Copy link
Collaborator

Choose a reason for hiding this comment

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

renderCalendar()가 체크박스 변경이 있을 때, 할 일이 삭제 및 추가 될 때 등등 여러 곳에서 호출되고 있는 것으로 보이는데 아마 실시간 업데이트를 위해서 그런 것 같아요! 지금은 괜찮지만 화면에서 큰 비중을 차지하는 calendar가 자주 렌더링되는 것은 성능적으로 고려해볼만한 것 같습니다. 프로젝트 진행하면서 유저를 위해 신경써야 하는 큰 부분중 하나가 렌더링 최적화이기 때문에 작은 과제에서부터 렌더링 최적화를 마음속에 품고 가시면 훗날 개발 우선순위를 계획할 때 도움이 될 것 같아요

});
};

updateTodo(selectedDate, todos);
//모달 렌더링
modalRoot.appendChild(modalOverlay);
//모달 이외 창 눌럿을 시 모달 닫기
modalOverlay.addEventListener("click", (e) => {
if (e.target === modalOverlay) {
modalRoot.removeChild(modalOverlay);
}
});
};
Empty file added script.js
Empty file.
26 changes: 26 additions & 0 deletions storage.js

Choose a reason for hiding this comment

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

함수를 따로 관리해서 재사용성을 높이신 점이 좋았어요!!

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const StorageKey = "todoData";
Copy link

Choose a reason for hiding this comment

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

변하지 않는 값을 담는 상수 변수명은 대문자로 작성하는 코딩 컨벤션이 있는데 따라보면 좋을 거 같습니다. 에어비앤비 코딩 컨벤션 22.15

//데이터 저장
export const saveData = (todoData) => {
localStorage.setItem(StorageKey, JSON.stringify(todoData));
};
//데이터 로드
export const loadData = () => {
const localStorageData = localStorage.getItem(StorageKey);
if (localStorageData) {
return JSON.parse(localStorageData);
}
return {};
};
//데이터 추가
export const addData = (date, todo, todoData) => {
if (!todoData[date]) {
todoData[date] = [];
}
todoData[date].push(todo);
saveData(todoData);
};
//데이터 삭제
export const deleteData = (date, index, todoData) => {
todoData[date].splice(index, 1);
saveData(todoData);
};
Loading