Skip to content

Conversation

@timothypark33
Copy link

@timothypark33 timothypark33 commented Dec 7, 2021

리뷰

input값 가져오기

input태그에서 값을 가져올 때에 여러가지 방법이 있는 것으로 기억합니다.
그중 form태그와 연결된 함수에서 event가 매개변수로 가져오고 event의 하위 객체안에서 찾아보았습니다.

습관들 데이터 관리하기

과제1과 마찬가지로 데이터(습관들)를 하위 컴포넌트가 쉽게 공유해 쓰고 관리하고자 최상단 App 컴포넌트에 배치했습니다.
그리고 그 데이터를 활용하는 함수들도 App 컴포넌트에 배치했습니다.

고칠점

같이 참여하는 김승규 학우분께서 깃 커밋을 깔끔하게 하시는거 반성이 되고 보고 배워야겠다고 느꼈습니다.
앞으로 chore과 feature을 구분해서 커밋해야겠습니다.

읽어주셔서 감사합니다.

기능

  • 사용자는 할 일을 입력할 수 있습니다.
  • 사용자는 추가 버튼을 눌러서 할 일을 추가할 수 있습니다.
  • 사용자는 할 일 목록을 볼 수 있습니다.
  • 사용자는 완료 버튼을 눌러 할 일을 삭제할 수 있습니다.

@timothypark33 timothypark33 changed the base branch from main to GUAJEGUICHAN December 7, 2021 11:03
Copy link
Collaborator

@moonkii moonkii left a comment

Choose a reason for hiding this comment

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

image

다른 사람들의 PR도 보면서 공부하는 자세 되게 좋은 것 같아요 👍
좋은 부분은 배우고 따라해보면서 나에게 부족한 점을 채워나가면 좋을 것 같아요.
지금처럼 하나의 파일에서 컴포넌트로 분리해보면서 파일로 분리해나가는 점진적인 방법으로 만들어 나가시는 것도 좋아요.

PR을 보내시기 전에 항상 린트를 확인해주세요!

src/index.jsx Outdated
import ReactDOM from 'react-dom'


function Habit({ onDelete, habit, keyValue }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

클릭이벤트라는 걸 이름에 드러내주면 좋을 것 같아요.
onClickDelete~ 같은 형태로 작성해주면 좋을 것 같네요 ;)

Copy link
Author

Choose a reason for hiding this comment

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

onClickDelete로 변경했습니다~~

src/index.jsx Outdated
return (
<div>
{habit}
<button onClick={() => onDelete(keyValue)} type="button" >완료</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

keyValue 라는 이름은 의도를 이해하기 어려운 것 같아요.
ID 같은 값을 주고 싶었던 건가요?

Copy link
Author

@timothypark33 timothypark33 Dec 8, 2021

Choose a reason for hiding this comment

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

정확합니다...

src/index.jsx Outdated
)
}

