Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Textarea 컴포넌트 추가 #31
Textarea 컴포넌트 추가 #31
Changes from 5 commits
536fcb6
9b2a1d0
8ee9343
db91cc1
374ffaf
fc16f08
44f2722
87c4940
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[질문]
이부분 지금까지 저는 props 파라미터에 직접 default 값을 주는 식으로 처리했었는데 defaultProps를 사용하는게 더 좋을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 recruit에서 작업할때 OptionalProp을 사용할때 eslint airbnb규칙에 의존된 react/require-default-props에 의해서 에러를 발생시키는 경우가 있었는데 현재 admin쪽에는 그러한 상황이 발생하지 않는것 같아요 (이유는 자세히 의존성을 보지 않아서 모르겠네요)
아마 @mango906 님이 recruit쪽에서 해당 코드를 리뷰했었어서 이렇게 맞추신거 같아요!
똑같이 맞춰서 가려면 defaultProps로 해도 좋을거 같고 사실 이 부분은 저희가 컨벤션으로 따로 맞춘게 없어서 admin에선 어느 방향으로 해도 괜찮은거 같습니다~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 요 부분은 이상하게 여기서만 default 파라미터가 작동하지 않더라구요;; 그래서 우선 이렇게 작업했었는데 한번 더 확인해볼게요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시 해보니 잘돼서 수정했습니다! 87c4940
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[제안]
이부분 rem으로 변경하면 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TextArea를 common한 컴포넌트로 사용할거라면 resize는 props로 받게끔 해주는것도 좋은것 같아요. 근데 현재 디자인에서 resize되는 textArea가 없다고 한다면 굳이 넣을 필요도 없다고 생각합니다! 디자인쪽이랑 얘기해보고 결정하면 될것 같아요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직까진 크게 필요할거 같지않아서, 만약 쓸 상황이 생긴다면 허용해놓도록 하겠습니다!