-
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주차] 권혜인 미션 제출합니다 #6
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,8 @@ | |||
//헤더에 해당되는 날짜 상수화 파일입니다. |
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.
이렇게 상수를 따로 모듈화 해서 여러 곳에서의 재사용 성을 높이고 유지 보수를 쉽게 한 것은 매우 좋은 것 같습니다!!! 👍🏻👍🏻
</head> | ||
|
||
<body> | ||
<div class="container"></div> | ||
<header> |
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.
시멘틱 태그의 장점으로 코드 가독성 및 유지보수가 용이하며 검색 엔진에 최적화된다는 것이 있는데 혜인 님의 html은 장점을 잘 살려 시멘틱 태그를 활용해 주신 듯 합니다!
|
||
let newTodoInputContainer = document.createElement("div"); | ||
newTodoInputContainer.className = "newTodoInputContainer"; | ||
newTodoInputContainer.append(newTodoInputEmoji, todoInput, confirmBtn); |
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.
아래에는 appendChild를 쓰신 부분이 있던데 append와 appendChild의 차이점 (append는 노드 객체와 문자열 모두 추가할 수 있는 반면, appendChild는 노드 객체만 추가 가능하다 & append는 한번에 여러개의 자식 노드를 추가할 수 있지만, appendChild는 한번에 한 개만 가능하다.) 을 잘 이해하고 계신 듯 합니다! 👍🏻
저는 기존에 appendChild만 알고 있었고 그것만 사용했는데... 제가 수정이 필요할 것 같네용 😂
@@ -0,0 +1,48 @@ | |||
import { todosWrapper } from "../constants/document.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.
이렇게 기능별 함수를 모듈화 해서 사용하신 것도 위의 상수 모듈화와 동일하게 유지보수에 강점을 가지고 있는 것 같습니다 👍🏻
//확인 눌렀을 때 | ||
function onClickConfirmBtn() { | ||
confirmBtn.addEventListener("click", (e) => { | ||
let todoId = Math.random(); |
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.
고유 id 생성을 위해 Math.random()을 사용하셨는데, 이 방식은 완전히 고유성을 보장하지 않기 때문에 같은 ID가 생성될 가능성이 (아주 낮지만) 존재하며, 이런 경우 기능이 의도한 대로 작동하지 않을 수 있다고 알고 있습니다!
UUID나 Date.now()를 활용하는 방식 등 더 고유성을 보장하는 방법을 사용하는 것으로 개선하는 것은 어떨까요?!
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.
고유 id 생성을 위해 Math.random()을 사용하셨는데, 이 방식은 완전히 고유성을 보장하지 않기 때문에 같은 ID가 생성될 가능성이 (아주 낮지만) 존재하며, 이런 경우 기능이 의도한 대로 작동하지 않을 수 있다고 알고 있습니다!
UUID나 Date.now()를 활용하는 방식 등 더 고유성을 보장하는 방법을 사용하는 것으로 개선하는 것은 어떨까요?!
의견감사합니당
todoContent.textContent = todoValue; | ||
|
||
doneBtn.addEventListener("change", () => { | ||
if (doneBtn.checked) { |
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를 사용해 보았는데 todo를 하나 생성하고 완료 표시를 누른 뒤 새로 고침을 하면 체크 표시가 초기화 되더라고요 😂😂
완료 상태 또한 localStorage에 저장해서 새로 고침되어도 상태가 유지될 수 있게 하는 방식은 어떨까요?! 그러기 위해서는 아이디와 값만을 저장하는 것이 아니라 배열 형태로 list를 저장하는 것을 추천 드립니다...!!
(더 좋은 방법이 있는 것 같다면 공유 부탁 드립니다 👍🏻)
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.
헐
진짜
생각도
못한
이슈네여
ㅋㅎㅋㅎ,,,,
진짜 완료 표시 local저장 생각지도 못했던,,, 아 감사합니다!!하하하하,,,
deleteBtn.textContent = "🗑️"; | ||
//삭제 | ||
deleteBtn.addEventListener("click", () => { | ||
onetodoWrapper.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.
삭제 버튼 클릭 시 onetodoWrapper.remove()를 호출하셨는데, onetodoWrapper 변수가 선언되기 전에 사용하신 것을 보아 JS의 호이스팅을 이용하신 것 같습니당
그러나 let, const 변수는 선언 전에 사용하려고 하면 참조 오류가 발생한다고 알고 있습니다 그러니 변수를 선언한 후 사용하도록 위치를 조정하는 것은 어떨까요?!
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.
+) 추가로, todo 삭제 시 alert나 'confirm('이 할 일을 삭제하시겠습니까?')' 등을 사용해 사용자에게 알리는 것은 어떨까용?!
이 점은 저도 수정하려고 합니다! 저는 단순히 alert만으로 삭제한다는 것을 알려주기만 하고 '취소' 선택지를 두지 않았었는데 confirm을 사용해 사용자가 선택할 수 있게 하면 더 좋을 것 같습니다!! 👍🏻👍🏻
font-size: 20px; | ||
} | ||
|
||
@media (min-width: 768px) and (max-width: 1023px) { |
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.
저는 과제 제출 전 반응형을 적용하지 못했어서...😔 미디어쿼리를 이용해 반응형을 구현하신 것 정말 좋은 것 같습니다!! 👍🏻👍🏻
다만, px을 조금 더 세세하게 나누어 보는 것은 어떨까요?! 현재는 화면의 전체 크기에 따라 조금 확확 바뀌는 감이 있는 것 같습니다!!
bottom: 1.2rem; | ||
} | ||
|
||
.newTaskBtn:hover { |
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.
위의 확인 버튼과 마찬가지로 버튼 hover 시 확실한 디자인의 변화를 줘서 완전 버튼을 누르는 것 같은 효과를 받았습니다 👍🏻👍🏻
저는 그저 배경화면의 색과 글자 굵기만 바뀌게 했는데 그림자 색까지 바뀌니 훨씬 입체적입니다!
} | ||
|
||
//확인 눌렀을 때 | ||
function onClickConfirmBtn() { |
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.
현재 todoInput.value에 대한 유효성 검사가 없어서 빈 입력값도 저장되고 있습니다 😂
입력 값이 비어있는 경우 할 일이 추가되지 않도록
if (todoInput.value.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.
+) 확인 버튼을 클릭해야만 새로운 todo가 생기는 것이 아니라 enter 키를 눌렀을 때도 새로운 todo가 생기도록 추가하면 어떨까요!?
제가 혜인 님의 todolist를 테스트하면서 습관적으로 enter을 치고 있더라고요 😂😂 저와 같은 사용자를 위해 enter로도 todo가 추가되도록 todoInput 요소에 keydown 이벤트 리스너를 추가하여 엔터 키를 감지하고, 엔터 키가 눌렸을 때 새로운 todo를 추가하는 함수를 호출하는 방식을 추천 드립니다...!!! (더 나은 방식이 있다면 채택 후 공유 부탁드립니다 👍🏻👍🏻👍🏻)
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.
저는 input, button 태그를 form으로 감싼 다음 click
대신에 submit
이벤트를 이용합니다!
대신에 submit이 일어나면 브라우저가 새로고침되기 때문에 이를 원하지 않으면 event.preventDefault()
메소드를 호출해주셔야 해요!
@@ -0,0 +1,151 @@ | |||
.doneBtnContainer { |
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 파일도 역할별로 모듈화 하니 유지보수에 정말 좋을 것 같습니다!! 👍🏻
저는 하나의 css 파일에 모든 디자인 요소를 모아두는 바람에 항상 ctrl F를 사용했었는데 ㅠㅠ 다음에는 저도 모듈화에 좀 더 힘을 써야겠다고 느껴지네용 👍🏻👍🏻
transform: rotate(-45deg); | ||
} | ||
|
||
.doneBtnContainer input[type="checkbox"]:checked + .doneBtnLabel, |
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.
우선 전체적으로 파일을 쪼개주셔서 읽기 너무 편했어요👍👍👍
그리고 시간이 없어서 급하게 하셨다고 했는데, localStorage 연결까지 해 주셔서 깜짝 놀랐어요!! 애니메이션도 열심히 만드신 것 같아서 인상깊었습니당🫶🫶 너무 고생하셨어요!!
궁금한 점이 있는데, 저는 type="module"을 이용하면 CORS 에러가 발생해서 export를 이용하지 못했거든요...ㅠㅠ 혹시 따로 설정하거나, 실행 환경을 어떻게 쓰시는지 여쭤봐도 될까용?!😭😭😭
export const todayMonth = currentDate.getMonth() + 1; | ||
export const todayDAY = currentDate.getDate(); |
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.
여기 todyaDAY
만 컨벤션에 어긋나 있네요!
그리고 날짜를 다룰 때는 date를 일, day를 요일로 쓰는 경우가 많아서 혼동할 가능성이 있을 것 같아요.
또, today+month나 today+day라는 조합이 어색해 보여요. 저라면 currentDate
를 currentDateObject
로 바꾸고 달, 일을 currentMonth
, currentDate
라고 쓸 것 같아요!
@media (min-width: 768px) and (max-width: 1023px) { | ||
* { | ||
font-size: 50%; | ||
} | ||
} | ||
@media all and (max-width: 767px) { | ||
* { | ||
font-size: 40%; | ||
} |
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.
여기에서 font-size
를 지정해 주신 이유가 따로 있을까요? px
과 rem
단위를 혼용해서 사용하고 계신데, 이로 인해서 예상치 못한 레이아웃 문제가 발생할 수 있을 것 같아요. html의 font-size의 변화에 따라서 rem의 크기는 변하지만 px의 크기는 변하지 않아서 디자인의 일관성이 깨질 수도 있으니까요! 단위 사용을 정리해보는 건 어떨까요?
@@ -0,0 +1,17 @@ | |||
//new Task를 눌렀을 때 나타나는 task input | |||
const newTodoInputEmoji = document.createElement("p"); | |||
newTodoInputEmoji.textContent = "✨"; |
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.
여기에서 textContent
를 써주신 부분이 좋았어요👍👍 저는 innerText
를 썼는데, 검색해보니 innerText
는 스타일 재계산이 일어나지만, textContent
는 스타일 재계산이 일어나지 않으므로 간단한 텍스트 업데이트에는 textContent
가 더 성능이 좋다고 합니다! 저두 textContent
로 바꿔야겠어요🫶🫶
|
||
//element 추가 함수 | ||
export function handleNewTask(todoId, todoValue) { | ||
let doneBtnContainer = document.createElement("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.
제가 보기에는 doneBtnContainer
객체의 property가 수정되는 일은 있지만, 다른 객체로 재할당되는 일은 없는 것 같아요. 아래의 doneBtn
, doneBtnLabel
등의 객체들도 const
로 선언하셔도 될 것 같아요!
handleDeleteStoreTask(todoId); | ||
}); | ||
|
||
let onetodoWrapper = document.createElement("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.
저는 onetodoWrapper
라는 변수 이름을 이해하기 어려웠어요🥹 one + todo + wrapper의 조합이라고 생각하고 코멘트 드리겠습니다...!
보통 단일 아이템은 todoItem
, todoListItem
등으로 사용하고(wrapper라기보다는 그냥 아이템 그 자체인 것 같아서 wrapper는 뗐어요), 이 li
아이템들을 모은 ul
을 todoList
정도로 사용하는 것 같아요. 이 부분은 혜인님 취향에 맞게 찾아보시고 사용하면 좋을 것 같아요!
|
||
//각 날짜 불러오기 | ||
export function headerInsertDate() { | ||
headerToday.append(todayMonth, "월", " ", todayDAY, "일"); |
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.
여기에서는 텍스트를 하나하나 append
해주는 방식으로 날짜를 알려주셨는데, 저라면 템플릿 리터럴을 사용해서 이렇게 작성해볼 것 같아요!
headerToday.textContent = `${todayMonth}월 ${todayDAY}일`
템플릿 리터럴을 이용하면 어디에 변수가 들어갈 지 알기 쉬워서 좋은 것 같아요😘
} | ||
|
||
//확인 눌렀을 때 | ||
function onClickConfirmBtn() { |
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.
저는 input, button 태그를 form으로 감싼 다음 click
대신에 submit
이벤트를 이용합니다!
대신에 submit이 일어나면 브라우저가 새로고침되기 때문에 이를 원하지 않으면 event.preventDefault()
메소드를 호출해주셔야 해요!
export function showTask() { | ||
if (localStorage.length > 0) { | ||
for (let i = 0; i < localStorage.length; i++) { | ||
const key = localStorage.key(i); | ||
const value = localStorage.getItem(key); | ||
if (value) { | ||
handleNewTask(key, 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.
- 우선 저는 이렇게
localStorage <= 0
일 때의 동작이 정의되지 않은 경우,
if (localStorage.length === 0) {
return;
}
for (let i = 0; i < localStorage.length; i++) {
...
}
처럼 early return을 하려고 해요! 코드의 depth를 줄일 수 있어서 과도한 들여쓰기를 피할 수 있거든요!! 물론 상황에 따라서 장단점이 있으니 한번 알아보시면 좋을 것 같아요!!
- 지금은
for
문을 이용해서 반복문을 사용하고 계신데요, 물론for
문도 좋지만, 저는forEach
가 어떤 일을 하는지 더 명확하게 알아볼 수 있다고 생각해요. 이 부분도 한번 고려해보세요!
Object.keys(localStorage).forEach(key => {
const value = localStorage.getItem(key);
if (value) {
handleNewTask(key, value);
}
});
.doneBtnContainer input[type="checkbox"]:checked + .doneBtnLabel::after, | ||
.doneBtnContainer .doneBtnLabel.checked::after { | ||
height: calc(var(--checkbox-height) / 2); | ||
-moz-animation: dothabottomcheck-19 0.2s ease 0s forwards; | ||
-o-animation: dothabottomcheck-19 0.2s ease 0s forwards; | ||
-webkit-animation: dothabottomcheck-19 0.2s ease 0s forwards; | ||
animation: dothabottomcheck-19 0.2s ease 0s forwards; | ||
} | ||
.doneBtnContainer input[type="checkbox"]:checked + .doneBtnLabel::before, | ||
.doneBtnContainer .doneBtnLabel.checked::before { | ||
height: calc(var(--checkbox-height) * 1.2); | ||
-moz-animation: dothatopcheck-19 0.4s ease 0s forwards; | ||
-o-animation: dothatopcheck-19 0.4s ease 0s forwards; | ||
-webkit-animation: dothatopcheck-19 0.4s ease 0s forwards; | ||
animation: dothatopcheck-19 0.4s ease 0s forwards; |
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.
이렇게 before
, after
로 체크표시를 만들고 애니메이션까지 넣는건 처음 봐요!! 짱신기하네용...👍
✅ 구현 링크
1주차 미션: Vanilla-Todo
서론
https://hae2nitodo.vercel.app/
✅ 미션
🎏 미션 목표
🫤 Key Questions
정리한 블로그 링크입니다!
https://velog.io/@hae2ni/CEOS-1주차-DOM-이벤트흐름-클로저스코프
1️⃣ DOM은 무엇인가요?
2️⃣ 이벤트 흐름 제어(버블링 & 캡처링)이 무엇인가요?
3️⃣ 클로저와 스코프가 무엇인가요?
🌹 필수 요건
🌷 선택 요건
🥀 느낀점
뼈대나 구조를 먼저 그리고 어떻게 개발할지를 생각하고 개발하는게 얼마나 중요한지 알면서도,
섣부른 마음에 일단 시도했던 것 같아요
그러다 보니 더 시간이 오래 걸리고 예상치도 못한 곳에서 에러가 터지더라구요
기본적인 자바스크립트 내용도 어렵게 느껴지고 더 공부해야할 게 확실히 많구나 많이 부족하구나라는 걸
뼈져리게 느꼈습니다ㅠㅠ!!
너무 아쉬웠고, 얼마나 기본기가 중요한지 더욱 경험으로 알게 되었습니다.
이래나 저래나 디자인도 많이 아쉽습니다...ㅎㅎ