ReactDOM.render(<App />, document.getElementById('app'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

파일 끝에 붙어 있는 ⛔️ (No newline at end of file) 를 보신적이 있으신가요??
Files changed 를 유심히 보셨다면 한 번씩은 보셨을텐데요.
항상 Pull Request를 보내고 나서 자신이 작성한 코드를 Files changed 에서 다시 한번 살펴보는 습관을 들이시길 추천해요.
파일 끝에 개행을 추가하지 않으면 파일 끝에 ⛔️ (No newline at end of file) 경고 문구가 붙어요.
파일 끝에 개행을 추가 하는 이유는 예전에는 컴파일러가 파일 끝에 개행문자가 없으면 한 줄이 끝나지 않은 것으로 인식해서 에러가 발생하는 이슈가 있었기 때문이에요.

최근에는 파일 끝에 개행문자가 없어도 컴파일러에서 별다른 문제가 발생하지 않지만 혹시나 모를 잠재적인 에러나 POSIX 에 명세되어 있기 때문에 개인적으로는 파일 끝에 개행문자를 추가 하시길 권장해요.

Copy link
Author

@timothypark33 timothypark33 Dec 8, 2021

Choose a reason for hiding this comment

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

저번에도 같은 것을 리뷰 받았었어서 vscode 설정을 바꿔보았었습니다.
그 설정이 자동으로 한줄 띄워줄줄 알았는데 아니었네요..
계속 신경써서 한 줄 띄겠습니다

src/index.jsx Outdated
Comment on lines 64 to 66
setState({
habits: [e.target[0].value, ...habits]
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
setState({
habits: [e.target[0].value, ...habits]
})
setState({
...state
habits: [e.target[0].value, ...habits]
});

만약 객체에 habits 말고 다른 상태 속성이 있으면 어떤 일이 발생할까요?

Copy link
Author

@timothypark33 timothypark33 Dec 8, 2021

Choose a reason for hiding this comment

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

그럼 habits만 남겠네요.. 리뷰 감사합니다!! 수정했습니다!

src/index.jsx Outdated
setState({
habits: [e.target[0].value, ...habits]
})
e.target[0].value = "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

event 객체를 상위 컴포넌트에서 처리해야할까요?
관심사의 분리에 대해 고민해보시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

리뷰해주신 것 바탕으로
event 객체를 Page에 컴포넌트로 옮겼습니다.
왜냐하면 Page컴포넌트에 Input 컴포넌트도 있는데
form 형태를 그리는 Input 컴포넌트와 데이터를 다루는 부분을 보기쉽게 분리하고자 하였습니다.

Copy link
Collaborator

@moonkii moonkii Dec 9, 2021

Choose a reason for hiding this comment

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

제가 의도한건 이벤트 객체를 실제 입력을 받는 Input 컴포넌트에서 처리하는 것이었어요.

예를들어

하위컴포넌트에서는

export default function TodoField({ onChange, ... }) {
  function handleChnage(event) {
   const { value } = event.target;
   onChange(value);
  }

  return ( .... );
}

상위 컴포넌트에는

export default function App() {
  function handleChangeTodo(value) {
   ....
  }

  return (
   ...
   <TodoField onChange={handleChangeTodo} ... />
  );
}

이런식으로 작성해주면 App 컴포넌트에서 상태를 변경해주는 함수는 하위 컴포넌트와 의존성이 끊어지기 때문에 하위컴포넌트에서 어떻게 구현되는지 알필요가 없고 단순히 값만 받아서 업데이트해주면 되는 구조를 만들 수 있어요.

Copy link
Author

Choose a reason for hiding this comment

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

이렇게 하니 보기가 더 깔끔해졌습니다. 감사합니다.

src/index.jsx Outdated
})
const { habits } = state;

function handleDelete(key) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

key 라고하면 헷갈리는 것 같아요.
의도를 드러내는 더 좋은 이름이 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

id로 변경했습니다~!

src/index.jsx Outdated
Comment on lines 32 to 33
<input type="text" name="content" placeholder="할 일을 입력해 주세요" />
<input type="submit" value="추가" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Input 컴포넌트를 어떻게 만들면 좋을지 리액트 공식문서 를 참고해보시면 좋을 것 같아요.

Copy link
Author

@timothypark33 timothypark33 Dec 8, 2021

Choose a reason for hiding this comment

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

uesEffect를 사용해야할것같은데 좀더 공부하고 내일 반영 하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

useEffect 는 쓸일이없어요!
제발 말한 건 input 태그 속성으로 value 를 넣어주는 걸 말한거에요.

참고:

@timothypark33
Copy link
Author

리뷰

커밋

커밋할때 정말 엄밀히 해야 리뷰하시는 분께서 헷갈리지 않겠다라는게 느껴집니다.
하나하나 수정할때마다 커밋내용에 최대한 맞아 떨어지게 해야겠습니다.
종종 중간에 작은 실수를 고치고 설정도 수정하고 커밋에 반영 안했었는데 추가하도록 하겠습니다.

import Page from './Page.jsx';


function App() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

export default function App() { ... } 같이 간단하게 표현해줄 수도이 있어요!

@moonkii
Copy link
Collaborator

moonkii commented Dec 9, 2021

PR 보내시기전에 항상 린트 확인해주세요!

@timothypark33
Copy link
Author

eslint를 잘못알고 사용했었습니다..
오늘은 elslint까지만 해보았습니다.

@timothypark33
Copy link
Author

[12/10]리뷰

리뷰해주신 부분을 고친것 중에서
[refactor] onClickSubmit이벤트 객체를 Input 컴포넌트에서 처리
[refactor] input태그에 value 속성 사용

가 저에게 큰 실력향상이 된 것같습니다. 감사합니다!

import Habits from './Habits';

function Input({ onSubmit, onClickDelete, habits }) {
const [value, setValue] = useState('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

입력받은 상태도 Page 컴포넌트에서 관리해주면 좋을 것 같아요.

@timothypark33
Copy link
Author

다른분들 리뷰를 참고해서
habits태그를 input이 아닌 page 컴포넌트에 옮겼습니다.

입력받은 상태도 Page 컴포넌트에서 관리 하도록 했습니다.

그런데 지금보니 Todo로 안하고 Habit으로 했었네요..

@moonkii moonkii merged commit d131515 into CodeSoom:GUAJEGUICHAN Dec 12, 2021
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.

2 participants