-
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주차] 이가빈 미션 제출합니다. #4
base: master
Are you sure you want to change the base?
Conversation
const checkedStatus = todo.completed ? 'checked' : 'unchecked'; | ||
const textColor = todo.completed ? '#C0C0C0' : '#000000'; |
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.
가빈님 멋진 과제 잘 봤습니다!!
코드 너무 깔끔하게 작성하시고, 주석처리도 깔끔해서 보기 너무 편했습니다.
추가 기능까지 구현했는데도 코드가 깔끔해서 놀랐어요. 부러워요..🥹
고생하셨습니다!!
box-shadow: 0 4px 8px rgba(0, 0, 0, 0.1); | ||
flex-grow: 1; | ||
margin: 0 20px; | ||
} |
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.
가빈님의 코드를 보면서, 변수나 함수 이름 하나하나에 신경을 쓰면서 코드를 짜신 것 같은 느낌이 들었어요.
또한, loadTodoList 함수를 작성하실 때도, 로직을 짜실 때 깊은 고민을 하신게 느껴져요.
결과적으로 가독성 좋은 깔끔한 코드가 나온 것 같아요!
제가 언급했던 매번 foreach 문 내에서 각각의 할일 하나씩 dom에 추가하는 부분만 최적화 하신다면 더욱 더 완벽한 코드가 될 것 같아요!! 이번 과제도 정말 수고 많으셨어요!
`; | ||
|
||
// 체크박스 | ||
todoItem.querySelector('.toggleComplete').addEventListener('click', () => { |
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)에 한 번만 이벤트 리스너를 추가한 후에,
e.target을 이용해 클릭된 자식 요소를 판별하여 로직을 진행시키는 방식을 제안드려요!!
} | ||
|
||
// 투두 렌더링 | ||
function loadTodoList(date) { |
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.
loadTodoList 함수를 잘 구현해주셨네요! 함수의 역할에 맞게 명칭을 잘 설정해주신 것 같습니다.
fetch: 데이터를 단순히 가져오는 작업에 집중된 단어로, 데이터를 가져오는 작업만 수행하고, 저장하거나 처리하지 않는 경우에 사용하는 반면,
load는, 데이터를 가져와서 처리하거나 저장하는 작업까지 포함하는 경우에 적합하다고 하더라구요.
가빈님께서 구현해주신 loadTodoList 함수는,
- 로컬 스토리지에서 가져온 투두 리스트를 렌더링하고,
- 완료 상태를 업데이트까지 가능해서, 함수명과 동작이 일치하는 것 같아 좋은 것 같습니다.
지원님 코드리뷰에도 언급했었는데, 함수명이나 변수명을 짓는데 있어 고민되신다면, 카카오 fe 개발자분의 발표 영상 참고해보시면 좋을 것 같아요!
https://www.youtube.com/watch?v=emGLxi0LvNI&t=556s
loadTodoList(date); | ||
}); | ||
|
||
todoListContainer.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.
가빈님의 코드를 보면, todoList에 forEach를 돌려 각각의 할일마다, DOM에 할일을 추가하고 있는 것 같아요.
혹시 리플로우와 리페인팅에 대해 들어보셨나요?
'만약 자바스크립트 코드에 DOM이나 CSSOM을 변경하는 DOM API가 사용된 경우 DOM이나 CSSOM이 변경된다. 이때 변경된 DOM과 CSSOM은 다시 렌더 트리로 결합되고 변경된 렌더 트리를 기반으로 레이아
웃과 페인트 과정을 거쳐 브라우저의 화면에 다시 렌더링한다. 이를 리플로우 reflow , 리페인트 repaint 라 한다.
리플로우는 레이아웃 계산을 다시 하는 것을 말하며, 노드 추가/삭제, 요소의 크기/위치 변경, 윈도우 리사이징 등 레이아웃에 영향을 주는 변경이 발생한 경우에 한하여 실행된다. 리페인트는 재결합된 렌더 트리를 기반으로 다시 페인트를 하는 것을 말한다.
따라서 리플로우와 리페인트가 반드시 순차적으로 동시에 실행되는 것은 아니다. 레이아웃에 영향이 없는 변경은 리플로우 없이 리페인트만 실행된다. '
모던 자바스크립트 deep dive 책을 인용해보았어요.
appendChild()는 dom에 직접적으로 요소를 추가하는 메서드이기 때문에, 할일 요소별로 이 메서드를 호출한다면 이로 인해 리플로우와 리페인팅이 빈번히 발생해서 서능이 저하될 수 있어요.
때문에, DocumentFragment라는 임시 컨테이너 객체를 만들어서,
todoItem들을 해당 객체에 미리 추가한 후, 마지막에 DocumentFragment 객체를 appendChild로 한 번만 DOM에 추가하면, DOM 조작을 최소화할 수 있을 것 같아요!
https://developer.mozilla.org/ko/docs/Web/API/DocumentFragment
function loadTodoList(date) {
const todoList = getTodoList(date);
const todoListContainer = document.querySelector('.todoList');
todoListContainer.innerHTML = '';
// DocumentFragment 생성
const fragment = document.createDocumentFragment();
todoList.forEach((todo, index) => {
const todoItem = document.createElement('li');
const checkedStatus = todo.completed ? 'checked' : 'unchecked';
const textColor = todo.completed ? '#C0C0C0' : '#000000';
todoItem.innerHTML = `
<img src="./src/${checkedStatus}.svg" class="toggleComplete">
<span style="color: ${textColor};">${todo.text}</span>
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
`;
// 투두 완료
todoItem.querySelector('.toggleComplete').addEventListener('click', () => {
todo.completed = !todo.completed;
saveTodoList(date, todoList);
loadTodoList(date);
});
// 삭제 버튼
todoItem.addEventListener('mouseover', () => {
todoItem.querySelector('.deleteBtn').style.display = 'inline';
});
todoItem.addEventListener('mouseout', () => {
todoItem.querySelector('.deleteBtn').style.display = 'none';
});
todoItem.querySelector('.deleteBtn').addEventListener('click', () => {
todoList.splice(index, 1);
saveTodoList(date, todoList);
loadTodoList(date);
});
// DocumentFragment에 todoItem 추가
fragment.appendChild(todoItem);
});
// todoListContainer에 한 번에 추가
todoListContainer.appendChild(fragment);
console.log(todoList);
updateLeftNum(todoList); }
todoListContainer.appendChild(fragment);
todoItem.innerHTML = ` | ||
<img src="./src/${checkedStatus}.svg" class="toggleComplete"> | ||
<span style="color: ${textColor};">${todo.text}</span> | ||
<img src="./src/deleteBtn.svg" class="deleteBtn" 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.
innerHTML 방식을 활용해서 할일을 간단하게 dom에 추가해주셨네요!!
가빈님의 코드를 보며, innerHTML를 잘 활용하고 계신다는 생각이 들었어요.
innerHTML은 간단하게 dom 조작이 가능하다는 장점이 있지만, 만약 요소 노드에 기존 자식 노드가 존재했다면, 기존 자식 노드들을 제거하고 할당한 html 마크업을 파싱해서 dom에 반영하기 때문에 주의해야 할 필요가 있을 것 같아요!
또한, 사용자가 입력한 값을 이 방식으로 삽입한느 경우에 xss(크로스 사이트 스크립팅 공격)에 취약해 위험하다고 하네요.
왜냐하면, 사용자가 제공한 입력을 직접 HTML로 삽입하기 때문에, 악성 스크립트가 삽입되어 HTML 구조에 포함될 수 있다고 하니 이에 대해 알아두면 좋을 것 같아요!
https://tecoble.techcourse.co.kr/post/2021-04-26-cross-site-scripting/
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.
안녕하세요 가빈 님!
이번 코드 리뷰를 맡은 송유선입니다.
가빈 님의 결과물에서는 투두리스트 저장하기, 삭제하기, 불러오기 등이 날짜 별로 잘 구현되어 있어 인상적이었습니다! 디자인 또한 사용자가 보기에 깔끔하고 예쁜 디자인으로 되어 있다고 느꼈어요😊 저도 코드 보면서, 다른 분들이 달아주신 리뷰 보면서 많이 배울 수 있었습니다. 과제하느라 고생 많으셨고 다음 과제도 함께 파이팅해요!
<ul class="todoList"> | ||
<li> | ||
<img src="./src/${checkedStatus}.svg" class="toggleComplete"> | ||
<span style="color: #000000;">세오스 1주차 과제 하기</span> | ||
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;"> | ||
</li> | ||
</ul> |
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 파일에 li 요소들이 정의되어 있는데, 이미 JS 파일에서
todoItem.innerHTML = `
<img src="./src/${checkedStatus}.svg" class="toggleComplete">
<span style="color: ${textColor};">${todo.text}</span>
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;">
`;
이 부분을 통해 투두리스트를 항목을 동적으로 추가하고 있습니다. 따라서 ul 안에 li 요소를 미리 넣어둔 것은 살짝 불필요하다고 느껴집니다..! JS의 innerHTML에 의해서 미리 정의된 li 요소는 사용되지 않거나 덮어쓰이게 되니까요. 다음과 같이 li 요소를 삭제하고 JS에서 리스트를 동적으로 생성하는 방식으로만 처리하는 것이 어떨까요?
<ul class="todoList"> | |
<li> | |
<img src="./src/${checkedStatus}.svg" class="toggleComplete"> | |
<span style="color: #000000;">세오스 1주차 과제 하기</span> | |
<img src="./src/deleteBtn.svg" class="deleteBtn" style="display: none;"> | |
</li> | |
</ul> | |
<ul class="todoList"> | |
<!-- JS에서 동적으로 추가됨 --> | |
</ul> |
function loadTodoList(date) { | ||
const todoList = getTodoList(date); | ||
const todoListContainer = document.querySelector('.todoList'); | ||
todoListContainer.innerHTML= ''; |
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.
현재 코드는 loadTodoList() 함수가 실행될 때마다 innerHTML을 전부 초기화하고 다시 생성하는 방식으로 되어 있습니다. 작은 작업에서는 큰 무리가 없지만, 만약 투두 항목이 많아진다면 렌더링 성능이 저하될 수 있어 보입니다! 모든 투두리스트를 한 번에 삭제하고 추가하는 방식보다, 새로운 투두가 추가되었을 때만 새 li 요소를 DOM에 추가하고 기존 투두가 수정되었을 때는 해당 항목만 업데이트하도록 수정하면 어떨까요? 최대한 기존 DOM을 활용하는 방식으로요..!
@@ -1 +1,94 @@ | |||
/* 본인의 디자인 감각을 최대한 발휘해주세요! */ | |||
@import url("https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/static/pretendard.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.
사실 저는 예전에 @font-face를 통해서 폰트를 직접 호스팅하는 방식으로 코딩했었거든요. 그 때는 사이트가 좀 커졌기 때문에 배포 시 로딩이 오래 걸릴까봐 해당 방법을 택했던 건데, 이 방식에 익숙해진 나머지 전 이번에도 직접 호스팅해서 불러왔어요... 그런데 생각해보니 투두리스트 과제처럼 규모가 작은 프로젝트의 경우에는 가빈 님처럼 CDN 방식으로 가져오는 것이 훨씬 효율적인 것 같네요!! 저도 다음에는 프로젝트의 규모나 배포 속도 등을 전반적으로 고려해서 폰트 불러오는 방식을 다양화해야겠어요 😄
가빈님 잘 봤습니다! 과제 고생많으셨어요😄 |
배우고 느낀 점
많은 시간을 투자한 부분
배포 링크
https://vanilla-todo-20th.vercel.app/
Key Question
1. DOM은 무엇인가요?
(+) Virtual DOM?
-> 속도 문제나 과부하로 인한 버그를 개선하고자 React의 Virtual DOM이 등장
2. 이벤트 흐름 제어 (버블링&캡처링)이 무엇인가요?
이벤트 전파 (Event Propagation)
이벤트 버블링 (Event Bubbling)
이벤트 캡처링 (Event Capturing)
✅ event.stopPropagation()
💡event.stopPropagation()은 이벤트 전파를 완전히 중단하기 때문에 버블링이나 캡처링 단계에서 이를 호출할 경우 다른 단계의 이벤트 처리가 제대로 이루어지지 않을 수 있다.
✅ event.stopImmediatePropagation()
✅ event.preventDefault()
3. 클로저와 스코프가 무엇인가요?
스코프
스코프의 종류
렉시컬 스코프
클로저
클로저의 활용