-
Notifications
You must be signed in to change notification settings - Fork 150
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: add ButtonGroup component #2064
Conversation
🦋 Changeset detectedLatest commit: eb5283b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ PR title follows Conventional Commits specification. |
Bundle Size ReportUpdated Components
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit eb5283b:
|
f6291c1
to
206b1d3
Compare
// throw error if child is not a button or dropdown with button trigger | ||
/* eslint-disable no-restricted-properties */ | ||
if ( | ||
!isValidAllowedChildren(child, '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.
Can we use componentIds.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.
!isValidAllowedChildren(child, 'Button') && | |
!isValidAllowedChildren(child, componentIds.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.
Build is failing, unable to see storybook preview.
@anuraghazra Build is successful, for some reason it's not coming in CI checks as usual. Preview Link - https://61c19ee8d3d282003ac1d81c-mdcfdyzkei.chromatic.com/ |
theme, | ||
color, | ||
color: buttonGroupProps.color ?? 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.
expected to give ButtonGroup a priority over individual 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.
Yes, all buttons should have the same color in a button group.
packages/blade/src/components/ButtonGroup/StyledButtonGroup.tsx
Outdated
Show resolved
Hide resolved
60fffb7
to
009523b
Compare
@@ -0,0 +1 @@ | |||
export * from './ButtonGroup'; |
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.
export the types too.
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.
It's already exported.
blade/packages/blade/src/components/ButtonGroup/ButtonGroup.web.tsx
Lines 149 to 150 in eb5283b
export { ButtonGroup }; | |
export type { ButtonGroupProps }; |
test: add ButtonGroup test cases (#2068)
Description
Adds new
ButtonGroup
component to Blade.Changes
Component Checklist