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

2주차-1 과제. 간단한 카운터앱 만들고 파일 분리하기 #154

Merged
merged 5 commits into from
Jun 28, 2022

Conversation

alstjr0183
Copy link

2주차 첫번째 과제를 해보았습니다 피드백 부탁드리겠습니다!

Copy link

@daadaadaah daadaadaah left a comment

Choose a reason for hiding this comment

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

과제를 잘 수행하신 것 같아요 👍
가독성 부분도 고려해서 디테일한 부분 챙겨보시면 좋을 것 같아요.

src/Button.jsx Outdated
Comment on lines 3 to 7
function Button({ value, onClick }) {
return (
<button onClick={onClick} type="button">{value}</button>
);
}

Choose a reason for hiding this comment

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

function Button({ children, onClick }) {
  return (
    <button type="button" onClick={onClick}>
      {children}
    </button>
  );
}

이런식으로 변경해주시면 버튼 컴포넌트를 범용으로 사용하실 수 있어요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!!
선생님 다른분한테는 children 이라는 단어대신 상황에맞는 명시적인 단어를 권유하시던데 저는 컴포넌트 파일을 따로 분리했기때문에 children 이라는 단어가 어울리는 이유가 맞나요?
image

Choose a reason for hiding this comment

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

다른분이랑 다른 피드백을 드린 이유는 두분의 컴포넌트가 달라서 이기 때문이예요.
alstjr0183님은 저 분보다 좀더 범용적으로 쓰일 수 있는 Button 컴포넌트를 만드셨기 때문이죠!

src/Button.jsx Outdated
);
}

export default Button;

Choose a reason for hiding this comment

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

특별히 export default 따로 써주신 이유 있으신가요? ㅎㅎ
저는 function이 여러개 있을 때, 이 function은 컴포넌트를 드러내는 function 이다라는 것을 드러내기 위해 아래 형태로 작성하는 편이거든요!

export default function Button() {}

Copy link
Author

Choose a reason for hiding this comment

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

오호 그러네요 하나일경우에는 export default로 바로 적어주는게 좋은거같습니다!

src/Button.jsx Outdated
@@ -0,0 +1,9 @@
import React from 'react';

Choose a reason for hiding this comment

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

리액트17 버전 이후부터는 생략이 가능해요!
사용하시는 리액트 버전을 확인하신 후 적용해보시면 좋을 것 같아요.

참고: https://ko.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#whats-different-in-the-new-transform

Copy link
Author

Choose a reason for hiding this comment

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

image
선생님 react 버전이 17이상인데 import 를 뺐더니 Uncaught ReferenceError: React is not defined 에러가 나더라구요..! 따로 설정이 필요한 부분인가요?

Copy link
Author

Choose a reason for hiding this comment

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

구글링 해서 babel.config.js 쪽이랑 webpack.config.js 쪽 수정을 해주니 정상작동되었습니다..!
babel.config.js
image

webpack.config.js
image

Choose a reason for hiding this comment

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

이거 해결 되신건가요?
아래는 이렇게 코멘트 남겨주셨는데, 여기에는 정상작동되셨다고 하셔서 해결되셨는지 궁금하네용!

import react 빼는 부분은 에러가나서 일단은 놔뒀습니다..! react 17버전인데 따로 설정이 필요한 부분일까요?!

Copy link
Author

Choose a reason for hiding this comment

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

앗 네 수정되었습니다 감사합니다!

src/Counter.jsx Outdated
Comment on lines 1 to 2
import React, { useState } from 'react';
import Button from './Button';

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.

수정했습니다!

src/index.jsx Outdated
Comment on lines 2 to 3
import reactDom from 'react-dom';
import Counter from './Counter';

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.

수정했습니다!

@alstjr0183
Copy link
Author

alstjr0183 commented Jun 14, 2022

피드백 하신부분 수정해보았습니다...!
import react 빼는 부분은 에러가나서 일단은 놔뒀습니다..! react 17버전인데 따로 설정이 필요한 부분일까요?! ( 수정완료되었습니다! )

@alstjr0183
Copy link
Author

선생님 props 네임을 children으로 하니까 이런 eslint에서 오류가 나는데.. 이런경우에는 네이밍을 변경해야할까요?
image

@daadaadaah
Copy link

daadaadaah commented Jun 14, 2022

선생님 props 네임을 children으로 하니까 이런 eslint에서 오류가 나는데.. 이런경우에는 네이밍을 변경해야할까요? image

네이밍 변경해야합니다!
그 전에 alstjr0183님이 children이라는 단어를 어떤 의도를 가지고 사용하셨는지 궁금해요!
React에서 제공하는 children props를 사용하고 싶으셨는지! 아니면 다른 의도가 있으셨는지!

추가로, eslint 에러가 날 때 왜 에러가 나는지에 대해서도 공부해보시면 좋을 것 같아요!

@alstjr0183
Copy link
Author

alstjr0183 commented Jun 14, 2022

수정했습니다!
저는 react에서 제공하는 children props가 있는줄 모르고
button 컴포넌트쪽에 children을 사용했으니까 당연히 props를 children으로 넘겨줘야지 해서 사용을했었는데
이렇게 사용할수있다는걸 처음알았네요..!

Copy link

@daadaadaah daadaadaah left a comment

Choose a reason for hiding this comment

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

과제 하시느라 수고하셨어요!👍

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.

None yet

3 participants