[Feat] Button 컴포넌트 구현 #162
Conversation
| } from '@/foundations/colors'; | ||
| import { BORDER_RADIUS_STYLE, BORDER_STYLE } from '@/foundations/border'; | ||
|
|
||
| interface ButtonProps { |
There was a problem hiding this comment.
p2) 바뀐 rule에 의하면 interface Props { 로 이름을 붙여줘야 할 것 같은데 어떠신가요??
There was a problem hiding this comment.
아 그렇네요..! 리뷰 받기 전에 바꾼다는게 계속되는 야근으로 바꾸지 못했습니다 ㅠㅠ 바꿔둘게요!! 감사합니다
| const backgroundMapper = { | ||
| brand: BRAND_COLOR, | ||
| primary: PRIMARY, | ||
| secondary: SECONDARY, | ||
| }; |
There was a problem hiding this comment.
p2) 현재는 brand, primary, secondary 프로퍼티를 이용하여 객체를 만드셨는데, brand 환경에서 사용할 기본 컬러와 hover 컬러를 묶어서 하나의 객체로 만들고, 같은 방법으로 다른 환경에서 사용 가능한 컬러를 묶어서 객체로 만드는 방법을 생각해 보았는데 어떠실까용??
There was a problem hiding this comment.
제가 이해한 바로는, 객체끼리 묶을 때 현재 배경색 / hover색 // 이런 식으로 묶인 상태인데 brand/primary 등 theme 기준으로 나누는 것에 대한 의견인 것으로 이해했습니다..! 맞을까요? 이전 코드를 다시 보니까 한눈에 들어오기 어려운 부분인 것 같네요 ㅠㅠ 준혁님이 말씀하신 방법을 포함해서 다양하게 고민해보겠습니다!
| border: ${BORDER_STYLE} ${GRAY_800}; | ||
| border-style: ${({ borderTheme }) => borderTheme}; |
There was a problem hiding this comment.
p3) 현재 하나는 리터럴, 하나는 함수형으로 변수에 접근을 해서 다소 깨끗하다는 느낌이 들지는 않으나, 오브젝트형으로 style 컴포넌트를 작성한다면, 해결될 문제일 것 같습니다. :)
| interface CustomButtonProps { | ||
| width?: string; | ||
| backgroundColor?: string; | ||
| hoverBackgroundColor?: string; | ||
| hoverTextColor?: string; | ||
|
|
||
| color?: string; | ||
| borderTheme?: 'none' | 'dashed'; // 추후 dashed 추가될 수 있음. 기본 solid | ||
| isSquare?: boolean; | ||
| disable?: boolean; | ||
| } |
There was a problem hiding this comment.
p2) 현재 Button과 CustomButton, 그리고 LinkButton 세 가지 컴포넌트를 정의해 주셨는데, 제 개인적인 의견으로는 Button 컴포넌트 하나만 있으면 될 것 같아요.
- 일단, 링크 버튼 같은 경우는
<Button>
<Link to="/">바로가기</Link>
</Button>형식 처럼 children을 받아서 작성할 수 있을 것 같고,
- CustomButton의 경우, 개발자에게 너무 많은 자율성을 주게 된다면, 디자인 적으로 오히려 통일감을 해칠 수 있다는 생각이 들어요. 필요한 경우가 더 많이 생긴다면, 그 때 커스텀 버튼을 기존 버튼에서 style을 extends 하는 방향으로 작성해 보는 것도 좋을 것 같아요.
There was a problem hiding this comment.
이 부분 구현할 때는, 제가 버튼 구현할 때 기존 CheQuiz에 있는 모든 버튼 모양을 반영하지 않았어서 custom을 두었었습니다. 저도 준혁님 의견에 동의합니다! customButton은 제거하고, 조금 정돈된 공통 버튼 스타일을 추가해보겠습니다.
| opacity: ${({ disable }) => (disable ? `0.8` : `1`)}; | ||
| cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)}; |
There was a problem hiding this comment.
p2) 해당 프로퍼티들이 각 컴포넌트에서 모두 확인할 수 있는데, 이 부분을 공통적인 스타일로 빼면 어떨까요?
const style = css`
opacity: ${({ disable }) => (disable ? `0.8` : `1`)};
cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)};
`하는 방식은 어떨까요?
이런 방식으로 사용할 경우, 다음과 같이 사용할 수 있어요.
참고
There was a problem hiding this comment.
보내주신 링크 봤는데, 정말 깔끔하고 이해하기 편하네요....! 역시 준혁님 팀 👍 이 코드를 보고 제 코드를 보니까 수정해야 할 부분이 많이 보이는 것 같아요.. 전반적으로 준혁님의 리뷰의 방향성 & 내용을 보니까 수정해야 할 부분을 많이 알 수 있었습니다. DX 부분을 많이 개선해보도록 할게요! 링크 감사합니다 + 추석 때 일이 있어 답변이 늦어졌습니다 ㅠㅠ 리팩토링 때 반영될 부분인 것 같아 그때까지 느리지만 더 쓰기 좋게 고쳐볼게요..!
📌 PR 설명
디자인시스템을 적용한 + 확장성이 있는 버튼 컴포넌트를 구현했습니다.
button과link가 있어, 양쪽 모두 사용할 수 있도록 컴포넌트를 별도 정의하였습니다.colorTheme,borderThemesizedisable을 받습니다.만든 모습 미리 보기
/test/design으로 들어가시면 보실 수 있습니다.💻 요구 사항과 구현 내용
03c7b3b 기본 버튼 시스템을 구현했습니다.
029fec6 구현한 버튼 관련 테스트 화면을 제작했습니다.
dea2c5b Link로도 버튼과 같은 스타일로 동작하도록 추가했습니다.
✔️ PR 포인트 & 궁금한 점
close [Feat] Button 컴포넌트 구현 #155