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

[10기 김장후] TodoList with CRUD #216

Open
wants to merge 14 commits into
base: kimjanghu
Choose a base branch
from
15 changes: 14 additions & 1 deletion index.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,20 @@ <h1>TODOS</h1>
<main>
<input class="toggle-all" type="checkbox" />
<ul id="todo-list" class="todo-list"></ul>
<div class="count-container"></div>
<div class="count-container">
<span class="todo-count"></span>
<ul class="filters">
<li>
<a class="all selected" href="#">전체보기</a>
</li>
<li>
<a class="active" href="#active">해야할 일</a>
</li>
<li>
<a class="completed" href="#completed">완료한 일</a>
</li>
</ul>
</div>
</main>
</div>
</body>
Expand Down
61 changes: 52 additions & 9 deletions src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,75 @@ import TodoCount from './components/TodoCount.js';
import TodoList from './components/TodoList.js';
import Component from './core/component.js';
import State from './core/State.js';
import { $ } from './utils/utils.js';
import { FILTER_TYPES, STORAGE_KEY } from './utils/constants.js';
import { $, setLocalStorageItem, getLocalStorageItem } from './utils/utils.js';

export default class App extends Component {
setState() {
this.todoList = new State([]);
this.todoList = new State(getLocalStorageItem(STORAGE_KEY.TODO) || []);
this.filteredTodoList = new State(null);
this.filterState = new State('all');
}

render() {
this.mountChildren();
this.mountTodoList();
}

setFilteredTodoList() {
const filterTodo = Object.freeze({
[FILTER_TYPES.ALL]: this.viewAllTodo.bind(this),
[FILTER_TYPES.ACTIVE]: this.viewActiveTodo.bind(this),
[FILTER_TYPES.COMPLETED]: this.viewCompletedTodo.bind(this),
});
filterTodo[this.filterState.get()]();
// this.todoListView.setState(this.filteredTodoList.get());
// this.todoListView.render();
this.renderComponent(this.todoListView);
this.renderComponent(this.todoCountView);
this.todoCountView.setState(this.filteredTodoList.get());
this.todoCountView.render();
}

renderComponent(view) {
view.setState(this.filteredTodoList.get());
view.render();
}

viewAllTodo() {
this.filteredTodoList.set(this.todoList.get());
}

viewActiveTodo() {
const activeTodo = this.todoList.get().filter((todo) => todo.checked === false);
this.filteredTodoList.set(activeTodo);
}

viewCompletedTodo() {
const completedTodo = this.todoList.get().filter((todo) => todo.checked === true);
this.filteredTodoList.set(completedTodo);
}

mountChildren() {
new NewTodoInput($('#new-todo-title'), {
todoList: this.todoList,
onSubmitTodo: this.mountTodoList.bind(this),

Choose a reason for hiding this comment

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

^^ 이렇게 바꾼 이유는 혹시 onSubmitTodo를 이 App.js 내부에서는 그려짐과 동시에 행해야 할 것이기에 명칭을 바꾼 것으로 생각됩니다만, key값은 그대로 두신 이유가 궁금해졌습니다.

Copy link
Author

Choose a reason for hiding this comment

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

props를 받는 컴포넌트의 네임을 바꾸기 귀찮아서 그대로 뒀습니다 ㅎㅎ 큰 이유는 없었어요

});

this.todoCountView = new TodoCount($('.count-container'), {
todoList: this.todoList,
});
}

mountTodoList() {
this.todoListView = new TodoList($('#todo-list'), {
todoList: this.todoList,
todoList: this.filteredTodoList.get() === null ? this.todoList.get() : this.filteredTodoList.get(),
checkTodo: this.checkTodo.bind(this),
deleteTodo: this.deleteTodo.bind(this),
editTodo: this.editTodo.bind(this),
});
this.todoCountView.render();

this.todoCountView = new TodoCount($('.count-container'), {
todoList: this.filteredTodoList.get() === null ? this.todoList.get() : this.filteredTodoList.get(),
Copy link

@JamieShin0201 JamieShin0201 Jul 12, 2021

Choose a reason for hiding this comment

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

todoList: this.filteredTodoList.get() ?? this.todoList.get(),

Nullish coalescing operator를 이용해 볼 수도 있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 한번 찾아보고 적용해보겠습니다!

filterState: this.filterState,
onClickTodoRender: this.setFilteredTodoList.bind(this),
});
}

checkTodo(id) {
Expand All @@ -44,22 +83,26 @@ export default class App extends Component {
return todo;
});
this.todoList.set([...todoList]);
setLocalStorageItem(STORAGE_KEY, todoList);
}

