-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
✔️ Deploy Preview for affectionate-jones-aa31c6 ready! 🔨 Explore the source changes: 87c4940 🔍 Inspect the deploy log: https://app.netlify.com/sites/affectionate-jones-aa31c6/deploys/620769413db0710007910e61 😎 Browse the preview: https://deploy-preview-31--affectionate-jones-aa31c6.netlify.app |
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.
LGTM
|
||
Textarea.defaultProps = { | ||
errorMessage: '', | ||
}; |
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
min-height: 20rem; | ||
padding: 1.2rem 1.4rem; | ||
background-color: ${theme.colors.white}; | ||
border: 1px solid ${theme.colors.gray30}; |
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.
고생하셨습니다 👍
border: 1px solid ${theme.colors.gray30}; | ||
border-radius: 1.2rem; | ||
outline: none; | ||
resize: none; |
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.
아직까진 크게 필요할거 같지않아서, 만약 쓸 상황이 생긴다면 허용해놓도록 하겠습니다!
변경사항
작업 유형
체크리스트