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

feature: Button 컴포넌트 구현 #17

Merged
merged 2 commits into from
Jul 26, 2023
Merged

feature: Button 컴포넌트 구현 #17

merged 2 commits into from
Jul 26, 2023

Conversation

bsy1141
Copy link
Collaborator

@bsy1141 bsy1141 commented Jul 23, 2023

👀 What is this PR?

Button 컴포넌트 구현 (Button, TextButton 구분해서 구현했음다)

📝 Changes

  • Button 종류가 primary, secondary, nudge, link, rounded(add 버튼) 5개인데 일단 color props로 해당 종류를 구분해서 받을 수 있도록 해두었고, 더 명확한 props가 있으면 좋겠다는 생각.. or 아니면 좀 더 명확하게 버튼을 아예 구분짓고 컴파운드 형식으로 구현할까 고민 중! 의견 받습니다🙋🏻‍♀️
  • 몬가 prettier가 자꾸 적용이 안되는 것 같긴한데... 일단은 올려둔 상황. 이거는 확인해볼게!
  • ts와 clsx가 아주 낯설어서 평소에 하던대로 Map Object만들어서 type,color 매핑하는 방식으로 구현해두었는데 Button 컴포넌트 더 깔끔하게 만들 수 있다면 좋을 것 같아서 ts 장인 윤규의 도움을 받고자 합니다ㅎ 코드리뷰 잘 부탁드려요~~
  • 아 그리고 ChevronRight 아이콘을 추가했는데 Icon의 fill, className 속성 때문에 아이콘에 색깔이 fill되서 들어가는 거 같아! 확인해주시면 감사하겠습니다.

📌 Related issue(s)

close #7

📷 Attachment(optional)

스크린샷 2023-07-24 오전 12 57 25

@bsy1141 bsy1141 added the 🌟 feature new feature label Jul 23, 2023
@bsy1141 bsy1141 requested a review from asdf99245 July 23, 2023 16:03
@bsy1141 bsy1141 self-assigned this Jul 23, 2023
@asdf99245
Copy link
Collaborator

Button 종류가 primary, secondary, nudge, link, rounded(add 버튼) 5개인데 일단 color props로 해당 종류를 구분해서 받을 수 있도록 해두었고, 더 명확한 props가 있으면 좋겠다는 생각.. or 아니면 좀 더 명확하게 버튼을 아예 구분짓고 컴파운드 형식으로 구현할까 고민 중! 의견 받습니다🙋🏻‍♀️

개인적으로 color props보단 형태에 가까우니깐 variant props를 사용하는거 좋은거같아 -> 보통 여러개 형태가 있을때 이거쓰긴 했어
컴파운드는 Button.Text, Button.Link 요런느낌인건가?

몬가 prettier가 자꾸 적용이 안되는 것 같긴한데... 일단은 올려둔 상황. 이거는 확인해볼게!

흐음.. 왜 그렇지 만약에 당장 안되면 일단 내가 formating 한번 해도 되고

아 그리고 ChevronRight 아이콘을 추가했는데 Icon의 fill, className 속성 때문에 아이콘에 색깔이 fill되서 들어가는 거 같아! 확인해주시면 감사하겠습니다.

요거 복붙하면 될거같아!

<svg viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg">
<path d="M8.08933 5.41748L15.9107 11.9999L8.08933 18.5823" stroke="currentColor" fill="transparent"/>
</svg>

로컬에서 해보니깐 요렇게 하면 되더라 path에 fill이 먹혀서 안에도 채워진거같은데 그냥 stroke fill을 transparent로 박아줬어

src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
src/components/shared/icon/Icon.tsx Outdated Show resolved Hide resolved
src/assets/icons/right-chevron.svg Outdated Show resolved Hide resolved
src/components/shared/Button/Button.tsx Outdated Show resolved Hide resolved
@bsy1141 bsy1141 requested a review from asdf99245 July 24, 2023 15:33
@asdf99245
Copy link
Collaborator

asdf99245 commented Jul 25, 2023

LGTM DDb 👍 요거 merge 하면될듯!

PR에 issue mapping에 close #7로만 해줘~

@bsy1141 bsy1141 merged commit f4fae4b into main Jul 26, 2023
@bsy1141 bsy1141 deleted the feature/button branch July 26, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: button 컴포넌트
2 participants