deleteTodo(id) {
const todoList = this.todoList.get().filter((todo) => todo.id !== id);
setLocalStorageItem(STORAGE_KEY.TODO, todoList);
this.todoList.set([...todoList]);
this.todoCountView.render();
}

editTodo(id, value) {
this.todoList.get().map((todo) => {
const todoList = this.todoList.get().map((todo) => {
if (todo.id === id) {
todo.todo = value;
return todo;
}
return todo;
});
setLocalStorageItem(STORAGE_KEY.TODO, todoList);
this.todoList.set([...todoList]);
this.todoListView.render();
}
}
3 changes: 3 additions & 0 deletions src/components/NewTodoInput.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import Component from '../core/component.js';
import { STORAGE_KEY } from '../utils/constants.js';
import { setLocalStorageItem } from '../utils/utils.js';

export default class NewTodoInput extends Component {
bindEvents() {
Expand All @@ -18,5 +20,6 @@ export default class NewTodoInput extends Component {
checked: false,
});
this.props.todoList.set(todoList);
setLocalStorageItem(STORAGE_KEY.TODO, todoList);
}
}
47 changes: 33 additions & 14 deletions src/components/TodoCount.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,39 @@
import Component from '../core/component.js';
import { FILTER_TYPES, TODO_BUTTONS } from '../utils/constants.js';
import { $, $$ } from '../utils/utils.js';

export default class TodoCount extends Component {
setState(newState) {
this.todoList = newState || this.props.todoList;
}

render() {
this.$target.innerHTML = `
<span class="todo-count">총 <strong>${this.props.todoList.get().length}</strong> 개</span>
<ul class="filters">
<li>
<a class="all selected" href="#">전체보기</a>
</li>
<li>
<a class="active" href="#active">해야할 일</a>
</li>
<li>
<a class="completed" href="#completed">완료한 일</a>
</li>
</ul>
`;
$('.todo-count').innerHTML = `총 <strong>${this.todoList.length}</strong> 개`;
}

bindEvents() {
$('.filters').addEventListener('click', ({ target }) => {
if (target.classList.contains(FILTER_TYPES.ALL)) {
this.props.filterState.set(FILTER_TYPES.ALL);
this.selectFilterTypeBtn(FILTER_TYPES.ALL);
}
if (target.classList.contains(FILTER_TYPES.ACTIVE)) {
this.props.filterState.set(FILTER_TYPES.ACTIVE);
this.selectFilterTypeBtn(FILTER_TYPES.ACTIVE);
}
if (target.classList.contains(FILTER_TYPES.COMPLETED)) {
this.props.filterState.set(FILTER_TYPES.COMPLETED);
this.selectFilterTypeBtn(FILTER_TYPES.COMPLETED);
}
Comment on lines +16 to +27

Choose a reason for hiding this comment

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

세개의 if문이 비슷해 보이는데 함수로 만드는 것도 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

네! 리펙토링 과정에서 한번 고려해보겠습니다.

this.props.onClickTodoRender();
});
}

selectFilterTypeBtn(type) {
$$('a', $('.filters')).forEach((tag) => {
tag.classList.remove(TODO_BUTTONS.SELECTED);
});

$(`.${type}`, $('.filters')).classList.add(TODO_BUTTONS.SELECTED);
}
}
26 changes: 20 additions & 6 deletions src/components/TodoList.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,25 @@ import { $, $$ } from '../utils/utils.js';
import { TODO_BUTTONS } from '../utils/constants.js';

