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주차] 송은수 미션 제출합니다. #6

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

songess
Copy link

@songess songess commented Mar 15, 2024

1주차 미션: Vanilla-Todo

배포링크

todoList

기능구현

  • 오늘 날짜 생성
  • todo 추가, 추가 시 validation 체크 및 enter로 입력
  • todo 삭제
  • todo 완료갯수 추가
  • todo 완료/미완료 여부에 따른 이동
  • localstorage 저장

느낀점

js만을 사용해서 동작을 만드려면 태그부터 속성까지 하나하나 만들어줘야 해서 손이 많이 가는 것 같습니다.

리액트의 state처럼 값이 변하면 리렌더링이 이루어지지 않기 때문에 값이 변할때마다 직접 DOM조작을 해줘야 했습니다.

뿐만 useEffect 등의 빌트인 함수도 없어서 리액트의 필요성을 느꼈습니다.

개선할 점

localstorage에 저장하는 과정에서 todo,done으로 구분을 하여 중복되는 코드가 생겼는데, 객체배열형태로 저장해 isDone변수를 추가해 완료/미완료를 구분하는 방식으로 짜면 중복되는 코드를 줄일 수 있을 거 같습니다.

Key Questions

DOM은 무엇인가요?

브라우저가 이해할 수 있는 자료구조로 프로퍼티와 메서드를 제공하는 트리 자료구조입니다.

렌더링 엔진이 HTML 문서를 파싱하면 문서, 요소, 속성, 텍스트등 모두 노드객체로 변하고 이 노드 객체들로 구성된 트리 자료구조를 DOM 이라 합니다.

js를 사용해 DOM을 조작하여 html의 구조를 변경합니다.

HTML (tag) Element를 JavaScript로 생성하는 방법은 어떤 것이 있고, 어떤 방법이 가장 적합할까요?

  1. createElement
const todoCard = document.createElement('div');
  1. insertAdjacentHTML
element.insertAdjacentHTML(position, '<div class="todoCard"></div>');

태그요소를 만들때는 만드는 거에만 집중하고, 위치를 정하는 것은 나중에 정하는 게 가독성이 좋아보여
createElement방식이 더 적합하다고 생각합니다.

Semantic tag에는 어떤 것이 있으며, 이를 사용하는 이유는 무엇일까요?

시맨틱 태그는 말 그대로 의미가 담긴 태그로, header, nav, section, article, aside, footer 등이 있습니다.

코드를 읽기 쉽게 해줍니다. 내가 짠 코드는 나만 읽는 게 아니라 같은 팀원도 읽고, 시간이 지나면 나도 내가 짠 코드를 기억하지 못할 수 있기 떄문에 가독성 좋은 코드가 필요하고, 시맨틱 태그는 코드의 가독성을 높이는데 도움을 줍니다.

또한 SEO를 향상시킵니다. 검색엔진이 검색을 할 때, 브라우저가 소스코드를 읽을 때 도움을 줍니다.

Flexbox Layout은 무엇이며, 어떻게 사용하나요?

반응형 웹사이트를 만들 때 주로 사용합니다. 뷰포트의 크기에 따라 요소들이 자리를 잡고, 뷰포트의 움직임에 맞춰 반응형으로 위치가 조정됩니다.

display: flex를 부모요소에 적용합니다.

flex-direction을 통해 방향을 결정합니다. 기본 값은 row이고, row | row-reverse | column | column-reverse를 사용할 수 있습니다.

flex-wrap을 통해 줄바꿈을 적용할지 말지 선택할 수 있습니다.

정렬을 위해 justify-content, align-items를 사용합니다. 각각 축방향, 축의 수직방향으로 정렬을 합니다.

align-self를 사용해 자식 중 하나의 요소만 위치를 조정할 수 있습니다. justify-self는 존재하지 않습니다.

gap을 사용해 자식요소들간의 거리를 조정할 수 있습니다.

