Skip to content

3주차_todo api 연동#15

Open
yeppin wants to merge 14 commits intomainfrom
seminar/week-3
Open

3주차_todo api 연동#15
yeppin wants to merge 14 commits intomainfrom
seminar/week-3

Conversation

@yeppin
Copy link

@yeppin yeppin commented Aug 2, 2022

1. `localhost:8000` proxy 연결

2. 2주차 기능 api 연동

  • 모든 todo 불러오기 GET/api/todos/
  • todo 생성 POST /api/todos/create/
  • todo 삭제 DELETE /api/todos/:id/
  • todo 완료여부 toggle PATCH /api/todos/:id/check/

3. 2주차 심화과제 기능 구현 및 api 연동

  • todo 텍스트 수정 PATCH /api/todos/:id/

각주는 세미나 작성자를 위해서 적어둔 것이므로 main merge시 삭제할 예정입니다~!

@yeppin yeppin self-assigned this Aug 2, 2022
Copy link

@kjw7953 kjw7953 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~~~~

const onSubmitTextUpdatedTodo = (e) => {
e.preventDefault();
onUpdate(id, todoText);
setIsTodoEditable(false);
Copy link

Choose a reason for hiding this comment

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

이것까지 해야하나 싶긴한데, 수정 api 요청이 실패할 경우에도 setIsTodoEditable 값이 바뀌는 것 같아서 정상적으로 response를 받았을 때만 setState 해주는 건 어떤가요? (적어보니 이것까지 해야하나라는 생각이 더 들긴하는데,,)

Copy link
Author

Choose a reason for hiding this comment

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

오.. 그러면 update axios쪽 then에 setState해주는 방식이죠..?!

Copy link
Member

Choose a reason for hiding this comment

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

@yeppin 요청 성공했을 때 editable 하다로 바뀌는 게 저도 좋을 것 같아요..!!

//edit icon 클릭시 div -> input으로 변경
const [isTodoEditable, setIsTodoEditable] = useState(false);
//todo text
const [todoText, setTodoText] = useState(text);
Copy link

Choose a reason for hiding this comment

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

todo를 props로 내려 받아서 todo.text를 별도의 로컬 state 선언하여 사용하는 것이 안티패턴인걸로 생각되는데, todoText 상태를 없애고 props로 내려받은 todo.text를 그대로 사용하면서 onChange 함수를 props로 내려받으면 어떨까요..? done 상태는 내려받은 props로 사용하면서, text 값만 별도로 관리하고 있어도 좀 더 부자연스럽긴 합니다.
다만 저도 꼭 해야할까 하는 고민이 있습니다... 엉엉

Copy link
Author

@yeppin yeppin Aug 4, 2022

Choose a reason for hiding this comment

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

<TodoItem key={todo.id} todo={todo} onToggle={onToggle} onUpdate={onUpdate} onRemove={onRemove} />
위 컴포넌트에 onChange={onChange}도 추가해서 props로 내려받자는 뜻으로 이해했는데 맞나요?!
그렇게 되면 TodoTemplate/index.js에 api 함수(onRemove, onCreate, onUpdate, onToggle) 뿐만 아니라 onChange가 들어가게 되어 10기가 구조를 헷갈리게 될까봐 조금 걱정..!!ㅠㅠㅠ 어떻게할까요

Copy link
Author

Choose a reason for hiding this comment

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

요거 먼저 코드는 그대로 두고, 재환오빠 설명(구조 이해하기 편하라고 요렇게 적었지만, 리액트 정석적으로 권장은 안 한다는..)을 세미나에 덧붙여주실 수 있을까요?! @lerrybe

Copy link
Member

Choose a reason for hiding this comment

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

@jaehvvan @yeppin
그러면 예빈이가 작성한대로 하고,
설명 덧붙이면서 todo props 속에 done 상태를 내려받고 있으니 text도 이와같이 관리하려면 어떻게 해야하는지 생각해보라고 하는 설명 추가하면 괜찮을까요?! (밑에 토글로 방법 설명)

Copy link
Author

Choose a reason for hiding this comment

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

오 조아요 조아요

@yeppin yeppin requested review from jaehvvan and kjw7953 August 4, 2022 09:27
@lerrybe lerrybe self-requested a review August 6, 2022 10:18
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