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주차] 윤영준 미션 제출합니다. #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
45 changes: 43 additions & 2 deletions index.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,53 @@
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Vanilla Todo</title>
<title>To Do List-yyj0917</title>
<link rel="stylesheet" href="style.css" />
</head>

<body>
<div class="container"></div>
<div class="wrapper">
<div class="container">
<!-- header -->
<header>
<h2 class="today"></h2>
<span>
<h1>Today</h1>
<input type="date" id="date-picker" class="date-picker">
</span>
</header>
<!-- nav -->
<nav>
<ul class="week-days"></ul>
</nav>
<!-- main -->
<main class="task-list"></main>
<!-- footer -->
<footer>
Comment on lines +25 to +28

Choose a reason for hiding this comment

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

주석으로 시맨틱 태그를 구분해주셔서 구조가 한 눈에 들어오니 읽기에도 편하네요👍 사소한 습관이지만 좋은 것 같습니다!

<button class="btn-modal">+</button>
<form class="task-form" style="display: none;">

Choose a reason for hiding this comment

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

나타났다, 사라졌다 하는 요소의 display: none 속성을 인라인으로 작성해주셨네요! 취향 차이이긴한데 css 파일에 작성하시는 것보다 이렇게 만들어주시는 것이 저는 코드 리뷰적으로는 더 편했어요 👍

<div>
<input
type="text"
id="task-title"
placeholder="Task Title"
required
/>
<input
type="text"
id="task-desc"
placeholder="Task Description"
required
/>
</div>
<div>
<button type="submit" class="btn save">save</button>
<button type="button" class="btn back">back</button>
Comment on lines +46 to +47

Choose a reason for hiding this comment

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

form 태그 내의 button을 저는 기본 타입인 submit으로만 사용했어서 간과하고 있었는데, 덕분에 type에 관해 알아보게 되어서 적극 활용해야겠다는 생각이 듭니다 😂

</div>
</form>
Comment on lines +30 to +49

Choose a reason for hiding this comment

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

html 요소의 form 시스템을 잘 활용하고 계신 것 같아요! type="submit" 속성을 가지는 버튼으로 제출도 편리하게 진행하시구요.
이걸 javascript 함수로 다르게 구현할 수도 있지만 좀 복잡해질 수도 있으니까요 🙃

</footer>
</div>
</div>
</body>
<script src="script.js"></script>

Choose a reason for hiding this comment

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

html 문서가 parsing 되는 시점을 고려하신 것 같습니다. 위의 body 태그 내부에 있는 DOM 요소들이 모두 load 된 후 js로 관련된 작업을 하기 위해서 script 태그를 뒤에 배치하셨군요!

이 방식도 정말 좋고, script 요소에 defer 속성을 주시면 그냥 편하게 head 태그에 삽입하셔도 될 거에요 :)
관련 자료 첨부할게요

Copy link
Author

Choose a reason for hiding this comment

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

HTML파싱과 스크립트 로드의 동작과 관련하여 더 공부하게 되었습니다. async, defer 속성을 잘 활용한다면 번들 사이즈가 큰 코드에서도 효율적으로 로딩 시간을 관리할 수 있겠군요! 감사합니다.

</html>
190 changes: 190 additions & 0 deletions script.js
Original file line number Diff line number Diff line change
@@ -1 +1,191 @@
//😍CEOS 20기 프론트엔드 파이팅😍