flex-grow를 통해 남는 공간을 채울 수 있습니다. 대부분의 경우 width/height속성과 함께 쓰입니다.

flex-shrink를 통해 요소가 수축하지 않게 설정합니다.

flex-basis를 통해 요소의 기본 크기를 설정합니다.

JavaScript가 다른 언어들에 비해 주목할 만한 점에는 어떤 것들이 있나요?

자료형이 미리 정해져 있지 않고 런타임에 동적으로 정해지기 때문에 유연하게 타입이 정해진 다는 것이 장점이고, 웹(브라우저) 뿐만 아니라 nodejs 등 서버환경에서도 사용할 수 있습니다. 또한 다양한 라이브러리, 프레임워크가 있어 원하는 기능을 쉽게 구현할 수 있다는 것도 장점이라 생각합니다.

코드에서 주석을 다는 바람직한 방법은 무엇일까요?

주석을 다는 이유는 다른 사람이 알아보기 위함이고, 유지보수를 보다 쉽게 하기 위해서입니다. 따라서 해당 변수, 함수가 어떤 용도로 사용되고 있는지 이해할 수 있게 달아야 합니다.

개발환경에서도 호흡이 길어지면 내가 하고 있는 것을 적어가면서 하는게 편리하다고 생각하는데, 이런 주석들은 배포이전에 지워줘도 괜찮을 것 같습니다.

Copy link

@Dahn12 Dahn12 left a comment

Choose a reason for hiding this comment

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

디자인도 깔끔하고 저와는 다른 스타일의 코드인것 같아 더 배워가는게 많았습니다! 첫 미션 수고 많으셨습니다! 다음 미션도 화이팅!👍

/>
<button class="addTodo">+</button>
</section>
<footer class="todoFooter">
Copy link

Choose a reason for hiding this comment

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

저는 header과 footer가 일반적인 웹사이트에서만 사용되는거라 생각했었는데, 훨씬 가독성도 좋고 이해하기 편하네요!

let todoCount = document.querySelectorAll('.todoCard').length + doneCount;

//등록 된 todo, 완료 된 todo 개수 표시
const noteTodoProgress = () => {
Copy link

Choose a reason for hiding this comment

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

변수명 같은 경우에는 제 코드리뷰에 달린 부분이기도 한데, count가 들어가 있으면 좀더 직관적으로 이해할 수 있을거 같아요!

todoContent.htmlFor = `todoCheckbox${checkboxNumber++}`;
todoContent.textContent = todo;
todoDelete.className = 'todoDelete';
todoDelete.textContent = 'X';
Copy link

Choose a reason for hiding this comment

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

이부분은 버튼 역할인 듯 한데 button 태그를 쓰면 좀더 편했을거 같아요!

/*
checkboxNumber: todoCard가 unique한 id를 갖기 위한 변수
*/
let checkboxNumber = 0;
Copy link

Choose a reason for hiding this comment

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

각각의 카드에 대해 다른 디자인이나 요소가 들어가는게 아닌 이상 id를 부여하지 않아도 될 듯합니다! 하지만 만약 복잡하게 관리해야하는 프로젝트에서는 좋은 방법이 될거 같아요!

};
noteTodoProgress();

//오늘 날짜 표시
Copy link

Choose a reason for hiding this comment

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

이 함수가 제일 상단에 위치하거나 부수적인 부분이라 느껴지신다면 하단으로 가는게 가독성이 더 좋았을거 같아요!

if (e.target.className === 'todoDelete') {
e.target.parentElement.remove();
updateTodoCount();
deleteDoneFromLocalStorage(e.target.previousElementSibling.textContent);
Copy link

Choose a reason for hiding this comment

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

previousElementSibling에 대해 알아갑니다! 리액트의 편리함을 다시 느끼네요..

background-color: #f8f9fa;
}

