-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add AlphaButton
component
#2182
Conversation
🦋 Changeset detectedLatest commit: f58e049 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
active && styles.active, | ||
className | ||
)} | ||
{...rest} |
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.
onClick 핸들러에 있었던 if (!disabled) 체크는 button 엘리먼트 자체적으로 처리해주고 있어서 생략 가능합니다
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.
L73: 사용처가 없어서 rest prop 안에 포함해도 될 거 같아요
Chromatic Report🚀 Congratulations! Your build was successful! |
de11f00
to
1059e80
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2182 +/- ##
==========================================
- Coverage 86.83% 86.22% -0.62%
==========================================
Files 130 131 +1
Lines 2803 2823 +20
Branches 832 843 +11
==========================================
Hits 2434 2434
- Misses 340 360 +20
Partials 29 29 ☔ View full report in Codecov by Sentry. |
active && styles.active, | ||
className | ||
)} | ||
{...rest} |
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.
L73: 사용처가 없어서 rest prop 안에 포함해도 될 거 같아요
/** | ||
* The text content in the button. | ||
* Do not pass `text` prop if it is an icon-only button. | ||
*/ | ||
text?: string |
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.
- Do not pass
text
prop if it is an icon-only button. : IconButton이 별도로 만들어지므로 불필요한 주석이 됩니다 - 반대로 AlphaButton의
text
prop은 required가 되어야할 거 같아요. 아이콘만 사용할 수는 없으니까요 - 마이너: prop 이름을
text
그대로 사용할지, 다른 컴포넌트에 맞게content
를 사용할지, 아니면children
으로 사용할지(HTML button element 처럼) 고민해보면 좋을 거 같습니다! Figma 스펙이랑도 맞춰보긴 해야할 거 같아요
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.
피그마 상으로는 text 속성이 있어서 그대로 사용해도 될 것 같습니다!
&:where(.style-primary.color-blue:focus-visible) { | ||
outline: 3px solid var(--bgtxt-blue-light); | ||
} | ||
|
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.
새롭게 디자인이 추가되었나보네요 👍
/* color */ | ||
/* stylelint-disable-next-line no-duplicate-selectors */ | ||
&:where(.variant-primary) { | ||
&:where(:not(.color-dark-grey, .color-light-grey)) { |
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.
not 선택자 없어도 되지 않을까요?
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.
어차피 --alpha-color-fg-absolute-white-dark가 #FFF 라서 없어도 된다는 의미일까요? 토큰 value 가 달라질 경우를 생각하면 의미가 있을 것 같았습니다
아하 어차피 뒤에서 overide 되니까 없어도 된다는 의미였군요..! 없애도 될 것 같네요
} | ||
|
||
&:where(.variant-tertiary.color-white) { | ||
:where(.ButtonIcon) { |
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.
:where(.ButtonIcon) { | |
&:where(.ButtonIcon) { |
전반적으로 통일하는 게 일관성 있을 거 같아요
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.
& :where(.ButtonIcon) {
로 하자는 말씀이시죠?? &
처럼 띄어쓰기가 있으면 child 를 선택하는 걸로 이해했습니다 (없는 것과 마찬가지)
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.
아 그렇네요, 잘못 봤네요 😅
|
||
/* background-color */ | ||
&:where(.variant-primary) { | ||
$background-color-by-color: ( |
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.
👍
1be2c9b
to
182b0c8
Compare
…1px gap in s Button
- remove duplicated active, hover selector - add hover style to tertiary&white button
…ed color property
182b0c8
to
f58e049
Compare
<!-- How to write a good PR title: - Follow [the Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/). - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ## Self Checklist - [x] I wrote a PR title in **English** and added an appropriate **label** to the PR. - [x] I wrote the commit message in **English** and to follow [**the Conventional Commits specification**](https://www.conventionalcommits.org/en/v1.0.0/). - [x] I [added the **changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md) about the changes that needed to be released. (or didn't have to) - [x] I wrote or updated **documentation** related to the changes. (or didn't have to) - [x] I wrote or updated **tests** related to the changes. (or didn't have to) - [x] I tested the changes in various browsers. (or didn't have to) - Windows: Chrome, Edge, (Optional) Firefox - macOS: Chrome, Edge, Safari, (Optional) Firefox ## Related Issue <!-- Please link to issue if one exists --> <!-- Fixes #0000 --> - Fixes channel-io#2178 ## Summary <!-- Please brief explanation of the changes made --> - V2 토큰을 사용하여 버튼 컴포넌트를 구현합니다. ## Details <!-- Please elaborate description of the changes --> - 디자인 상으로 Icon 유무와 Floating 여부에 의해서 4가지로 컴포넌트가 나눠져 있어서 버튼 컴포넌트를 non-floating & not-icon-only 인 경우로 제한했습니다. 다만 (Floating)IconButton 처럼 아이콘만 있는 버튼을 분리하는게 좋을지는 아직 고민중입니다. 디자인 스펙에 맞추려면 분리하는게 좋을듯한데, 분리하게 되면 꽤나 겹치는 부분이 있어서 코드적으로 관리 포인트가 2가지 생기는게 우려됩니다. - hover or active 상태인 경우에 대해서는 v2 토큰을 사용한 디자인이 나와있지 않아서 v1 토큰으로 구현하고 버튼 안에 있는 텍스트나 스피너도 기존의 컴포넌트를 사용했습니다. 그런데 기존의 hover 디자인을 확인하니 primary&blue 버튼인 경우에 디자인 스펙과 다른 부분이 있어서 Button 컴포넌트를 수정했습니다. - 스토리북은 우선순위가 높지 않아 보여서 최대한 간단한 형태로 만들었습니다. ### Breaking change? (Yes/No) - No <!-- If Yes, please describe the impact and migration path for users --> ## References <!-- Please list any other resources or points the reviewer should be aware of --> - [버튼(internal)](https://www.figma.com/file/8J76noM3T1Sp5cNPhnYQiG/Action?type=design&node-id=0%3A1&mode=design&t=L4yIeScuZnOADvDx-1)
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @channel.io/[email protected] ### Minor Changes - Update icons ([#2204](#2204)) by @kimminkyung94 Added - center-focus.svg - group-remove-filled.svg - shipping-filled.svg - snooze-filled.svg - star-circle-filled.svg - untag-filled.svg - workflow.svg Modified - chat-check-filled.svg - chat-check.svg - chat-progress-filled.svg - chat-progress.svg - check-circle-filled.svg - check-circle.svg - check-verification-filled.svg - clock-filled.svg - contract.svg - expand.svg - goal.svg - limit.svg - people.svg - tag.svg - untag.svg ## @channel.io/[email protected] ### Patch Changes - Change outline color of primary and blue `Button` component when focused. ([#2182](#2182)) by @yangwooseong - Add `AlphaButton` component. ([#2182](#2182)) by @yangwooseong - Add `AlphaFloatingButton` component. ([#2193](#2193)) by @yangwooseong - Updated dependencies - @channel.io/[email protected] ## @channel.io/[email protected] ### Patch Changes - Add the font-family format supported by Google Web Font ([#2209](#2209)) by @leejiwoo2002 ## [email protected] ### Patch Changes - Updated dependencies - @channel.io/[email protected] - @channel.io/[email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Self Checklist
Related Issue
Summary
Details
Breaking change? (Yes/No)
References