// DOM Load시 실행되는 이벤트 리스너
document.addEventListener('DOMContentLoaded', function () {
const currentDate = new Date();
const today = document.querySelector('.today');
const weekDays = document.querySelector('.week-days');
const taskList = document.querySelector('.task-list');
const modalBtn = document.querySelector('.btn-modal');
const taskAddForm = document.querySelector('.task-form');
const backBtn = document.querySelector('.back');
const datePicker = document.getElementById('date-picker');

let selectedDate = getFormattedDate(currentDate); // selectedDate 초기값은 오늘 날짜

Choose a reason for hiding this comment

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

아래 코드에서 함수 선언식으로 만들어주심 getFormattedDate() 함수를 사용해주고 계시군요! function 키워드를 사용하신 함수 선언식은 hoisting이 발생할 때에 TDZ(Temporary Dead Zone)이 없어 정상적으로 잘 작동하지만, let 키워드를 토대로 함수 표현식으로 작성하셨다면 중간 중간 TDZ 에 걸려 예상대로 함수가 동작하지 않을 수도 있어요.

Javascript의 호이스팅 대해서도 공부해보셔도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

인지하지 못하고 있던 부분 알려주셔서 감사합니다. 리뷰를 통해 함수 선언 방식에 따른 호이스팅 여부와 TDZ라는 개념을 알아갈 수 있었습니다. 호이스팅을 통해 코드상 에러는 발생하지 않았겠지만, 조금 더 고려해서 함수를 위로 올리도록 하겠습니다!


// 상단 날짜 업데이트 함수
function updateDisplayDate(date) {
today.innerText = date.toLocaleString('en-US', { year: 'numeric', month: 'long', day: 'numeric' });

Choose a reason for hiding this comment

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

date 인스턴스의 메서드를 잘 활용하셔서 알맞은 포맷의 데이터를 사용자에게 UI로 제공하는 모습 👍

}

// input type=date의 초기값 설정
function getFormattedDate(date) {

Choose a reason for hiding this comment

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

js 과제라 어쩔 수 없긴 한데,,,, date 인자에 타입스크립트 적용하고 싶은 욕구가 드네요 😙

const year = date.getFullYear();
const month = ('0' + (date.getMonth() + 1)).slice(-2);
const day = ('0' + date.getDate()).slice(-2);
Comment on lines +23 to +24
Copy link

@psst54 psst54 Sep 9, 2024

Choose a reason for hiding this comment

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

지나가다 한줄 남깁니다..!🙇‍♀️🙇‍♀️ padStart()같은 메소드를 이용하면 문자열 앞을 원하는 길이가 될 때까지 패딩을 넣어 줄 수 있습니당!!

return `${year}-${month}-${day}`;
}

// input의 초기 값을 오늘 날짜로 설정
datePicker.value = selectedDate;

// 날짜 선택 이벤트 리스너
datePicker.addEventListener('change', function(event) {
const selectedDateObj = new Date(event.target.value); // 사용자가 선택한 날짜
selectedDate = selectedDateObj.toISOString().split('T')[0]; // 선택된 날짜의 'YYYY-MM-DD' 형식으로 설정
updateDisplayDate(selectedDateObj); // 상단 날짜 업데이트
loadWeekDays(selectedDateObj); // 선택한 날짜의 주간 표시
loadTasks(selectedDate); // 선택된 날짜의 할 일 불러오기
});

// 일주일 날짜 생성, 오늘 날짜 표시
function loadWeekDays(selectedDateObj = currentDate) {

Choose a reason for hiding this comment

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

함수의 인자로 default value를 적극적으로 활용해주셔서 처음 DOM이 로드될 때의 상황까지 잘 커버해주신 것 같습니다

weekDays.innerHTML = '';
updateDisplayDate(selectedDateObj);

// 날짜 계산
const firstDayOfWeek = new Date(selectedDateObj);
firstDayOfWeek.setDate(selectedDateObj.getDate() - selectedDateObj.getDay() + 1); // 월요일(0: 일요일, 1: 월요일)
Copy link

@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.

배포링크에서 확인해보니 현재 날짜가 8일(일요일)인데 주차는 9일(월요일)부터 시작인 주차가 뜨는 현상이 있었습니다! firstDayOfWeek을 정의한 곳에서 getDay() 값이 0인 일요일에 대해서도 +1되면서 그 다음주차가 뜨는 로직때문인 것 같습니다.

selectedDateObj.getDay()를 변수로 저장하여 값이 0일 때(일요일인 때)와 그렇지 않을 때를 조건문으로 구분해서 예외적으로 처리해주면 정확한 주차가 뜰 것 같네요😀

Copy link
Author

Choose a reason for hiding this comment

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

미처 확인하지 못한 부분인 것 같습니다. 짚어주셔서 감사합니다!


for (let i = 0; i < 7; i++) {
const day = new Date(firstDayOfWeek);
day.setDate(firstDayOfWeek.getDate() + i);
const dayElement = document.createElement('li');
const dateKey = day.toISOString().split('T')[0];

// week set
const weekSpan = document.createElement('span');
weekSpan.className = 'week';
weekSpan.innerText = day.toLocaleString('en-US', { weekday: 'short' });

// day set
const daySpan = document.createElement('span');
daySpan.className = 'day';
daySpan.innerText = day.getDate();

dayElement.dataset.dateKey = dateKey;
dayElement.addEventListener('click', () => {
selectDate(dayElement);
selectedDate = dateKey;
datePicker.value = dateKey;
loadTasks(dateKey);
});

Choose a reason for hiding this comment

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

dayElement라는 li 요소에 리스너가 추가되어있는데, 이번에 배운 버블링 개념을 적용해보면 좋을 것 같아요. 상위 요소인 ul (weekDays)에 이벤트를 위임하면 li 개수만큼 리스너를 여러 번 할당할 필요 없이 한 번만 추가하여 쓸 수 있으니까요! 참고하면 좋을 만한 링크 달아두겠습니다 ☺️

https://myeongsu0257.tistory.com/76
https://gobae.tistory.com/143

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.

오 버블링 개념을 바로 활용하시는 부분이 정말 뛰어난 것 같습니다. ul에만 리스너를 한번 할당하게 된다면 메모리, 유지보수, 조작비용 측면에서 많은 이점이 있음을 알게 되었습니다. 지원님 코드에서의 closest을 활용해서 수정하는 방안으로 해보겠습니다. 감사합니다.


// Today selected set
if (dateKey === selectedDateObj.toISOString().split('T')[0]) {
dayElement.classList.add('selected');
// selectedDate = dateKey;
loadTasks(selectedDate);
}

// 개별 스타일을 위한 span tag 삽입
dayElement.appendChild(weekSpan);
dayElement.appendChild(daySpan);
weekDays.appendChild(dayElement);
}
}
// 선택된 날짜 표시
function selectDate(dayElement) {
const selected = document.querySelector('.selected');
if (selected) {
selected.classList.remove('selected');
}
dayElement.classList.add('selected');
}

// load To Do List
function loadTasks(dateKey) {
taskList.innerHTML = ''; // 기존 할 일 목록 지우기
const tasks = JSON.parse(localStorage.getItem(dateKey)) || []; // LocalStorage에서 해당 날짜의 할 일 가져오기

tasks.forEach((task, index) => {
addTaskToDOM(task, index);
});
}


// Add To Do List -> DOM에 추가 -> 화면에 띄우기용
function addTaskToDOM(task, index) {
const taskElement = document.createElement('article');
taskElement.className = 'task';
Copy link

@Programming-Seungwan Programming-Seungwan Sep 7, 2024

Choose a reason for hiding this comment

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

task라는 클래스명도 참 좋은데, 오히려 task-item이라는 것으로 네이밍하면 더 semantic하게 나중에도 잘 이해할 수 있지 않을까 싶어요🙃

taskElement.innerHTML = `
<div class="task-detail">
<span class="task-time">${task.time}</span>
<h3 class="task-title">${task.title}</h3>
<p class="task-desc">${task.desc}</p>
<button class="task-delete" data-index="${index}">delete</button>
</div>
`;
Comment on lines +130 to +137

Choose a reason for hiding this comment

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

새로운 todo를 innerHTML 문자열로 삽입해주셨네요! 그 과정에서 문자열 리터럴에 변수도 잘 활용해주셨구요.

하지만 innerHTML 방식은 XSS(cross site scripting) 공격에 취약할 수 있다고 해요. 이 방식 말고도 다소 코드가 길어질 여지는 있지만, createElement로 요소들을 더 만든 뒤, appendChild() 메서드를 사용하는 방법도 고려해봄직 할 것 같아요.

물론 innerHTML은 기존의 DOM 요소를 갈아끼우는 방식으로, 제가 추천드린 방법은 기존의 요소에 추가하는 방식으로 동작하여 관련된 고려가 더 필요할 수도 있어요.

찾아보니 replaceChild() 라는 DOM API를 활용할 수도 있군요! 저두 영준님 덕분에 관련 공부를 더 하고 갈 수 있네욥

Copy link
Author

Choose a reason for hiding this comment

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

XSS 공격을 고려했을 때 충분히 문제가 생길 수 있음을 알려주셔서 감사합니다! 요소에 추가하는 부분과 대체하는 부분이 있을 수 있으니 그 부분을 고려하면서 한번 수정해보겠습니다. 알려주신 내용을 공부하다 textContent vs innerText의 차이도 알게 되었습니다. 파생되는 지식들이 재미를 더 붙여주는 것 같아요!

taskElement.querySelector('.task-delete').addEventListener('click', function () {

Choose a reason for hiding this comment

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

innerHTML에 요소를 넣어주시고, 그 다음 이벤트 핸들러를 달아주신 것 좋습니다.

제가 QA를 위해 동일한 내용의 todo를 배열에 중복으로 넣고 삭제를 했을 때 알맞은 것이 삭제되나 확인해봤는데, index 값을 처음 DOM에 추가될 때 잘 가지고 있어 제대로 동작하는군요!

이 부분까지 고려하시는 것 너무 좋았던거 같아요. React는 이런 것을 자체적으로 해주므로 너무 편한 기술인 것을 알 수 있는 것 같아요 💪🏼

const confirm = window.confirm('삭제하시겠습니까?');
if (confirm) {
deleteTask(selectedDate, index); // 삭제 버튼 클릭 시 할 일 삭제
}
});
taskList.appendChild(taskElement);
}

// Add Task -> LocalStorage에 추가
function addTaskToLocal(dateKey, task) {
console.log(dateKey);

Choose a reason for hiding this comment

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

불필요한 콘솔 출력문은 삭제해주세요 :)

const tasks = JSON.parse(localStorage.getItem(dateKey)) || [];

// 작성시간
task.createdTime = new Date().toLocaleString('en-US', { year: 'numeric', month: 'long', day: 'numeric', hour: 'numeric', minute: 'numeric', second: 'numeric' });

tasks.push(task);
localStorage.setItem(dateKey, JSON.stringify(tasks));
}

// Delete Task -> LocalStorage에서 삭제 -> 기존 목록 불러오고, 목록에서 삭제 후 삭제된 목록 저장...흠
function deleteTask(dateKey, taskIndex) {
const tasks = JSON.parse(localStorage.getItem(dateKey)) || [];
tasks.splice(taskIndex, 1);
localStorage.setItem(dateKey, JSON.stringify(tasks));
loadTasks(dateKey);
Comment on lines +161 to +164
Copy link

@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.

tasks에 할당된 JSON.parse(localStorage.getItem(dateKey)) || []가 아래쪽의 loadTasks 함수 내에서도 반복되고 있어서 별도의 함수로 만들어 재사용하면 더 용이할 것 같다는 생각이 듭니다!

}

// Modal Button
modalBtn.addEventListener('click', () => {
modalBtn.style.display = 'none';
taskAddForm.style.display = 'flex';
});

// back navigation
backBtn.addEventListener('click', () => {
modalBtn.style.display = 'block';
taskAddForm.style.display = 'none';
});

Choose a reason for hiding this comment

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

리액트의 조건부 렌더링이 그리워지는 순간이네요...! button 컴포넌트는 기본적으로 inline-block으로 동작해서 여기에서는 크게..? block으로 갈 필요는 없을 것 같아요! 어차피 지금 footer 컴포넌트가 flex이고 버튼이 자체적으로 50, 50의 너비와 높이를 가지니까요!


// Save Task -> submit 이벤트가 폼을 자동으로 제출하면서 페이지가 새로고침됨 -> 그렇기 때문에 save button이 아닌
// form에 이벤트리스너를 등록해서 기본동작을 막아야 함.
taskAddForm.addEventListener('submit', function (event) {
event.preventDefault();

const taskTitleInput = document.getElementById('task-title');
const taskDescInput = document.getElementById('task-desc');
const taskTitle = taskTitleInput.value;
const taskDesc = taskDescInput.value;

Choose a reason for hiding this comment

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

Suggested change
const taskTitleInput = document.getElementById('task-title');
const taskDescInput = document.getElementById('task-desc');
const taskTitle = taskTitleInput.value;
const taskDesc = taskDescInput.value;
const taskTitle = document.getElementById('task-title').value ;
const taskDesc = document.getElementById('task-desc').value;

위의 방식으로 코드를 더 간결하게 줄일 수도 있을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

좋은 의견 감사합니다! 다만 알려주신 코드로 진행했을 때 제 코드 밑에 input field 를 ''로 초기화하는 부분이 있는데 const로 taskTitle을 DOM객체가 아니라 value값 즉, 변수를 선언했기 때문에 밑에서 ''로 초기화할 때 const로 선언된 변수를 새로 선언하는 에러가 발생했어서 DOM 객체와 value를 분리시켜 적용했었습니다! 혹시 제 코드 방법이 아니라 다른 방법으로 input field를 초기화할 수 있는 방안이 있다면 심심하실 때 알려주십셔!

Copy link

@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.

추가로 가지신 의문점에 대해 저도 생각 하나 남겨봅니다!
위에서 말씀해주신대로 코드를 간결하게 줄인다면 아래쪽 초기화 부분은

document.getElementById('task-title').value = '';
document.getElementById('task-desc').value = '';

와 같이 DOM 객체에 직접 접근하는 식으로 바로 쓸 수 있을 것 같습니다! 다만 이렇게 되면 반복적으로 DOM을 탐색하게 되어 성능 면에서는 기존대로 한 번 DOM 접근을 통해 저장하는 방식이 규모가 커진 경우에는 좀 더 유리하지 않을까 싶습니다. 물론 여기서는 코드의 가독성을 위해 줄여도 좋겠다고 생각합니다! 초기화 부분에 대해 또 다른 방안이 있다면 저도 무척 궁금하네요 !!

Copy link
Author

Choose a reason for hiding this comment

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

DOM객체에 직접 접근하는 방식도 좋은 것 같습니다! 다른 방법을 찾아보았는데 이번처럼 title, desc 모든 form의 입력필드를 초기화할 때 taskAddForm.reset(); 이 코드처럼 form 내의 모든 요소를 초기화하는 방법도 있는 것 같습니다. 다만 말 그대로 모든 요소를 초기화하는 것이기 때문에 이번처럼 조건이 맞을 때만 사용해야 할 것 같아요!

Copy link

@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.

입력값에 공백문자만 들어갔을 때도 빈칸으로 제출되기 때문에 trim()함수를 적용한 입력값의 존재여부를 판단함으로써 이를 막는 처리를 해주면 더 좋을 것 같습니다! 참고링크 남겨드립니다

Copy link
Author

Choose a reason for hiding this comment

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

세심한 부분까지 알려주셔서 감사합니다. 덕분에 앞으로 예외사항을 더 고려할 수 있을 것 같습니다.


if (selectedDate) { // 날짜가 선택된 경우에만 저장

Choose a reason for hiding this comment

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

일단 DOM이 로드 되고 selectedDate는 날짜가 바뀔 때에도 웬만하면 존재는 해보이는데 따로 조건문으로 빼주신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 이전 selectedDate를 let selectedDate = '';로 선언했을 때를 고려하여 조건문으로 설정했었는데 코드 수정된 후에 변경이 안 되었는 것 같습니다. 지적 감사합니다!

const newTask = {
title: taskTitle,
desc: taskDesc,
time: new Date().toLocaleString('en-US', { hour: 'numeric', minute: 'numeric', second: 'numeric' })
};

Choose a reason for hiding this comment

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

제 코드를 보고 스스로 개선하면 좋겠다고 생각한 부분이 있었는데, 영준님 코드에도 비슷한 부분이 있어 공유드리고자 리뷰합니다.

새로운 테스크를 만들 때, 생성자를 활용하는 방법도 좋을 것 같습니다.
참고자료


// input field init
taskTitleInput.value = '';
taskDescInput.value = '';

addTaskToLocal(selectedDate, newTask);
loadTasks(selectedDate);
modalBtn.style.display = 'block';
taskAddForm.style.display = 'none';
}
});

loadWeekDays(currentDate);
loadTasks(selectedDate);

});
Loading