.wrapper {
Copy link

Choose a reason for hiding this comment

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

container과 wrapper의 차이를 저도 잘 몰라서 이번 기회에 찾아봤습니다. 이블로그가 도움이 되었으면 좋겠어요!

display: flex;
flex-direction: column;
flex: 1 0;
overflow-y: scroll;
Copy link

Choose a reason for hiding this comment

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

scroll처리까지!👏

overflow-y: scroll;
}

.todoSection span,
Copy link

Choose a reason for hiding this comment

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

span은 기본적으로 inline 속성이 있어서 콘텐츠 사이에 특정한 디자인을 줘야 할 때 많이 사용하는 것으로 알고있습니다! flex속성으로 배치해줘야하는 요소라면 div 태그나 제목임을 표시해 줄 수 있는 h 태그도 괜찮았을거 같아요! 제가 참고한 블로그를 추가해둘게요! span태그

Copy link
Member

@ddhelop ddhelop left a comment

Choose a reason for hiding this comment

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

안녕하세요~~

과제 너무 고생하셨어요. 고민과 시간 투자를 많이 하신 흔적이 보였어요.

코드보면서 배울점이 많았던 리뷰였던 것 같아요.
KEY QUESTIONS도 열심히 하셨네요 😀


//localstorage에 저장된 todo,done 불러오기
const todoList = JSON.parse(localStorage.getItem('todo'));
const donelist = JSON.parse(localStorage.getItem('done'));
Copy link
Member

@ddhelop ddhelop Mar 17, 2024

Choose a reason for hiding this comment

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

변수명은 목적은 잘 설명하고 있으나, camelCase를 사용하여 'doneList'로 사용하면 더 깔끔할 것 같아요.

/*
checkboxNumber: todoCard가 unique한 id를 갖기 위한 변수
*/
let checkboxNumber = 0;
Copy link
Member

Choose a reason for hiding this comment

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

checkboxNumber를 createTodoCard 함수 내에서 관리하도록 변경하면 좋을 것 같아요. 해당 변수는 DOM 요소의 ID를 관리하기 위해 사용되는 것 같은데, 각 TODO 항목을 생성할 때마다 고유 ID를 생성하는 로직으로 대체하면 좋을 것 같아요.
전역변수는 코드의 길이가 길어지면 가독성을 해치고, 버그 발생의 원인이 있다고 하여 되도록이면 함수내에서 쓰는게 좋다고 합니다!

Comment on lines +52 to +72
const addTodoToLocalStorage = (todo) => {
const todoList = JSON.parse(localStorage.getItem('todo'));
localStorage.removeItem('todo');
if (todoList) {
const newTodoList = [...todoList, todo];
localStorage.setItem('todo', JSON.stringify(newTodoList));
} else {
localStorage.setItem('todo', JSON.stringify([todo]));
}
};
const addDoneToLocalStorage = (todo) => {
const doneList = JSON.parse(localStorage.getItem('done'));
localStorage.removeItem('done');
if (doneList) {
const newDoneList = [...doneList, todo];
localStorage.setItem('done', JSON.stringify(newDoneList));
} else {
localStorage.setItem('done', JSON.stringify([todo]));
}
};

Copy link
Member

Choose a reason for hiding this comment

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

addTodoToLocalStorage 함수와 addDoneToLocalStorage함수 그리고 deleteTodoFromLocalStorage와 deleteDoneFromLocalStorage 함수가 매우 유사한거 같아요. 공통 로직을 하나로 합치는 함수를 작성해보면 코드 가독성이나 유지보수에 정말 좋을 것 같습니다.

