2주차-2 과제. 간단한 Todo App 만들기 #138

alstjr0183 · 2022-06-15T11:38:48Z

2주차 과제 제출합니다 피드백 부탁드릴게요!

daadaadaah

빠르게 PR 날려주신 점 너무 잘하셨어요! 👍

남겨 드린 코멘트 참고하셔서 다음 PR 날려주세요!ㅎㅎ
그리고, 아직 파일 분리가 이루어지지 않았는데, 파일 분리하실 때 1커밋 1리팩토링 한번 진행해보시구요!ㅎㅎ

daadaadaah · 2022-06-15T13:22:21Z

src/App.jsx

+
+export default function App() {
+  const [state, setState] = useState({
+    inputValue: '',


inputValue 보다 좀더 명확하게 표현할 수 있는 단어는 없을까요? 🤔 input 과 Value 둘다 너무 추상적인 단어라고 느껴져서요!

할일을 등록하는 input의 값을 가진 상태이니 todoInsertInputValue 이렇게 네이밍 해보았습니다!

daadaadaah · 2022-06-15T13:23:51Z

src/App.jsx

+export default function App() {
+  const [state, setState] = useState({
+    inputValue: '',
+    todoList: [],


List라는 용어 말고 다른 단어를 써보시는건 어떠신가요?
저 같은 경우, 여러개를 표현할 때 복수형을 주로 사용합니다.
왜냐하면, List라는 용어는 컴퓨터 세계에서 자료형 List 이미 존재하고 있는 단어라, 혼동을 줄 수 있다고 생각하기 때문입니다!ㅎㅎ

todos로 변경해보았습니다!

daadaadaah · 2022-06-15T13:24:01Z

src/App.jsx

+
+        <button onClick={handleClickInsertButton} type="button">추가</button>
+
+        {todoListIsNull ? (<div>할 일이 없어요!</div>) : todoList.map(({ text, index }, mapIdx) => (


예외처리할 떄 자주 사용하는 guard clauses 를 활용해서 표현해 보시는건 어떠신가요?

수정하였습니다!!

daadaadaah · 2022-06-15T13:43:17Z

src/App.jsx

+  };
+
+  const handleClickInsertButton = () => {
+    const list = todoList;


list 변수보다 좀더 의도를 드러내는 용어는 없을까요? 🤔
list 라는 용어보다 어떤 리스트인지가 중요한다고 생각하기 때문이예요!

할일의 목록을 담고있는거라 todos 하려했는데 제가 이미 사용중인 네이밍이더라구요..! 물론 사용해도 에러는 안나지만 같은 네이밍을 사용하면 헷갈릴수있는 상황을 만들수도있지않을까 싶어서 겹치길래 todoList로 변경해보았습니다!

daadaadaah · 2022-06-15T13:54:26Z

src/App.jsx

+
+  const handleClickInsertButton = () => {
+    const list = todoList;
+    list.push({ text: inputValue, index: list.length });


push 를 사용하게 되면, 배열 자체를 직접 수정하게 됩니다. 이럴 경우, 리액트에서 중요하게 생각하는 불변성 유지가 어렵습니다. 배열 자체를 직접 수정하지 않고 코드를 표현하는 방법은 없을까요? 🤔

index를 좀더 유니크하게 관리하는 방법은 없을까요? 🤔

담을 정보를 변수로 만들어준뒤 스프레드연산자를 이용해서 불변성을 지켜준형태로 해보았습니다

서버에 id가 저장된다고 가정했을때 중복되지않기 위해선 어떤 방법이 있을까 고민을해보았는데 전세계 인구수가 77억정도더라구요 현재시간 + 100억사이중 랜덤한숫자 조합을 하면 겹칠일이 거의 없지않을까 싶어서 이렇게 해보았습니다 좀 아쉬운 부분이있을까요?

daadaadaah · 2022-06-15T13:55:23Z

src/App.jsx

+
+  const handleClickCompleteButton = (index) => {
+    const list = todoList;
+    list.splice(index, 1);


splice도 push 처럼 배열 자체를 직접 수정하게 됩니다. 배열 자체를 직접 수정하지 않고 코드를 표현하는 방법은 없을까요? 🤔

filter를 이용하여 고유의 id를 비교한뒤 겹치지않는것들만 빼와서 저장시켰습니다. ]
여기서도 list라는 변수명을 사용했었는대요 남은할일목록이니까 remainingTodoList 이렇게 네이밍을 해보았습니다!

daadaadaah · 2022-06-15T14:11:20Z

src/App.jsx

+import { useState } from 'react';
+
+export default function App() {
+  const [state, setState] = useState({


state 라는 표현보다 좀더 의도가 드러나는 표현을 없을까요? 🤔

todo 의 정보를 담고있어서 todo라고 심플한 네이밍을 해봤는데 더 나은 네이밍이있을까요?

app쪽 할일이없을경우 있을경우 처리를 guard clauses를 활용하여 리팩토링했습니다

변수의 네이밍을 좀더 시맨틱하게 변경한뒤 할일 추가 , 삭제 기능 함수에 원본배열을 건들이지 않게끔 동작하도록 리팩토링 하였습니다

input쪽에 onkeypress 이벤트를 이용하여 엔터를 눌렀을때도 할일이 등록되게끔 기능 추가를 해보았습니다!

alstjr0183 · 2022-06-16T07:45:20Z

전체적으로 리뷰해주신 부분 수정을 해보았습니다..! 구현사항에는 없지만 input에서 엔터를 눌렀을때 등록되게끔 기능도 추가해보았습니다

input, button , title은 좀더 범용적으로 사용할수있는 컴포넌트인거같아서 common이라는 폴더로 빼고 todoNull, todos는 todo기능을 구현하는 페이지에서만 사용할거같아서 이렇게 정리해보았습니다

alstjr0183 · 2022-06-16T07:54:39Z

폴더도 범용성있는 컴포넌트로생각되는 부분은 common으로 아닌부분은 todo로 나눠봤는데 선생님의 의견이 궁금합니다!

daadaadaah

폴더도 범용성있는 컴포넌트로생각되는 부분은 common으로 아닌부분은 todo로 나눠봤는데 선생님의 의견이 궁금합니다!

저도 범용성 있는 컴포넌트들은 common 폴더로 관리합니다! 아주 잘하셨어요👍👍👍👍
다만, Input 컴포넌트와 Title 컴포넌트는 여러 곳에서 사용하지 않는데, common 폴더에서 관리하시는 특별한 이유가 있으신가요?

컴포넌트로 빼실 때, 왜 이렇게 분리하는지 이유가 있는게 중요해요!
지금처럼 규모가 작은 앱인 경우, 저라면 App 컴포넌트만 봐도 이 앱이 무슨 앱이고, 어떤 화면으로 구성되어 있는지 단번에 알아차리도록 컴포넌트를 분리할 것 같아요!

한번 고민해보시고 수정이 필요하다고 생각되면 수정해주세요🙂

daadaadaah · 2022-06-16T14:25:51Z

src/App.jsx

+    const timeNumber = new Date().getTime();
+    const randomNumber = Math.floor(Math.random() * 10000000000);
+
+    const id = timeNumber + randomNumber;


uuid를 사용해보시는건 어떠신가요?
정말 random()함수는 완전히 유니크함을 관리할 수 있을까?에 대해서 한번 구글링해보시는 것도 좋을 것 같아요!

수정했습니다!
uuid를 구글링하여 라이브러리를 사용하여 사용했는데 uuid를 직접 구현을 해보라는 뜻으로 말씀하신걸까요?

아니요! 이번 PR처럼 라이브러리를 한번 사용해보시라는 이야기었습니다!🙂

daadaadaah · 2022-06-16T14:26:44Z

src/App.jsx

+  };
+
+  const handleEnter = (e) => {
+    if (e.key === 'Enter') handleClickInsertButton();


한줄이더라도, if문에 {}를 써주시는 것이 더 좋아요!

수정했습니다!

daadaadaah · 2022-06-16T14:27:24Z

src/App.jsx

+      {todosIsNull && <TodoNull />}
+      {!todosIsNull && (
+        <Todos todos={todos} onClick={handleClickCompleteButton} />
+      )}


예외처리할 떄 자주 사용하는 guard clauses 를 활용해서 표현해 보시는건 어떠신가요?

컴포넌트안에서 조건을 줘서 렌더링 시키는 방식으로 변경해보았습니다!

daadaadaah · 2022-06-16T14:28:51Z

src/App.jsx

+    });
+  };
+
+  const todosIsNull = todos.length === 0;


따로 변수화 하신 이유가 있으신가요? todos.length === 0 그대로 써도 될 것 같아서요!

원래했던 방식에서는 todos.length===0 을 2번타이핑하는 부분이있어서 변수화를 했는데 변경후에는 그럴필요가없어서 변수화 하지않고 한번만 사용했습니다!

daadaadaah · 2022-06-16T14:30:03Z

src/components/todo/TodoNull.jsx

@@ -0,0 +1 @@
+export default function TodoIsNull() { return (<div>할 일이 없어요!</div>); }


컴포넌트에 Null라는 단어는 어색한 것 같아요!
따로 컴포넌트로 빼신 이유가 있으신가요?
컴포너트를 빼실 때, 왜 이렇게 분리하는지 이유가 있는게 중요해요!

저는 만약 여기 컴포넌트가 css나 기능추가로 코드가 길어진다는 가정을해서 미리 쪼갰는데 과제의 요구사항만을 위해서라면 굳이 쪼갤 필요가없는거같네요!

daadaadaah · 2022-06-16T14:31:33Z

src/App.jsx

+
+    setTodo({
+      ...todo,
+      todos: remainingTodoList,


new 라는 단어를 활용해서 표현해도 좋을 것 같아요! (예 :newTodos)

수정했습니다! 전보다 더 간결하고 알아보기 쉬운 네이밍이네요!

daadaadaah · 2022-06-16T14:33:12Z

src/App.jsx

+
+export default function App() {
+  const [todo, setTodo] = useState({
+    todoInsertInputValue: '',


todoInsertInputValue 대신 inputTodo 또는 newTodo 단어들을 사용하면, alstjr0183님의 의도가 달리 전달될까요?

밑에 리뷰 남겨주신것처럼 2개의 state로 변경해서 관리해보았습니다!

daadaadaah · 2022-06-16T14:38:47Z

src/App.jsx

+    todos: [],
+  });
+
+  const { todoInsertInputValue, todos } = todo;


todo에 todos가 들어가 있는게 어색해 보여요

이런식으로 따로 관리해보는건 어떠신가요?

const [todos, setTodos] = useState([]); const [todo, setTodo] = useState('');

수정했습니다!
todo , todos로 나눠서 해보았는데 todo 사용하는 부분은 input의 value를 관리하는상태인데 inputTodo 이런 네이밍으로 하는건 별로일까요?

inputTodo 어디서 사용하고 계신가요? 현재 PR 없어서요ㅠ

inputTodo 어디서 사용하고 계신가요? 현재 PR 없어서요ㅠ

아하 현재 const [todo, setTodo] = useState(''); 이렇게 사용하는부분이 input의 value값을 보여주는 state잖아요 그래서 저는 inputTodo 이렇게 변경을 하면 어떨까 의견을 여쭤본거였습니다!

inputTodo로 해도 괜찮았을 것 같아요!

daadaadaah · 2022-06-16T14:43:38Z

src/components/Todos.jsx

  return (
    todoList.map(({ text, index }, mapIdx) => (
      <div key={index}>
        <span>{text}</span>
        <button
          onClick={() => {
-            handleClickCompleteButton(mapIdx);
+            onClick(mapIdx);


mapIdx 말고 index를 사용해서 구현해보시는건 어떠신가요?

여기 부분은 수정하기 전 코드같아요!
지금은 배열객체안에있는 id를 이용해서 todos를 업데이트 시킨답니다!

할일 등록시 배열에 유니크한 id 값을 주기 위하여 uuid 라이브러리를 사용했습니다

uuid 라이브러리추가

불필요하게 생성된 컴포넌트를 제거한뒤 guard clauses를 사용하여 리팩토링 해보았습니다

title , todonull , todos 컴포넌트를 제거했습니다

todo 라는 state를 객체로 만들어서 하나로 관리했었는데 todo , todos로 나눠서 관리하게끔 리팩토링 진행했습니다

alstjr0183 · 2022-06-17T05:39:17Z

말씀주신 리뷰 수정했습니다!
궁금한점이있는데 여기서 말씀하신부분에서 title은 왜그렇게 말씀하셨는지 이해가 되는데 input 컴포넌트도 button과 같이 input의 역할만을 할수있게끔 만들었었는데 왜 input 컴포넌트만 common에서 관리하시는 특별한 이유가 있는지 여쭤보셨을까요??

ol , li 태그를 이용해서 할일 등록시 자동으로 1 , 2 ... 숫자가 붙게끔 변경하였습니다

alstjr0183 · 2022-06-17T06:24:55Z

자동으로 넘버 붙게끔 하는 구현도 있었네요.. 이제야 확인해서 수정했습니다!

daadaadaah

왜 input 컴포넌트만 common에서 관리하시는 특별한 이유가 있는지 여쭤보셨을까요??

현재 화면 구성[입력창 1개(할일 입력란), 버튼 2개(추가 버튼, 완료 버튼) 등]으로 봤을 때, 버튼은 범용성 있는 컴포넌트로 빼서 2곳(추가버튼, 완료버튼)에서 사용할 수 있을 것 같았는데, Input 은 1개만 필요하므로, 이 요소를 Input 컴포넌트로 분리시킬 이유가 없다고 생각했어요!

그래서, 범용성 있는 Button 컴포넌트만 common 폴더에서 관리하고, 다른 컴포넌트는 다른곳에서 관리하는 것이 더 나을 것 같다고 생각해서, 물어봤어요!

daadaadaah · 2022-06-17T11:59:00Z

src/components/todo/TodosView.jsx

+          <button
+            onClick={() => {
+              onClick(id);
+            }}
+            type="button"
+          >
+            완료
+          </button>


Button 컴포넌트를 이부분도 사용할 수 있게끔 해보시는건 어떠신가요?

앗 그러네요 저부분도 button이군요!!
아하 그러면 이해가됬습니다 저는 모두 다 한곳씩 쓰는데 input 컴포넌트랑 button 컴포넌트의 차이가뭘까 생각했었거든요..!

daadaadaah · 2022-06-17T12:07:40Z

src/App.jsx

+
+      <Button onClick={handleClickInsertButton}>추가</Button>
+
+      <TodosView todos={todos} onClick={handleClickCompleteButton} />


View라는 단어를 빼고 Todos로도 충분히 이 컴포넌트가 무엇을 보여주는지 파악할 수 있을 것 같아요!

저도 선생님의 의견의 동의합니다!!

button 태그로 이루어져있던 코드를 만들어놓은 Button컴포넌트로 변경하였습니다 TodosView 라는 네이밍을 가진 컴포넌트를 좀더 간결한 Todos로 변경하였습니다

alstjr0183 · 2022-06-18T10:16:50Z

피드백 감사합니다!!
말씀주신부분들 이해가 되었습니다!!!
저 실수로 이슈로 등록해버렸는데 삭제하려구 구글링했는데 우측하단에 delete가있던데 저는 왜 안보이죠 ㅠ
피드백주신부분들은 수정완료했습니다ㅎㅎ

daadaadaah

마지막 코멘트가 될 것 같아요!ㅎㅎ
남겨드리 코멘트 보고 한번 더 PR 부탁해요💪

저 실수로 이슈로 등록해버렸는데 삭제하려구 구글링했는데 우측하단에 delete가있던데 저는 왜 안보이죠 ㅠ

우측 하단이면 어디 말씀하시는걸까용? 🤔

daadaadaah · 2022-06-18T13:29:21Z

src/components/common/Input.jsx

@@ -0,0 +1,12 @@
+export default function Input({


Input 컴포넌트 그대로 쓰기로 결정하신거예요?
Button 컴포넌트랑 차이점에 대해 이해하셔서 따로 수정할 줄 알았거든요!

앗 머리로만 이해를하고 분리를 안해놨네요.. 하하.. 수정했습니다!

daadaadaah · 2022-06-18T13:29:48Z

src/components/todo/Todos.jsx

@@ -0,0 +1,23 @@
+import Button from '../common/Button';
+
+export default function TodosView({ todos, onClick }) {


깔끔하게 View 뺍시당!🚀

넵 수정했습니다!

Input 컴포넌트는 한곳에서 쓰니까 todo 폴더로 분리했습니다 파일명을 Todos로 변경했는데 안에 함수네이밍은 그대로였네요 수정했습니다!

Input 컴포넌트를 todo폴더로 분리하면서 줄바꿈을 해주었습니다

alstjr0183 · 2022-06-18T13:39:39Z

마지막에 거의 다됬다 생각해서 디테일하게 신경을 안썼네요..! 앞으론 마무리까지 깔끔히하겠습니다!
한주간 리뷰 감사합니다!

todo에서 inputTodo로 네이밍을 변경했습니다

daadaadaah

이번주도 수고 많으셨습니다! 😎
회고를 통해 한주를 돌아보시고, 다음주에도 같이 화이팅해봐요!👊

alstjr0183 added 2 commits June 15, 2022 00:25

feat : 투두리스트 기능구현 완료

850b4d8

refactor : app파일 분리

8678b97

daadaadaah reviewed Jun 15, 2022

View reviewed changes

alstjr0183 added 10 commits June 16, 2022 08:37

feat: Title 컴포넌트 추가

ff6f344

feat: Button 컴포넌트 추가

77f7d79

feat: Input 컴포넌트 추가

259d1ee

feat: TodoNull 컴포넌트 추가

62ce23d

feat: Todos 컴포넌트 추가

c148457

fix: Todos 컴포넌트 클릭이벤트 네이밍 변경

2d83dfc

refactor: app 페이지 컴포넌트로 구성 작업완료

ce83cb0

refactor: 할일이있을경우 없을경우 조건 리팩토링

3a79a39

app쪽 할일이없을경우 있을경우 처리를 guard clauses를 활용하여 리팩토링했습니다

refactor : 변수이름 변경 , todo 등록 삭제 함수 로직변경

d3a2f65

변수의 네이밍을 좀더 시맨틱하게 변경한뒤 할일 추가 , 삭제 기능 함수에 원본배열을 건들이지 않게끔 동작하도록 리팩토링 하였습니다

feat: input에 엔터입력시 등록기능도 추가

2389520

input쪽에 onkeypress 이벤트를 이용하여 엔터를 눌렀을때도 할일이 등록되게끔 기능 추가를 해보았습니다!

refactor: 폴더 분리

1b0f4e4

input, button , title은 좀더 범용적으로 사용할수있는 컴포넌트인거같아서 common이라는 폴더로 빼고 todoNull, todos는 todo기능을 구현하는 페이지에서만 사용할거같아서 이렇게 정리해보았습니다

daadaadaah self-assigned this Jun 16, 2022

daadaadaah reviewed Jun 16, 2022

View reviewed changes

alstjr0183 added 5 commits June 17, 2022 13:46

feat: id 유니크한 값으로 uuid 사용

36bfa12

할일 등록시 배열에 유니크한 id 값을 주기 위하여 uuid 라이브러리를 사용했습니다

feat: uuid 라이브러리 package.json

d5d705a

uuid 라이브러리추가

refactor: 불필요한 컴포넌트화 제거 , guard clauses 리팩토링

82d8e50

불필요하게 생성된 컴포넌트를 제거한뒤 guard clauses를 사용하여 리팩토링 해보았습니다

remove: 불필요한 컴포넌트 제거

6f7949e

title , todonull , todos 컴포넌트를 제거했습니다

refactor: useState 2개로 상태관리하게끔 변경

e755a36

todo 라는 state를 객체로 만들어서 하나로 관리했었는데 todo , todos로 나눠서 관리하게끔 리팩토링 진행했습니다

feat: 할일목록 자동으로 번호넣게끔 변경

20d5df7

ol , li 태그를 이용해서 할일 등록시 자동으로 1 , 2 ... 숫자가 붙게끔 변경하였습니다

daadaadaah reviewed Jun 17, 2022

View reviewed changes

refactor: button태그 컴포넌트로 변경 , 컴포넌트명 수정

acbd32f

button 태그로 이루어져있던 코드를 만들어놓은 Button컴포넌트로 변경하였습니다 TodosView 라는 네이밍을 가진 컴포넌트를 좀더 간결한 Todos로 변경하였습니다

alstjr0183 mentioned this pull request Jun 18, 2022

> inputTodo 어디서 사용하고 계신가요? 현재 PR 없어서요ㅠ #146

Closed

daadaadaah reviewed Jun 18, 2022

View reviewed changes

alstjr0183 added 2 commits June 18, 2022 22:35

refactor: Input컴포넌트 분리 및 Todos 수정

2fc23b7

Input 컴포넌트는 한곳에서 쓰니까 todo 폴더로 분리했습니다 파일명을 Todos로 변경했는데 안에 함수네이밍은 그대로였네요 수정했습니다!

refactor: 파일 구분

168f77e

Input 컴포넌트를 todo폴더로 분리하면서 줄바꿈을 해주었습니다

refactor: state명 todo -> inputTodo 변경

4c3ec6d

todo에서 inputTodo로 네이밍을 변경했습니다

daadaadaah approved these changes Jun 18, 2022

View reviewed changes

daadaadaah merged commit e67efba into CodeSoom:alstjr0183 Jun 20, 2022


		<button onClick={handleClickInsertButton} type="button">추가</button>

		{todoListIsNull ? (<div>할 일이 없어요!</div>) : todoList.map(({ text, index }, mapIdx) => (

		@@ -0,0 +1 @@
		export default function TodoIsNull() { return (<div>할 일이 없어요!</div>); }


		<Button onClick={handleClickInsertButton}>추가</Button>

		<TodosView todos={todos} onClick={handleClickCompleteButton} />

		@@ -0,0 +1,23 @@
		import Button from '../common/Button';

		export default function TodosView({ todos, onClick }) {

2주차-2 과제. 간단한 Todo App 만들기 #138

2주차-2 과제. 간단한 Todo App 만들기 #138

Conversation

alstjr0183 commented Jun 15, 2022

daadaadaah left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstjr0183 Jun 16, 2022 • 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

alstjr0183 commented Jun 16, 2022

alstjr0183 commented Jun 16, 2022

daadaadaah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstjr0183 Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

daadaadaah Jun 17, 2022 • 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

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

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

alstjr0183 commented Jun 17, 2022

alstjr0183 commented Jun 17, 2022

daadaadaah left a 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

alstjr0183 commented Jun 18, 2022 • edited Loading

daadaadaah left a comment • 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

alstjr0183 commented Jun 18, 2022 • edited Loading

daadaadaah left a comment

Choose a reason for hiding this comment

alstjr0183 Jun 16, 2022 •

edited

Loading

daadaadaah left a comment •

edited

Loading

alstjr0183 Jun 17, 2022 •

edited

Loading

daadaadaah Jun 17, 2022 •

edited

Loading

alstjr0183 commented Jun 18, 2022 •

edited

Loading

daadaadaah left a comment •

edited

Loading

alstjr0183 commented Jun 18, 2022 •

edited

Loading