[mobx] todomvc #1

hochan222 · 2021-10-19T11:50:29Z

목적: MobX로 todomvc를 만들어봅니다.

Demo

요구 사항

기능 구현

개선할 점

increasedId 캐쉬가 초기화될때, index 0 이 겹치는 문제.
toggle-all input을 누를 때, All, Active, Completed에 있는 요소만 toggle 되어야한다.
Enter 입력시 수정중인 input이 완료된다.
한글 입력 시 event.code가 두번 발생해서, 마지막 글자 한번거 todo item으로 등록되는 현상

todoStore와 UIStore로 관심사 분리를 위한 rootStore 생성

완료된 할 일만 나열할 수 있다.

기존에는 TodoList외의 태그 요소들도 존재했다. 따라서, 조금더 포괄적인 의미의 TodoContent로 변경

할 일을 일괄적으로 진행중으로 할 수 있다.

damilog · 2021-11-03T02:40:53Z

src/component/TodoApp/TodoApp.tsx

+import { IRootStore } from '../../types/models/index';
+
+function TodoApp(): React.ReactElement {
+  const rootStore: IRootStore = useContext(StoreContext) as IRootStore;


Entry Point 인 TodoApp 컴포넌트에서 Store를 내려주어 하위 컴포넌트가 Store접근을 위해 따로 context에 접근하지 않아도 되도록 하셨군요!👍 혹시 추후에 TodoStore가 아닌 rootStore 내 다른 Store에 접근할 가능성을 생각하신걸까요??😮 현재 구조로써는 TodoApp 하위 컴포넌트들이 모두 TodoStore 만을 사용하기에 TodoStore만 내려줘도 괜찮을 것 같습니다!

const { todoStore: store } = rootStore; //... <TodoInput store={store} />

UIStore를 사용해서 UI 관련? 상태들을 관리해주고 싶었는데, 막상 써보니까 active나 complete는 href 시켜주고, TodoItem check boolean 도 분리할 필요가 없겠더라구요. 그래서 레거시가 되었습니다 ㅎㅎ.. 아직, UIStore는 언제 사용해야할지 감이 안오네요..!

그래도 UIStore까지 관리하시는 걸 고려하셨다니 대단하십니다..저도 호찬님처럼 추후 과제에 UIStore 도입을 고민해봐야겠어요!

damilog · 2021-11-03T02:49:39Z

src/component/TodoContent/TodoContent.tsx

+import React from 'react';
+import { observer } from 'mobx-react-lite';
+import { useLocation } from 'react-router-dom';
+


라이브러리 import 구간과 컴포넌트 import 구간을 분리하셨군요! 저는 import 순서만 라이브러리 -> 컴포넌트 -> 유틸성 파일 등으로 구성하는데 호찬님은 이렇게 공백을 두시는 편인가요?! (그냥 궁금해서 여쭤봅니다ㅎㅎ)

오.. 사실 이번에는 아무생각없이 import 했습니다.(그래서 다른 파일보면 개행이 잘 안되어있는 부분이 많은데요) 평소에는 외부 라이브러리 -> (개행) -> 내부 라이브러리 -> (개행) -> (css 라이브러리 import '~~' 형태로 존재하는것) 순서대로 import 합니다.

다미님 방식은 같은 내부 라이브러리라도 컴포넌트나 유틸성 파일에 따라서 개행을 주시는 방식이신가요?

아니요 저는 순서만 맞출뿐 따로 개행을 해주진 않습니다!

damilog · 2021-11-03T02:53:33Z

src/component/TodoContent/TodoContent.tsx

+const TodoContent = ({ store: rootStore }: { store: IRootStore }): React.ReactElement => {
+  const { todoList, toggleCheck, removeContent, toggleAllCheck, editContent } = rootStore.todoStore;
+  const location = useLocation();
+  const pathname = location.pathname;


아래처럼 바꿔도 괜찮을 것 같습니다!

const {pathname} = location

오..! 좋아 보입니다.

const {pathname} = location 게 쓰면, location에 pathname이 존재하지 않을때, error가 나지 않을까 잠시 생각했는데, MDN pathname을 보니 이때는 빈 문자열이 들어가네요!

damilog · 2021-11-03T02:57:00Z

src/component/TodoContent/TodoContent.tsx

+    case '/completed':
+      return selectedTodolist.filter((item) => item.checked === true);
+  }
+  return selectedTodolist;


이부분을 switch 문의 default 로 넣으면 어떨까요??

switch 문에서 default를 넣는 것이 클린코드라는 것에 대한건 동의합니다.

제 코드에 default를 포함시키지 않는 이유는 아래와 같이 default를 포함하게되면 return selectedTodolist;가 중복되어서 없앴습니다. (switch 문 밖의 return selectedTodolist;를 지우게 되면, 함수 최상단에 return이 없어서 eslint가 잡습니다.)

switch (pathname) { case '/': return selectedTodolist; case '/active': return selectedTodolist.filter((item) => item.checked === false); case '/completed': return selectedTodolist.filter((item) => item.checked === true); default: return selectedTodolist; } // return selectedTodolist;

그래도 중복해서 쓰는게 좋을까요..?

아 eslint 문제였군요;; 흠...default 문에 return이 있는데도 잡는군요..ㅠㅠ 이건 어쩔 수 없는 경우네요..!!

damilog · 2021-11-03T03:06:08Z

src/component/TodoContent/TodoContent.tsx

+import TodoList from '../TodoList';
+import TodoToggleAllButton from '../TodoToggleAllButton';
+
+const todoListFilter = (pathname: string, todoList: ITodoContext[]): ITodoContext[] => {


path에 따라 todoList를 필터링 하는 로직을 깔끔하게 구현하셨네요! 👍
혹시 함수 이름을 좀 더 구체적이고 서술적인 이름으로 지으면 어떨까요? getTodoListByPathName getTodoListByFilter과 같은 함수 네이밍이 떠오르네요ㅎㅎ

damilog · 2021-11-03T03:20:11Z

src/component/TodoInput/TodoInput.tsx

+  const onKeyDownHandler = (e: React.KeyboardEvent<HTMLInputElement>) => {
+    if (e.keyCode === 13) {
+      addContent(content);
+      setContent('');


오 저는 input 후 리셋 처리 좋습니다~

damilog · 2021-11-03T03:27:55Z

src/component/TodoItem/TodoItem.tsx

+
+  return (
+    <li className={classNames({ completed: checked }, { editing: editStatus })} onClick={(e) => onClickHandler(e, id)}>
+      {itemElement}


editStatus에 따라 다른 template을 리턴하는군요! 이 로직을 함수로 구현하면 어떨까요? 해당 로직을 함수로 묶으면 함수명 만으로도 어떤 목적을 가지고 쓰여진 로직인지 파악하기 쉽고, 컴포넌트 전역에 존재하는 itemElement를 함수 안에서 구현하여 함수 외부에서는 itemElement 변경하지 못하게 함으로써 itemElement를 더 안전하게 관리할 수 있다고 생각합니다!😁

해당 로직을 함수로 묶으면 함수명 만으로도 어떤 목적을 가지고 쓰여진 로직인지 파악하기 쉽고

editStatus 가 문자열이었다면, editStatus === 'example1', editStatus === 'example2' 어떤 값인지 알지못해 함수로 묶어주는것이 좋다고 생각합니다.

하지만, editStatus은 문자열이아닌 아닌 boolean 값이기 때문에

if (editStatus) {

다음 구문의 의미가 editStatus를 확인하고 일때, if문을 실행하라. 라고 바로 알 수 있다고 생각합니다. 의미가 불분명하다면 함수명으로 만드는것보다는 변수명을 조금 더 구체적으로 boolean을 나타내는 네이밍인 checkEditstatus로 바꿔주는것이 더 좋아보입니다. 하지만,

컴포넌트 전역에 존재하는 itemElement를 함수 안에서 구현하여 함수 외부에서는 itemElement 변경하지 못하게 함으로써 itemElement를 더 안전하게 관리할 수 있다고 생각합니다!

오.. 저는 만드는것에 대한 거부감이 있었는데, 계속 생각해보니 다미님 의견이 더 좋은 의견인것같아요.

makeItemElement(checkEditstatus) 로 만드는게 좋아보입니다!

오 근데 다시 생각해보니

따로 함수를 빼기에는 안의 메소드들을 사용하고 있어서 if문이 좋아보이기도합니다. (컴포넌트 내부에 메소드로 만들기에는 줄수가 늘어나서 비교적 간단한 로직이여서 음음 고민이 됩니다) 다미님은 어떻게 생각하시나요?

함수로 결정!

앗ㅎㅎ 어제 답글을 달아주셨군요!! 저는 컴포넌트 안에서 함수로 만드는 것으로 피드백 드린 거 였는데 제 설명이 부족했네요..! 저도 함수로 빼는 게 좋아보입니다. 해당 코드에서 제가 우려되었던 부분은 (1)let으로 정의된 itemElement 변수의 가변성. (2) itemElement 가 정의된 위치와 그 값이 정해지는 위치가 떨어져있어 밀집도가 떨어지는 것 처럼 보인다.(상단에 state, 변수 등을 잘 정리해주셨지만, 처음에 itemElement 관련 로직만 봤을 때 itemElement가 뭐지? 라는 생각이 잠시 들었네요..😅) 였는데, 이렇게 함수로 묶으니 가독성이 좋아보입니다ㅎㅎ 추가적으로 혹시 함수명을 'check여부에 따라 item을 만든다' 이런식으로 수정하는 건 어렵겠죠??!

@damilog 님 의견에 동의합니다~! 앞으로 이런 경우가 있으면 함수로 묶어주는 편이 더 좋을것같네요!

추가적으로 혹시 함수명을 'check여부에 따라 item을 만든다' 이런식으로 수정하는 건 어렵겠죠??!

제가 check여부에 따라 item을 만든다 이부분은 함수명에 포함하지 않는건, 함수의 조건이지 목적이 아니라고 생각했습니다.
함수의 조건은 함수안에 포함되는편이 함수를 더 잘 나타내지 않을까요?

const 아이템엘리먼트를만든다 = (편집상태확인: boolean) => { if (편집상태가 참이면) { ... } return ... }

추가적인 조건이 포함된다 하더라도 함수명에 포함하는것보다는 함수안에 추상화된 형태로 넣는게 좋아보입니다. (실제 코드에서는 매개변수에 잘 표현되어있어서 따로 추상화보다는 삼항식으로 표현했습니다.)

1

const 편집상태확인을통한아이템엘리먼트를만든다 = (...) => { if (편집상태가 참이면) { ... } return ... } const oo상태확인을통한아이템엘리먼트를만든다 = (...) => { if (oo상태가 참이면) { ... } return ... }

2

const 아이템엘리먼트를만든다 = (...) => { if (편집상태가 참이면) { ... } else if (oo 상태가 참이면) { ... } return ... }

damilog · 2021-11-03T05:07:47Z

src/stores/TodoStore.tsx

+    this.rootStore = rootStore;
+  }
+
+  private getLocalStorage = (): ILocalStorage => {


local storage 접근 메서드를 private 로 설정하셨군요 👍

damilog · 2021-11-03T05:17:50Z

src/stores/TodoStore.tsx

+    localStorage.setItem('todoList', JSON.stringify({ todoList: this.todoList, increaseId: this.increaseId }));
+  };
+
+  getLeftItems = (): number => {


남은 Todo Item의 개수를 리턴하는 함수이니 getLeftItemsCount 등으로 네이밍 하면 어떨까요?!

좋아요! getLeftItemsLength 저는 요걸루 하겠습니댜!

damilog

컴포넌트 구조도 좋고 react router 적용을 고민하신 점, 그리고 특히 타입스크립트를 꼼꼼하게 작성해주신 점이 인상 깊었습니다. 고생하셨습니다!! 몇 가지 코멘트 드렸으니 도움이 되셨으면 좋겠네요 😁

hochan222 added 19 commits October 15, 2021 13:43

docs: add requirement

c7d9029

feat: 마크업을 컴포넌트 기능별로 나누기

91f9766

docs: 엔터를 누르면 등록이 된다., requirement

6558bbb

feat: add stores

caccc50

refactor: removeContent method

94f1824

feat: 할 일을 등록할 수 있다.

cbfbe88

feat: 할 일의 전체 현황을 나타낼 수 있다.

b41295d

feat: 할 일을 완료할 수 있다.

d052f20

feat: 할 일을 삭제할 수 있다.

9729438

feat: add rootStore

5988f5f

todoStore와 UIStore로 관심사 분리를 위한 rootStore 생성

feat: 진행중인 할 일만 나열할 수 있다.

6cd1b62

완료된 할 일만 나열할 수 있다.

feat: 남은 할일을 계산할 수 있다. (items left)

4f847c2

refactor: makeTodoCountTag, 가독성 높임, TodoFooter

089e305

refactor: todoList를 todoContent로 변경 및 구조 리펙토링

cfb2b15

기존에는 TodoList외의 태그 요소들도 존재했다. 따라서, 조금더 포괄적인 의미의 TodoContent로 변경

feat: 할 일을 일괄 완료 할 수 있다.

ae7d9b8

feat: 할 일을 일괄 삭제 할 수 있다.

b729378

feat: toggleAll checkbox, TodoToggleAllButton

1cbeaea

할 일을 일괄적으로 진행중으로 할 수 있다.

fix: increasedId 캐쉬가 초기화될때, index 0 이 겹치는 문제.

fbc7617

fix: 완료한 일을 일괄 삭제 할 수 있다.

4d9bc12

hochan222 changed the title ~~draft todomvc~~ pmobx] todomvc Oct 19, 2021

hochan222 changed the title ~~pmobx] todomvc~~ [mobx] todomvc Oct 19, 2021

hochan222 added 5 commits October 20, 2021 02:48

fix: 구조분해할당 undefined 처리

ed35f7c

feat: 할 일을 수정할 수 있다.

06eb745

feat: input을 누를 때, , , 에 있는 요소만 toggle 되어야한다.

bf61ed3

feat: 입력시 수정중인 input이 완료된다.

954cacd

fix: gh-pages route err

53edce7

hochan222 marked this pull request as ready for review October 20, 2021 09:24

hochan222 requested a review from damilog October 20, 2021 09:24

hochan222 added 2 commits October 27, 2021 00:13

refactor: add main tag

ebe9e53

fix: input warining for readOnly

c8dfbb7

fix: 한글 입력 시 가 두번 발생해서, 마지막 글자 한번거 todo item으로 등록되는 현상

83c3d23

hochan222 self-assigned this Oct 27, 2021

hochan222 added 2 commits October 27, 2021 18:03

feat: react-router-dom 적용해서, gh-pages 경로 에러 수정

6b65c37

feat: react-router-dom 적용해서, gh-pages 경로 에러 수정

40b9790

damilog reviewed Nov 3, 2021

View reviewed changes

hochan222 added 4 commits November 4, 2021 15:51

refactor: todoListFilter naming to getTodoListByPathName

5afce92

refactor: pathname 구조분해할당으로 변경, TodoContent

cc9bcbb

refactor: naming getLeftItems to getLeftItemsLength

543026b

refactor: add makeItemElement, TodoItem

caa2110

hochan222 closed this Nov 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mobx] todomvc #1

[mobx] todomvc #1

hochan222 commented Oct 19, 2021 •

edited

Loading

damilog Nov 3, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading

damilog Nov 5, 2021

damilog Nov 3, 2021

hochan222 Nov 4, 2021

damilog Nov 4, 2021

damilog Nov 3, 2021

hochan222 Nov 4, 2021 •

edited

Loading

damilog Nov 3, 2021

hochan222 Nov 4, 2021 •

edited

Loading

damilog Nov 5, 2021

damilog Nov 3, 2021

damilog Nov 3, 2021

damilog Nov 3, 2021

hochan222 Nov 4, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading

hochan222 Nov 5, 2021

damilog Nov 5, 2021

hochan222 Nov 5, 2021

damilog Nov 3, 2021

damilog Nov 3, 2021

hochan222 Nov 5, 2021

damilog left a comment

[mobx] todomvc #1

[mobx] todomvc #1

Conversation

hochan222 commented Oct 19, 2021 • edited Loading

요구 사항

기능 구현

개선할 점

damilog Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

hochan222 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hochan222 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hochan222 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hochan222 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

hochan222 Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damilog left a comment

Choose a reason for hiding this comment

hochan222 commented Oct 19, 2021 •

edited

Loading

damilog Nov 3, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading

hochan222 Nov 4, 2021 •

edited

Loading