export default class TodoList extends Component {
setState(newState) {
this.todoList = newState || this.props.todoList;

Choose a reason for hiding this comment

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

해당 문법에 대해 익숙지 않아서 질문드립니다..!ㅠㅠ~
혹시 7라인의 의미가 어떻게 되나요..

Copy link
Author

Choose a reason for hiding this comment

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

||, &&, ?? 연산자를 잘 사용하면 좋은데요. || 같은 경우 a || b일 때, a가 참값이라면 b를 보지않고 바로 a를 실행합니다. 반대로 a가 false값이라면 b를 실행합니다! 이런식으로 if보다는 || && 를 사용하는 것도 좋다고 들었어요

}

render() {
const todoList = this.props.todoList
.get()
.reduce((html, { id, todo }) => (html += this.renderTodo({ id, todo })), '');
console.log(this.todoList);
const todoList = this.todoList.reduce(
(html, { id, todo, checked }) => (html += this.renderTodo({ id, todo, checked })),
'',
);
this.$target.innerHTML = todoList;
}

renderTodo({ id, todo }) {
renderTodo({ id, todo, checked }) {
const checkTodo = checked ? 'completed' : '';
return `
<li class="todo" data-id=${id}>
<li class="todo ${checkTodo}" data-id=${id}>
<div class="view">
<input class="toggle" type="checkbox" data-id=${id} />
${this.checkCheckbox(id, checked)}

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.

보통 리액트의 경우에도 저런식으로 return에 직접 사용할 수 있지만, best practice를 보면 내부에 로직을 작성하기 보다는 저렇게 함수로 분리해서 하는걸 추천하더라구요! 물론 코딩 스타일 차이일거같아요. 내부에 로직을 작성해도 큰 문제는 없습니다.

<label class="label">${todo}</label>
<button class="destroy" data-id=${id}></button>
</div>
Expand All @@ -23,6 +30,12 @@ export default class TodoList extends Component {
`;
}

checkCheckbox(id, checked) {
return checked
? `<input class="toggle" type="checkbox" data-id=${id} checked />`
: `<input class="toggle" type="checkbox" data-id=${id} />`;
}

bindEvents() {
$$('.todo').forEach((item) => {
item.addEventListener('click', ({ target }) => {
Expand All @@ -31,6 +44,7 @@ export default class TodoList extends Component {
if (classList.contains(TODO_BUTTONS.TOGGLE)) {
target.closest('.todo').classList.toggle('completed');
this.props.checkTodo(targetId);
console.log(targetId);
}

if (classList.contains(TODO_BUTTONS.DESTROY)) {
Expand Down
17 changes: 14 additions & 3 deletions src/utils/constants.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
const TODO_BUTTONS = {
const TODO_BUTTONS = Object.freeze({

Choose a reason for hiding this comment

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

freeze가 const만으로 안되는 부분을 해결해주기에 사용하신 거겠죠?
const만으로 충분히 불변이 보장될 것이라고 생각했어서요.

Copy link
Author

Choose a reason for hiding this comment

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

네 임의 조작을 방지하기 위해 freeze를 사용했습니다.

TOGGLE: 'toggle',
LABEL: 'label',
DESTROY: 'destroy',
EDIT: 'edit',
EDITING: 'editing',
COMPLETED: 'completed',
};
SELECTED: 'selected',
});

export { TODO_BUTTONS };
const FILTER_TYPES = Object.freeze({
ALL: 'all',
ACTIVE: 'active',
COMPLETED: 'completed',
});

Choose a reason for hiding this comment

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

Object.freeze 좋은 사용방법 배워갑니다 👍

const STORAGE_KEY = Object.freeze({
TODO: 'todoList',
});

export { TODO_BUTTONS, FILTER_TYPES, STORAGE_KEY };
11 changes: 8 additions & 3 deletions src/utils/utils.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
const $ = (selector) => document.querySelector(selector);
const $$ = (selector) => document.querySelectorAll(selector);
const $ = (selector, tag = document) => tag.querySelector(selector);
const $$ = (selector, tag = document) => tag.querySelectorAll(selector);
const setLocalStorageItem = (key, value) => localStorage.setItem(key, JSON.stringify(value));
const getLocalStorageItem = (key) => {
const todoList = localStorage.getItem(key);
return JSON.parse(todoList);
Comment on lines +5 to +6

Choose a reason for hiding this comment

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

함수명과 리턴 값 (변수명?) 이 비슷하면 좋을 것 같아요.

};

export { $, $$ };
export { $, $$, setLocalStorageItem, getLocalStorageItem };

Choose a reason for hiding this comment

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

util을 따로 분류하여 하시는 것 정말 좋아보입니다..!
안그래도 제 코드의 반복되는 부분이 많아 어떤 방법이 좋을지 찾고있었는데
앞으로 이런식으로 분류하면 되겠네요 ㅎㅎ
배워갑니다 😸 😸 😸 😸