-
Notifications
You must be signed in to change notification settings - Fork 3
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
[Feat] Button 컴포넌트 구현 #162
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
import styled from '@emotion/styled'; | ||
import { Link } from 'react-router-dom'; | ||
import { | ||
BRAND_COLOR, | ||
GRAY_800, | ||
PRIMARY, | ||
SECONDARY, | ||
} from '@/foundations/colors'; | ||
import { BORDER_RADIUS_STYLE, BORDER_STYLE } from '@/foundations/border'; | ||
|
||
interface ButtonProps { | ||
colorTheme?: 'brand' | 'primary' | 'secondary'; | ||
borderTheme?: 'none' | 'dashed'; // 추후 dashed 추가될 수 있음. 기본 solid | ||
size?: 'large' | 'medium' | 'small' | 'fit-content'; | ||
disable?: boolean; | ||
} | ||
|
||
const backgroundMapper = { | ||
brand: BRAND_COLOR, | ||
primary: PRIMARY, | ||
secondary: SECONDARY, | ||
}; | ||
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p2) 현재는 brand, primary, secondary 프로퍼티를 이용하여 객체를 만드셨는데, brand 환경에서 사용할 기본 컬러와 hover 컬러를 묶어서 하나의 객체로 만들고, 같은 방법으로 다른 환경에서 사용 가능한 컬러를 묶어서 객체로 만드는 방법을 생각해 보았는데 어떠실까용?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 제가 이해한 바로는, 객체끼리 묶을 때 현재 배경색 / hover색 // 이런 식으로 묶인 상태인데 brand/primary 등 theme 기준으로 나누는 것에 대한 의견인 것으로 이해했습니다..! 맞을까요? 이전 코드를 다시 보니까 한눈에 들어오기 어려운 부분인 것 같네요 ㅠㅠ 준혁님이 말씀하신 방법을 포함해서 다양하게 고민해보겠습니다! |
||
|
||
const hoverColorMapper = { | ||
brand: 'tomato', | ||
primary: '#3c5997', | ||
secondary: '#aaaaaa', | ||
}; | ||
|
||
const sizeMapper = { | ||
large: '8rem', | ||
medium: '6rem', | ||
small: '4.5rem', | ||
}; | ||
|
||
const fontSizeMapper = { | ||
large: '1.1rem', | ||
medium: '1rem', | ||
small: '0.8rem', | ||
}; | ||
|
||
export const Button = styled.button<ButtonProps>` | ||
width: ${({ size }) => | ||
size === 'fit-content' ? undefined : size && sizeMapper[size]}; | ||
height: 2.5rem; | ||
|
||
border: ${BORDER_STYLE} ${GRAY_800}; | ||
border-style: ${({ borderTheme }) => borderTheme}; | ||
Comment on lines
+47
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p3) 현재 하나는 리터럴, 하나는 함수형으로 변수에 접근을 해서 다소 깨끗하다는 느낌이 들지는 않으나, 오브젝트형으로 style 컴포넌트를 작성한다면, 해결될 문제일 것 같습니다. :) |
||
border-radius: ${BORDER_RADIUS_STYLE}; | ||
|
||
color: ${({ colorTheme }) => (colorTheme === 'primary' ? 'white' : 'black')}; | ||
font-size: ${({ size }) => | ||
size === 'fit-content' ? undefined : size && fontSizeMapper[size]}; | ||
|
||
background-color: ${({ colorTheme }) => | ||
colorTheme && backgroundMapper[colorTheme]}; | ||
opacity: ${({ disable }) => (disable ? `0.8` : `1`)}; | ||
|
||
&:hover { | ||
background-color: ${({ colorTheme, disable }) => | ||
!disable && colorTheme && hoverColorMapper[colorTheme]}; | ||
} | ||
cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)}; | ||
`; | ||
|
||
interface CustomButtonProps { | ||
width?: string; | ||
backgroundColor?: string; | ||
hoverBackgroundColor?: string; | ||
hoverTextColor?: string; | ||
|
||
color?: string; | ||
borderTheme?: 'none' | 'dashed'; // 추후 dashed 추가될 수 있음. 기본 solid | ||
isSquare?: boolean; | ||
disable?: boolean; | ||
} | ||
Comment on lines
+66
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p2) 현재 Button과 CustomButton, 그리고 LinkButton 세 가지 컴포넌트를 정의해 주셨는데, 제 개인적인 의견으로는 Button 컴포넌트 하나만 있으면 될 것 같아요.
<Button>
<Link to="/">바로가기</Link>
</Button> 형식 처럼 children을 받아서 작성할 수 있을 것 같고,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분 구현할 때는, 제가 버튼 구현할 때 기존 CheQuiz에 있는 모든 버튼 모양을 반영하지 않았어서 custom을 두었었습니다. 저도 준혁님 의견에 동의합니다! customButton은 제거하고, 조금 정돈된 공통 버튼 스타일을 추가해보겠습니다. |
||
|
||
export const CustomButton = styled.button<CustomButtonProps>` | ||
height: 2.5rem; | ||
width: ${({ width }) => width}; | ||
|
||
color: ${({ color }) => color}; | ||
background-color: ${({ backgroundColor }) => backgroundColor}; | ||
&:hover { | ||
background-color: ${({ hoverBackgroundColor, disable }) => | ||
!disable && `${hoverBackgroundColor}`}; | ||
color: ${({ hoverTextColor }) => hoverTextColor}; | ||
} | ||
|
||
border: ${BORDER_STYLE} ${GRAY_800}; | ||
border-style: ${({ borderTheme }) => borderTheme}; | ||
border-radius: ${({ isSquare }) => (isSquare ? 0 : BORDER_RADIUS_STYLE)}; | ||
|
||
opacity: ${({ disable }) => (disable ? `0.8` : `1`)}; | ||
cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)}; | ||
`; | ||
|
||
export const ButtonLink = styled(Link)<ButtonProps>` | ||
display: inline-flex; | ||
justify-content: center; | ||
align-items: center; | ||
|
||
width: ${({ size }) => | ||
size === 'fit-content' ? undefined : size && sizeMapper[size]}; | ||
height: 2.5rem; | ||
|
||
border: ${BORDER_STYLE} ${GRAY_800}; | ||
border-style: ${({ borderTheme }) => borderTheme}; | ||
border-radius: ${BORDER_RADIUS_STYLE}; | ||
|
||
color: ${({ colorTheme }) => (colorTheme === 'primary' ? 'white' : 'black')}; | ||
font-size: ${({ size }) => | ||
size === 'fit-content' ? undefined : size && fontSizeMapper[size]}; | ||
text-decoration: none; | ||
|
||
background-color: ${({ colorTheme }) => | ||
colorTheme && backgroundMapper[colorTheme]}; | ||
opacity: ${({ disable }) => (disable ? `0.8` : `1`)}; | ||
|
||
&:hover { | ||
background-color: ${({ colorTheme, disable }) => | ||
!disable && colorTheme && hoverColorMapper[colorTheme]}; | ||
} | ||
cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)}; | ||
`; | ||
export const CustomButtonLink = styled(Link)<CustomButtonProps>` | ||
display: inline-flex; | ||
justify-content: center; | ||
align-items: center; | ||
text-decoration: none; | ||
|
||
height: 2.5rem; | ||
width: ${({ width }) => width}; | ||
|
||
color: ${({ color }) => color}; | ||
background-color: ${({ backgroundColor }) => backgroundColor}; | ||
&:hover { | ||
background-color: ${({ hoverBackgroundColor, disable }) => | ||
!disable && `${hoverBackgroundColor}`}; | ||
color: ${({ hoverTextColor }) => hoverTextColor}; | ||
} | ||
|
||
border: ${BORDER_STYLE} ${GRAY_800}; | ||
border-style: ${({ borderTheme }) => borderTheme}; | ||
border-radius: ${({ isSquare }) => (isSquare ? 0 : BORDER_RADIUS_STYLE)}; | ||
|
||
opacity: ${({ disable }) => (disable ? `0.8` : `1`)}; | ||
cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)}; | ||
Comment on lines
+147
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. p2) 해당 프로퍼티들이 각 컴포넌트에서 모두 확인할 수 있는데, 이 부분을 공통적인 스타일로 빼면 어떨까요? const style = css`
opacity: ${({ disable }) => (disable ? `0.8` : `1`)};
cursor: ${({ disable }) => (disable ? `not-allowed` : `pointer`)};
` 하는 방식은 어떨까요? 이런 방식으로 사용할 경우, 다음과 같이 사용할 수 있어요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 보내주신 링크 봤는데, 정말 깔끔하고 이해하기 편하네요....! 역시 준혁님 팀 👍 이 코드를 보고 제 코드를 보니까 수정해야 할 부분이 많이 보이는 것 같아요.. 전반적으로 준혁님의 리뷰의 방향성 & 내용을 보니까 수정해야 할 부분을 많이 알 수 있었습니다. DX 부분을 많이 개선해보도록 할게요! 링크 감사합니다 + 추석 때 일이 있어 답변이 늦어졌습니다 ㅠㅠ 리팩토링 때 반영될 부분인 것 같아 그때까지 느리지만 더 쓰기 좋게 고쳐볼게요..! |
||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import { | ||
Button, | ||
ButtonLink, | ||
CustomButton, | ||
CustomButtonLink, | ||
} from '@/designs/Button'; | ||
|
||
function DesignPage() { | ||
return ( | ||
<div> | ||
<h1>버튼 디자인 시스템</h1> | ||
<h1>colorTheme: brand</h1> | ||
<Button colorTheme="brand" size="small"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="brand" size="medium"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="brand" size="large"> | ||
댓글 쓰기 | ||
</Button> | ||
<br /> | ||
<h1>colorTheme: primary</h1> | ||
<Button colorTheme="primary" size="small"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="primary" size="medium"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="primary" size="large"> | ||
댓글 쓰기 | ||
</Button> | ||
<br /> | ||
<h1>colorTheme: secondary</h1> | ||
<Button colorTheme="secondary" size="small"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="secondary" size="medium"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="secondary" size="large"> | ||
댓글 쓰기 | ||
</Button> | ||
<br /> | ||
<h1>borderTheme과 buttonSize: fit-content</h1> | ||
<Button colorTheme="secondary" size="medium" borderTheme="dashed"> | ||
댓글 쓰기 | ||
</Button> | ||
<Button colorTheme="secondary" size="fit-content"> | ||
아주 긴 버튼을 만들 때, 내부 값에 따라서 크기가 정해집니다. | ||
</Button> | ||
<br /> | ||
<h1>button에 disable 상태 줄 때 모습</h1> | ||
<Button colorTheme="brand" size="large" disable> | ||
비활성화 | ||
</Button> | ||
<Button colorTheme="primary" size="large" disable> | ||
비활성화 | ||
</Button> | ||
<Button colorTheme="secondary" size="large" disable> | ||
비활성화 | ||
</Button> | ||
<h1>커스텀 버튼</h1> | ||
<CustomButton | ||
isSquare | ||
borderTheme="none" | ||
color="white" | ||
backgroundColor="tomato" | ||
hoverBackgroundColor="antiqueWhite" | ||
hoverTextColor="brown" | ||
> | ||
커스텀 버튼 | ||
</CustomButton> | ||
<h1>버튼-링크</h1> | ||
<ButtonLink to="/" colorTheme="secondary" size="large"> | ||
버튼링크 | ||
</ButtonLink> | ||
<ButtonLink to="/" colorTheme="brand" size="medium" disable> | ||
버튼링크 | ||
</ButtonLink> | ||
<CustomButtonLink | ||
to="/" | ||
color="white" | ||
backgroundColor="royalblue" | ||
hoverBackgroundColor="tomato" | ||
width="300px" | ||
> | ||
커스텀버튼링크 | ||
</CustomButtonLink> | ||
</div> | ||
); | ||
} | ||
|
||
export default DesignPage; |
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.
p2) 바뀐 rule에 의하면
interface Props {
로 이름을 붙여줘야 할 것 같은데 어떠신가요??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.
아 그렇네요..! 리뷰 받기 전에 바꾼다는게 계속되는 야근으로 바꾸지 못했습니다 ㅠㅠ 바꿔둘게요!! 감사합니다