-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/component : Button 컴포넌트, color pallet #20
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
Conversation
|
💄 Storybook: https://67c9a019e2c059ab48fd9565-gkypxamaxe.chromatic.com/ |
| "@mdx-js/react": "^3.1.0", | ||
| "@mdx-js/rollup": "^3.1.0", |
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.
스토리북에서 mdx 파일 작성할때 필요하다고 해서 설치했고 import는 안해도 된다고 하는데.. 없어도 되는건가?
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.
https://storybook.js.org/docs/writing-docs/mdx
공식문서엔 다른느낌인데 나중에 mdx를 활용할거면 둬도 될거같어
| const meta: Meta<typeof Button> = { | ||
| component: Button, | ||
| title: "UI/Button", | ||
| tags: ["!autodocs"], |
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.
config에는 autodocs 해놨는데 여기서는 왜 false 시킨거야?
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.
음 이 스토리에서는 docs 필요 없다고 생각해서.. docs 여부는 통일하는게 좋은가요?
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.
오빠가 만들어둔 샘플 스토리 보니 Docs 를 내가 잘 못쓰고 있었네 ㅎ
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.
나도 엄청 기본만 쓰는 형식이긴해
| className={`${buttonStyle[args.variant]} ${args.className}`} | ||
| {...args} | ||
| > | ||
| {args.icon} |
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.
icon은 뒤에 올수도 있음
| return ( | ||
| <button | ||
| type={type} | ||
| className={`${buttonStyle[args.variant]} ${args.className}`} |
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.
args.className이 undefined가 되면 class에 undefined가 들어가버려서
undefined인 경우 처리가 필요함
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.
- font pretendard 적용 필요
- text style 정의가 안되어 있음
- active도 disabled 일 때 되면 안됨
- disabled 상태일 때에 pointer가 cursor면 안됨
- 적절한 transition을 추가했으면 함
- px을 추가
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.
css :not 을 이용하면 코드가 훨씬 간결해지고
recipe compoundVariants를 이용해서 분기처리한다면 안전해지는 장점이 있음
원하는 방식으로 진행하면 좋을것 같음
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.
recipe 로 써보려고 했는데 사용법이 미숙해서 우선 :not 으로 처리했음니다
| alignItems: "center", | ||
| gap: "4px", | ||
| borderRadius: "8px", | ||
| minHeight: "40px", |
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.
min 값으로 해줘야하는 이유는 뭔가요? children으로 뭐가들어올지 몰라서?
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.
사실 우리 스펙에는 height로 해도 되긴한데 너 말대루 children에 높이가 큰 아이콘이나 children이 오는 케이스를 고려하는거지 그런 케이스를 고려했을 때에 height를 고정하는건 좋은 방향성은 아니라서
| fontWeight: "600", | ||
| lineHeight: "17px", | ||
|
|
||
| outlineOffset: "-1px", |
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 를 쓰는 경우
border의 경우에는 height를 정했기 때문에 높이값은 변하지 않지만 auto인 width에는 border 때문에 늘어나버림. 이런경우 border가 없는 케이스(primary 등)와 ui가 달라지는 현상이 발생함
2025-03-25.11.51.50.mov
- outline + 음수 outline-offset 를 쓰는 경우
outline은 box-sizing과 무관하게 무조건 영역 바깥쪽에 px만큼의 outline을 만듦. 즉, 너비나 높이에 영향을 주는 요소가 아님
이 때, outline-offset을 outline의 크기만큼 음수로 주게 되면 영역 안쪽으로 outline을 배치시킬 수 있음. 이로써 border를 대체함
2025-03-25.11.54.00.mov
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.
아~ base 에서 1px tranparent 로 주는것보단 이게 더 권장되는 방법이야?
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.
transparent로 주더라도 width에는 투명한 1px이 생기는데, 이러면 옆에 만약에 무언가 있다면 불필요한 gap이 생기겠지
|
|
||
| fontSize: "14px", | ||
| fontWeight: "600", | ||
| lineHeight: "17px", |
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.
lineHeight 피그마에 나와있어? 내가 볼땐 auto로 되어있어서.
아니면 적절한 값으로해?
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.
figma dev mode가 안되니깐 불편하긴 하다. dev mode면 다보이는데.,..
아래방법으로로 확인할 수 있어
2025-03-25.11.47.23.mov
packages/ui/src/styles/global.css.ts
Outdated
| backgroundColor: Color.bg, | ||
| color: Color.text.default, | ||
| // font 설정 추가 | ||
| fontFamily: font, |
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.
@vanilla-extract/css의 style은 css의 값에 넣는게 아니라 class에 넣는거임. 너가 만든 style 들을 생각해봐
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.
그냥 string으로 "Pretendard" 로 넣거나 const pretendard = "Pretendard"; 를 export 시켜서 import 해서 넣으면 됨
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.
icon, children, className, variant는 args에서 빼서 쓰는게 코드가 더 간결해질거 같음
variant, iconPosition은 default value가 있는게 편할거야 매번 입력하기 싫다면
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.
반영했습니다~!
jongjunpark
left a comment
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.
👍
Button 컴포넌트
Color pallet 스토리북에 추가