-
Notifications
You must be signed in to change notification settings - Fork 10
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주차] 변지혜 미션 제출합니다. #10
base: master
Are you sure you want to change the base?
Conversation
index.html
Outdated
@@ -8,7 +8,30 @@ | |||
</head> | |||
|
|||
<body> | |||
<div class="container"></div> | |||
<main> | |||
<section class="container"> |
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.
semantic tag 들 적극 사용하는 습관 아주 좋은 것 같습니다!!
todoItem.appendChild(todoText); | ||
document.querySelector(".todo-list").appendChild(todoItem); | ||
|
||
// input 창 초기화 |
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.
printTodoItem 함수에서 한번 더 input 을 불러와서 value를 비워주는 것 보다는 addTodoItem 함수에서 이미 선언한 input이 있으므로 여기서 printTodoItem 을 호출한 후 value 비움 처리를 하는것이 더 효율적이지 않을까 생각합니다 ㅎㅎ
.todo-title { | ||
.top-content { | ||
display: flex; | ||
flex-direction: row; |
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.
display:flex 의 default direction 이 row라 flex-direction 은 생략해줘도 무방할 거 같아요~!!
todoList.js
Outdated
const today = new Date(); | ||
const year = today.getFullYear(); | ||
const month = String(today.getMonth() + 1).padStart(2, "0"); | ||
const day = String(today.getDate()).padStart(2, "0"); |
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.
padStart 함수로 깔끔한 코드 너무 좋은 거 같아요 ㅎㅎ
@@ -5,7 +5,7 @@ | |||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |||
<link rel="stylesheet" href="todo.css" /> | |||
<title>Vanilla Todo</title> | |||
<link rel="icon" href="/img/favicon.ico" /> | |||
<link rel="icon" href="./img/favicon.ico" /> | |||
</head> |
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.
컨테이너 크기를 잡을 때 % 활용하여 반응형을 고려한 부분 너무 좋은 것 같습니다.
폰트사이즈나 버튼 같은 것들의 크기도 잡을 때도 px 도 좋지만 반응형을 고려하여 media-query 및 rem 단위로 작업하는 법도 익히시면 좋을 것 같습니다 ㅎㅎ
const progress = document.querySelector(".progress"); | ||
|
||
const displayForm = () => { | ||
if (form.style.display === "none") { | ||
form.style.display = "flex"; | ||
progress.style.display = "none"; | ||
progressContainer.style.display = "none"; |
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.
form.style.display 는 해당 요소의 inline style 에만 접근 할 수 있어서 처음에 화면 접속 시 버튼을 클릭하면 처음 css 파일에 설정해 준 대로 form.style.display==="none" 이 trigger 될 거라는 기대와 달리, inline style 에 지정해 준 것이 아니므로 else 문이 실행되게 되어 첫 버튼 클릭 시에는 아무런 변화가 일어나지 않습니다.
인라인 style 로 초기 상태를 세팅해 주거나 window.getComputedStyle 메서드에 대해 알아보면 좋을 거 같아요~!!
@@ -11,6 +32,15 @@ | |||
|
|||
.todo-title { | |||
margin: 0; | |||
font-family: "Pretendard-Regular"; |
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.
초기에 요소에 디폴트로 세팅되어있는 margin, padding 등의 값들을 초기화하고 싶다면 reset css 를 검색하여 해당 css를 css 파일 상단에 세팅해준 뒤 작업하는 것도 좋을 것 같아요 ㅎㅎㅎ
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.
안녕하세요
프론트 파트장 김문기입니다~!! 🙌
progress bar를 포함해서 background도 단순 색이 아닌 예쁜 이미지로 넣어주시고 다양한 icon도 사용해서 예쁘게 잘 구현된 것 같아서 고생 많이 했겠다 싶었습니다 ㅠㅠ
일주일동안 과제 하느라 너무 고생많으셨어요!!! 스터디에서 뵐게요~!!!
@font-face { | ||
font-family: "SUITE-Regular"; | ||
src: url("https://cdn.jsdelivr.net/gh/projectnoonnu/[email protected]/SUITE-Regular.woff2") | ||
format("woff2"); | ||
font-weight: 400; | ||
font-style: normal; | ||
} | ||
|
||
@font-face { | ||
font-family: "Pretendard-Regular"; | ||
src: url("https://cdn.jsdelivr.net/gh/Project-Noonnu/[email protected]/Pretendard-Regular.woff") | ||
format("woff"); | ||
font-weight: 400; | ||
font-style: normal; | ||
} |
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 추가 좋아요~!
.progress-bar { | ||
width: 0; | ||
height: 20px; | ||
border-radius: 5px; | ||
background-color: #7adb84; | ||
transition: width 0.3s ease-in-out; | ||
} | ||
|
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.
transition까지 들어간 progress-bar 멋지네요 👍👍👍👍
@@ -0,0 +1,178 @@ | |||
/* Local Storage */ | |||
// Local Storage에 할 일 목록 저장 | |||
const saveTodoListToLocalStorage = () => { |
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에 담는 함수를 제작하신 것 정말 대단한것같아요!
다만, 로직을 볼 때,
- 하나의 item이 추가 / 삭제될 때 마다 모든 리스트를 다시 가져와야한다는 점에서
-> 4, 5번 줄을 함수 밖으로 뺀 후 전역변수로 선언하여 변수의 재사용성을 높일 수 있을 것 같습니다 - 추가 / 삭제될 하나의 리스트만 이동하면 된다는 점에서
-> 7, 8번 줄을 전역변수(변경 가능한 let)로 선언하고, 필요할때마다 추가 / 삭제하면서 사용하면 map의 사용을 줄일 수 있을 것같아요
const addTodoItem = () => { | ||
event.preventDefault(); | ||
// input에 입력한 value를 선택하여 todoContent에 대입 | ||
const todoContent = document.querySelector(".todo-input").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.
todoItem.appendChild(todoText); | ||
todoItem.appendChild(todoDel); |
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.
todoItem.appendChild(todoText); | |
todoItem.appendChild(todoDel); | |
todoItem.append(todoText, todoDel); |
와 같이도 적을 수 있어요~!
// li에 item 추가 | ||
todoItem.appendChild(todoText); | ||
todoItem.appendChild(todoDel); | ||
document.querySelector(".todo-list").appendChild(todoItem); |
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.
아래 현민님께서 잘 적어주셨는데, 코드 전체적으로
`document.querySelector(".todo-list")가 반복되는 것 같아서,
js 코드 내에서 이를 변수에 저장 후 변수로 불러와서 사용해도 좋을 것 같습니다
const printDoneItem = (text) => { | ||
const doneItem = document.createElement("li"); | ||
const doneText = document.createElement("span"); | ||
const doneDel = document.createElement("button"); | ||
|
||
doneText.innerText = text; | ||
doneText.className = "done-item-text"; | ||
doneText.addEventListener("click", toggleDoneToDo); | ||
|
||
doneDel.className = "delete-button"; | ||
doneDel.addEventListener("click", deleteDoneItem); | ||
|
||
doneItem.className = "list-item"; | ||
doneItem.appendChild(doneText); | ||
doneItem.appendChild(doneDel); | ||
|
||
document.querySelector(".done-list").appendChild(doneItem); | ||
}; |
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.
이 부분은 printTodoItem과 같은 로직으로 작동하는 것 같아서,
은비님의 코드리뷰에 적어놓은 부분 처럼 로직을 합쳐도 괜찮을 것 같아요
const updateItemCount = () => { | ||
const todoCount = document.querySelectorAll(".todo-item-text").length; | ||
const doneCount = document.querySelectorAll(".done-item-text").length; | ||
const totalCount = todoCount + doneCount; | ||
const countText = `${doneCount} / ${totalCount}`; | ||
progress.innerText = countText; | ||
|
||
let ratio = 0; | ||
if (totalCount !== 0) { | ||
ratio = doneCount / totalCount; | ||
} | ||
progressBar.style.width = `${ratio * 100}%`; | ||
}; |
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.
progressBar까지 업데이트 해주시는 부분이 정교하게 잘 작성된것같습니다 🔥🔥🔥
.todo-list, | ||
.done-list { | ||
list-style: none; | ||
margin: 0 0 10px; | ||
padding: 0px 30px 0px 10px; | ||
} |
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.
이 부분에 height값을 정해준 후(max-height도 좋아요) overflow-y: scroll을 통해 scroll으로 구현을 하면
리스트가 너무 길어져서 화면 밖으로 나가는 문제를 해결할 수 있을 것 같아요!
못생긴 스크롤바를 커스텀하는 부분은 링크 달아놓을게요
스크롤바 커스터마이징
const today = new Date(); | ||
const year = today.getFullYear(); | ||
const month = String(today.getMonth() + 1).padStart(2, "0"); | ||
const day = String(today.getDate()).padStart(2, "0"); |
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.
padStart로 날짜 표시할 때 0 붙여주는 거 좋은 것 같아요! 배워갑니다 ㅎㅎ
<section class="top-content"> | ||
<h2 class="todo-title">TODOLIST</h2> | ||
<span class="date"></span> | ||
</section> |
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.
저는 거의 div로 처리했는데 semantic tag 다양하게 사용하신게 더 알아보기 좋은 것 같아요!
progressBar.style.width = `${ratio * 100}%`; | ||
}; | ||
|
||
const init = () => { |
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.
저는 초기 렌더링 시 실행되는 함수를 그냥 하단에 모아놨는데 이렇게 init함수로 따로 모아두는 게 유지보수할 때 좋을 것 같아요!
const loadTodoListFromLocalStorage = () => { | ||
const todoList = localStorage.getItem("todoList"); | ||
if (todoList) { | ||
const todoItems = JSON.parse(todoList); |
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, todoItems가 같은 역할을 하는 변수같은데 let으로 선언해서 파싱하고 업데이트 해줘도 좋을 것 같아요~!
1주차 미션: Vanilla-Todo
안녕하세요! 18기 프론트 변지혜입니다😄
조금은 빡빡하게 작성했지만... 그래도 최선을 다했습니다.
안좋은 습관이 많이 들어있다는 걸 다시금 체감했네요...ㅎㅎ
React의 소중함을 깨닫기도 하고, 잊고 있던 JS 지식도 되돌아보는 기회가 된 것 같습니다.
정말 최근에 마크다운 언어 쓰는 법을 익혀서 이것저것 많이 써봤는데요!☺️
개발하는 것만큼 열심히 이것저것 썼다만 너무 의욕만 앞선 것 같기도 합니다.
읽기에 불편하지 않기를 바라며... PR 보냅니다
Key Questions
DOM은 무엇인가요?
HTML (tag) Element를 JavaScript로 생성하는 방법은 어떤 것이 있고, 어떤 방법이 가장 적합할까요?
Semantic tag에는 어떤 것이 있으며, 이를 사용하는 이유는 무엇일까요?
필수 요건 설명
배포 링크
CSS의 Flexbox의 경우,
grid를 딱... 한 번 쓰긴 했지만...그 외에는 모두 flex 사용했습니다!html, css, js로 코드 작성하였습니다.
제 나쁜 습관 중 하나가 commit을 뜸하게 날린다는 것인데요..!
초반에는 노력했으나 후반부 타임어택 이슈로 듬성한 commit을 남기게 되어 조금은 아쉽습니다🥲
기능 설명
(볼륨이 작아서 뺄까 고민했지만 그래도 적어봅니다...)
아쉬운 점 + 더 구현하고 싶은 기능
아쉬운점
코드를 더 효율적으로 써야할 것 같습니다.
기능 구현이 최우선이라는 마인드를 버리고 싶었지만... 해당 마인드가 여전히 남아있는 코드들입니다ㅜㅜ
앞으로는 처음 짤 때부터 어떤 방식이 효율적일까 좀 더 고민하고서 써내려가고 싶습니다.
todo, done 파트를 나눠 일일이 함수를 만들었는데, 이를 하나로 묶은 경우를 전 기수 코드에서 보고 많이 반성했습니다..🥺
css 같은 경우도 background img 설정은 한 줄로 묶을 수 있는 게 많았는데 놓쳐서 아쉽습니다.
애매한 반응형... ( 글씨 크기!!)
최대한 px 단위로 지정하지 않으려 노력했지만... 그리고 이런저런 다양한 꼼수들을 써봤지만 반응형은 쉽지 않네요.
특히 글자 크기도 %나 다른 단위를 사용했으면 좋았겠다는 생각이 많이 듭니다.
보신다면 150% 추천, 적어도 100% 이상으로 봐주시면 감사하겠습니다..ㅎㅎ
사용자 편의성?
삭제 버튼을 원래는 hover 시에 뜨는 게 아니라 떠있는 상태로 디자인해서인지 text와 꽤 떨어져있는데요.
정렬되어 있는 깔끔함을 선택할까 편의성을 고려할까 하다가 일단은 원래대로 해놓았습니다.
삭제 버튼 크기를 줄여서 깔끔하게 뜨되 편하게 누를 수 있는 위치에 놓았어도 좋았을 것 같습니다.
더 구현해보고 싶은 기능
progress 상태에 따라 전체 style color 변경
원래 처음 하고 싶었던 것은 progress 에 따라 border, list item 등 전체적인 style color가 그라데이션으로 바뀌는 것을 해보고 싶었어요. 그래서 너무 React만 썼었는지... progress 비율에 따라 색을 지정하는 함수를 만들고, 이를 props
(여기부터 vanilla로는 글렀다고 생각하긴 했네요...)로 전달하면 가능하지 않을까! 싶었습니다. 하지만 가뜩이나 시간도 부족해서 이를 구현할 시간이 택도 없었기에..^^ 정해진 색으로 우선 제출해보았습니다.날짜 부분 달력 형태로 나타내기
localstorage 값에 날짜에 따른 todolist를 넣어보면 어떨까 싶기도 했는데요. 그러면 날짜별로 list가 달라져서 이용하기가 불편할 것 같다는 생각이 들어... 달력 형태로 하되 드래그 앤 드롭 기능으로 item들 옮길 수 있으면 좋지 않을까 생각만 해봤습니다...ㅎㅎ 그래도 디자인적인 면에서 달력으로도 나타낼 수 있으면 좋을 것 같습니다!
링크 및 참고자료
로컬에서 만든 branch 안 보일 때
Git add, commit, push 취소하기
배경 이미지 관련 CSS
이건 자료는 찾아봤지만 시간과 디자인 감각 부족으로 실패한..^^ 달력 관련 자료입니다!
자바 스크립트를 이용한 달력 만들기