Suggested change
const addTodoToLocalStorage = (todo) => {
const todoList = JSON.parse(localStorage.getItem('todo'));
localStorage.removeItem('todo');
if (todoList) {
const newTodoList = [...todoList, todo];
localStorage.setItem('todo', JSON.stringify(newTodoList));
} else {
localStorage.setItem('todo', JSON.stringify([todo]));
}
};
const addDoneToLocalStorage = (todo) => {
const doneList = JSON.parse(localStorage.getItem('done'));
localStorage.removeItem('done');
if (doneList) {
const newDoneList = [...doneList, todo];
localStorage.setItem('done', JSON.stringify(newDoneList));
} else {
localStorage.setItem('done', JSON.stringify([todo]));
}
};
const updateLocalStorageList = (key, todo, isDelete = false) => {
let list = JSON.parse(localStorage.getItem(key)) || [];
if (isDelete) {
list = list.filter(item => item !== todo);
} else {
list.push(todo);
}
localStorage.setItem(key, JSON.stringify(list));
};

예를 들면 이런식으로 함수를 수정하면 코드의 양을 줄일 수 있을 것 같아요 :)

Comment on lines +30 to +33
if (isDone) {
document.querySelector('.doneSection').append(todoCard);
} else {
document.querySelector('.todoSection').append(todoCard);
Copy link
Member

Choose a reason for hiding this comment

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

todoSection이나 doneSection에 document.querySelector를 실행하여 매번 DOM 요소에 접근하고 있는 것 같아서, 이는 성능저하에 큰 영향을 끼친다고 합니다!

Suggested change
if (isDone) {
document.querySelector('.doneSection').append(todoCard);
} else {
document.querySelector('.todoSection').append(todoCard);
const todoSection = document.querySelector('.todoSection');
const doneSection = document.querySelector('.doneSection');

이런식으로 DOM요소를 미리 찾아 변수를 저장해서 쓰는 것도 코드의 양과 성능 향상에 도움이 될 수 있을 것 같습니다.

Comment on lines +101 to +119
//오늘 날짜 표시
const today = new Date();
const year = today.getFullYear();
const month = today.getMonth() + 1;
const date = today.getDate();
const day = today.getDay();
const dayList = [
'Sunday',
'Monday',
'Tuesday',
'Wednesday',
'Thursday',
'Friday',
'Saturday',
];
document.querySelector('.headerDate').append(`${date}`);
document.querySelector('.headerMonth').append(`${month}`);
document.querySelector('.headerYear').append(`${year}`);
document.querySelector('.headerDay').append(`${dayList[day]}`);
Copy link
Member

Choose a reason for hiding this comment

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

오늘 날짜 표시하는 로직이 조금 긴 것 같습니다!!

Suggested change
//오늘 날짜 표시
const today = new Date();
const year = today.getFullYear();
const month = today.getMonth() + 1;
const date = today.getDate();
const day = today.getDay();
const dayList = [
'Sunday',
'Monday',
'Tuesday',
'Wednesday',
'Thursday',
'Friday',
'Saturday',
];
document.querySelector('.headerDate').append(`${date}`);
document.querySelector('.headerMonth').append(`${month}`);
document.querySelector('.headerYear').append(`${year}`);
document.querySelector('.headerDay').append(`${dayList[day]}`);
const today = new Date();
const dayList = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday'];
function updateHeader(selector, content) {
document.querySelector(selector).append(content);
}
updateHeader('.headerDate', today.getDate());
updateHeader('.headerMonth', today.getMonth() + 1);
updateHeader('.headerYear', today.getFullYear());
updateHeader('.headerDay', dayList[today.getDay()]);

document.querySelector(...).append(...) 호출을 줄이고,
querySelector와 append를 결합하는 호출을 하나의 함수로 만들어 재사용하면 더 가독성이 좋아질 것 같습니다 !!

@@ -1 +1,166 @@
/* 본인의 디자인 감각을 최대한 발휘해주세요! */
@import url('https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/variable/pretendardvariable.css');
Copy link
Member

Choose a reason for hiding this comment

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

@import를 사용하여 외부 CSS를 가져오는건 페이지 로딩 시간에 영향을 줄 수 있다고 해요!
그래서 태그를 사용하여 CSS를 불러오는 것이 병렬 다운로드가 가능하여 성능에 더 유리하다고 해요 :)

Comment on lines +7 to +11
font-family: 'Pretendard Variable', Pretendard, -apple-system,
BlinkMacSystemFont, system-ui, Roboto, 'Helvetica Neue', 'Segoe UI',
'Apple SD Gothic Neo', 'Noto Sans KR', 'Malgun Gothic', 'Apple Color Emoji',
'Segoe UI Emoji', 'Segoe UI Symbol', sans-serif;
background-color: #f8f9fa;
Copy link
Member

Choose a reason for hiding this comment

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

전역에 margin, padding, box-sizing 설정하는 것 좋아요! 그치만 font-family, background-color 속성은 모든 요소에 상속이 되서 나중에 프로젝트 크기가 커지면 다른 배경색이나 다른 글꼴이 필요해지는 경우가 많아서, 구체적인 요소에 설정하는 것이 좋을 것 같아 보여요!

Comment on lines +101 to +107
.todoCard:hover,
.doneCard:hover,
.todoCheckbox:hover,
.todoContent:hover {
background-color: #e9eaea;
cursor: pointer;
}
Copy link
Member

Choose a reason for hiding this comment

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

todoCard:hover, .doneCard:hover, .todoCheckbox:hover, .todoContent:hover에서 동일한 background-color를 중복 적용하고 있어서.,

공통 스타일을 .todoCard:hover, .doneCard:hover에만 적용하고, 내부 요소는 상속을 통해 호버 효과를 받도록 하면 코드의 중복을 줄이고, 유지보수에 용이할 것 같습니다.

Copy link

@geeoneee geeoneee left a comment

Choose a reason for hiding this comment

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

은수님 안녕하세요! 프론트엔드 운영진 김지원입니다 😀

선택 구현 요소까지 구현해주셔서 과제 즐겁게 보았습니다.
깔끔한 UI도 돋보였던 것 같아요ㅎㅎ
다른 분들이 리뷰 잘 남겨주셔서 저는 몇 가지만 코멘트 간단히 남겨보았으니 한 번 확인해주세요!

첫 과제 하느라 수고 많으셨어요! 다음 과제도 화이팅!!

const newDoneList = doneList.filter((item) => item !== todo);
localStorage.setItem('done', JSON.stringify(newDoneList));
};

Choose a reason for hiding this comment

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

deleteTodoFromLocalStoragedeleteDoneFromLocalStorage 로직에 겹치는 부분이 많은 것 같아서 다음과 같이 함수를 하나로 합쳐도 좋을 것 같습니다:)

Suggested change
const deleteFunc = (todo, listName) => {
const itemList = JSON.parse(localStorage.getItem(listName));
const updatedList = itemList.filter((item) => item !== todo);
localStorage.setItem(listName, JSON.stringify(updatedList));
};

const todoInput = document.querySelector('.todoInput');

const pushNewTodo = () => {
if (todoInput.value.trim()) {

Choose a reason for hiding this comment

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

trim() 으로 공백 처리해주셨네요 👍🏻


//localstorage에 저장된 todo,done 불러오기
const todoList = JSON.parse(localStorage.getItem('todo'));
const donelist = JSON.parse(localStorage.getItem('done'));

Choose a reason for hiding this comment

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

여기서 localStorage.getItem('todo')와 localStorage.getItem('done') 이 null인 경우를 방지하기 위해 이런식으로 코드 리팩토링하셔도 좋을 것 같습니다~

Suggested change
const donelist = JSON.parse(localStorage.getItem('done'));
const doneList = JSON.parse(localStorage.getItem('done'), '[]');

style.css Outdated

.wrapper {
width: 50%;
margin: 2rem auto;

Choose a reason for hiding this comment

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

반응형 고려해서 rem 사용하신 점 너무 좋습니다~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants