-
Notifications
You must be signed in to change notification settings - Fork 861
[EuiSplitButton] Add split button component #9269
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
ece7064 to
646eb83
Compare
|
Looks great @mgadewoll ! One concern I've is to don't lock the icon to
Could we override the And maybe also the other way around, could we avoid to use the |
@JoseLuisGJ
For this case, I'd not advise to prevent passing |
- ensure child prop is applied if the parent is set to false
acstll
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.
Only a few suggestions, almost all non-blocking and most of them regarding the docs. Awesome implementation, thank you @mgadewoll
packages/website/docs/components/navigation/buttons/split_button.mdx
Outdated
Show resolved
Hide resolved
packages/website/docs/components/navigation/buttons/split_button.mdx
Outdated
Show resolved
Hide resolved
packages/eui/src/components/button/split_button/split_button.tsx
Outdated
Show resolved
Hide resolved
| export const EuiSplitButton = Object.assign(_EuiSplitButton, { | ||
| ActionPrimary: EuiSplitButtonActionPrimary, | ||
| ActionSecondary: EuiSplitButtonActionSecondary, | ||
| }); |
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.
[no action required] I really like this pattern, hopefully we can discuss using this more moving forward?
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, let's open that as general discussion for further future features/refactors. We could either do it async or as part of a weekly. 👍
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.
Neat! I love how easy Object.assign makes the type to resolve correctly 🎉
| ); | ||
|
|
||
| return popoverProps ? ( | ||
| <EuiPopover anchorPosition="downCenter" {...popoverProps} button={action} /> |
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.
[non-blocking] since we don't have the arrows anymore, I would make this downRight / @JoseLuisGJ @ryankeairns what do you think?
| <EuiPopover anchorPosition="downCenter" {...popoverProps} button={action} /> | |
| <EuiPopover anchorPosition="downRight" {...popoverProps} button={action} /> |
acstll
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.
🟢 Awesome work, thanks a lot for addressing my comments!
I left a couple more of non-blocking suggestion. Approving now anyways. Also let's wait for @tkajtoch's review.
packages/website/docs/components/navigation/buttons/split_button.mdx
Outdated
Show resolved
Hide resolved
packages/eui/src/components/button/split_button/split_button.tsx
Outdated
Show resolved
Hide resolved
7830648 to
83bccc3
Compare
packages/eui/src/components/button/split_button/split_button.tsx
Outdated
Show resolved
Hide resolved
packages/eui/src/components/button/split_button/split_button.tsx
Outdated
Show resolved
Hide resolved
tkajtoch
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.
The new split button component looks really nice ✨ I love the way you implemented it!
I confirmed all props work as expected and match the design spec.
I left a couple small comments. LMK what you think!
tkajtoch
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.
Let's go!
💚 Build SucceededHistory
cc @mgadewoll |
💚 Build Succeeded
History
cc @mgadewoll |
## Dependency updates - `@elastic/eui`: `v111.0.0` ⏩ `v111.1.0` - `@elastic/eui-theme-borealis`: `v5.2.0` ⏩ `v5.3.0` --- ## Changes - Removed `euiBasicTable.tableCaptionWithPagination`, `euiBasicTable.tableAutoCaptionWithPagination`, `euiBasicTable.tableSimpleAutoCaptionWithPagination`, `euiBasicTable.tableAutoCaptionWithoutPagination` i18n tokens - Added `euiBasicTable.caption.itemCountPart.withTotalItemCount`, `euiBasicTable.caption.paginationPart.withPageCount`, `euiBasicTable.caption.tableName`, `euiBasicTable.caption.emptyState` i18n tokens - Updated snapshot tests - Updated a couple of Jest assertions due to [this EUI change](https://github.com/elastic/eui/pull/9254/changes)), see 127ab80 ## Package updates ### `@elastic/eui` [v111.1.0](https://github.com/elastic/eui/releases/tag/v111.1.0) - Added `dashedCircle` icon ([#9278](elastic/eui#9278)) - Added `crossProjectSearch` icon ([#9275](elastic/eui#9275)) - Added component token `components.tourStepIndicatorInactiveColor` and `components.tourStepIndicatorActiveColor` ([#9271](elastic/eui#9271)) - Remapped `EuiBeacon` component `success` variant to use `success` color token instead of `accentSecondary` ([#9271](elastic/eui#9271)) - Added `EuiSplitButton` and its respective sub-components `EuiSplitButton.ActionPrimary` and `EuiSplitButton.ActionSecondary` ([#9269](elastic/eui#9269)) - Added `productRobot` icon ([#9259](elastic/eui#9259)) - Added beta `euiContainer()`, `euiContainerCSS()`, and `euiContainerQuery()` Emotion utilities to help work with CSS Container Queries ([#9264](elastic/eui#9264)) - Added `useEuiContainerQuery` hook to observe container query changes in JavaScript ([#9251](elastic/eui#9251)) - Updated EuiFlexGroup's `gutterSize` from `l` to `m` ([#9132](elastic/eui#9132)) - Updated EuiSpacer's `size` from `l` to `m` ([#9132](elastic/eui#9132)) - Updated EuiHorizontalRule's `margin` from `l` to `m` ([#9132](elastic/eui#9132)) - Updated EuiPageHeader's tab `size` from `l` to `m` ([#9132](elastic/eui#9132)) - Updated EuiEmptyPrompt's spacer `size` between title and text from `m` to `s` ([#9132](elastic/eui#9132)) - Updated EuiSearchBar's `gutterSize` from `m` to `s` ([#9132](elastic/eui#9132)) <img width="2158" height="392" alt="image" src="https://github.com/user-attachments/assets/e217f5a5-5b4f-48df-830d-a60861939945" /> <img width="1692" height="608" alt="image" src="https://github.com/user-attachments/assets/d1f49e86-ad8c-4d80-9d02-54c73baae616" /> <img width="2182" height="302" alt="image" src="https://github.com/user-attachments/assets/295c768c-a2df-48f5-80cb-1a1ce5b19e00" /> <img width="1904" height="1066" alt="image" src="https://github.com/user-attachments/assets/f96cbde6-0529-4f5b-be5b-b56f28c5d2b7" /> <img width="1566" height="128" alt="image" src="https://github.com/user-attachments/assets/be4df105-9e32-4a9d-89f0-fe973f441495" /> **Bug fixes** - Fixed flyout overlay masks not being visible for `EuiDataGrid`'s fullscreen mode by reducing the `z-index` of the fullscreen mode overlay ([#9267](elastic/eui#9267)) **Accessibility** - Added information about the empty state of `EuiBasicTable` in the table caption ([#9265](elastic/eui#9265)) - Improved `EuiBasicTable` accessibility by ensuring a fallback `tableCaption` is applied if none is provided ([#9254](elastic/eui#9254)) ### `@elastic/eui-theme-borealis` v5.3.0 - Added component token `components.tourStepIndicatorInactiveColor` and `components.tourStepIndicatorActiveColor` ([#9271](elastic/eui#9271))
## Dependency updates - `@elastic/eui`: `v111.0.0` ⏩ `v111.1.0` - `@elastic/eui-theme-borealis`: `v5.2.0` ⏩ `v5.3.0` --- ## Changes - Removed `euiBasicTable.tableCaptionWithPagination`, `euiBasicTable.tableAutoCaptionWithPagination`, `euiBasicTable.tableSimpleAutoCaptionWithPagination`, `euiBasicTable.tableAutoCaptionWithoutPagination` i18n tokens - Added `euiBasicTable.caption.itemCountPart.withTotalItemCount`, `euiBasicTable.caption.paginationPart.withPageCount`, `euiBasicTable.caption.tableName`, `euiBasicTable.caption.emptyState` i18n tokens - Updated snapshot tests - Updated a couple of Jest assertions due to [this EUI change](https://github.com/elastic/eui/pull/9254/changes)), see elastic@127ab80 ## Package updates ### `@elastic/eui` [v111.1.0](https://github.com/elastic/eui/releases/tag/v111.1.0) - Added `dashedCircle` icon ([elastic#9278](elastic/eui#9278)) - Added `crossProjectSearch` icon ([elastic#9275](elastic/eui#9275)) - Added component token `components.tourStepIndicatorInactiveColor` and `components.tourStepIndicatorActiveColor` ([elastic#9271](elastic/eui#9271)) - Remapped `EuiBeacon` component `success` variant to use `success` color token instead of `accentSecondary` ([elastic#9271](elastic/eui#9271)) - Added `EuiSplitButton` and its respective sub-components `EuiSplitButton.ActionPrimary` and `EuiSplitButton.ActionSecondary` ([elastic#9269](elastic/eui#9269)) - Added `productRobot` icon ([elastic#9259](elastic/eui#9259)) - Added beta `euiContainer()`, `euiContainerCSS()`, and `euiContainerQuery()` Emotion utilities to help work with CSS Container Queries ([elastic#9264](elastic/eui#9264)) - Added `useEuiContainerQuery` hook to observe container query changes in JavaScript ([elastic#9251](elastic/eui#9251)) - Updated EuiFlexGroup's `gutterSize` from `l` to `m` ([elastic#9132](elastic/eui#9132)) - Updated EuiSpacer's `size` from `l` to `m` ([elastic#9132](elastic/eui#9132)) - Updated EuiHorizontalRule's `margin` from `l` to `m` ([elastic#9132](elastic/eui#9132)) - Updated EuiPageHeader's tab `size` from `l` to `m` ([elastic#9132](elastic/eui#9132)) - Updated EuiEmptyPrompt's spacer `size` between title and text from `m` to `s` ([elastic#9132](elastic/eui#9132)) - Updated EuiSearchBar's `gutterSize` from `m` to `s` ([elastic#9132](elastic/eui#9132)) <img width="2158" height="392" alt="image" src="https://github.com/user-attachments/assets/e217f5a5-5b4f-48df-830d-a60861939945" /> <img width="1692" height="608" alt="image" src="https://github.com/user-attachments/assets/d1f49e86-ad8c-4d80-9d02-54c73baae616" /> <img width="2182" height="302" alt="image" src="https://github.com/user-attachments/assets/295c768c-a2df-48f5-80cb-1a1ce5b19e00" /> <img width="1904" height="1066" alt="image" src="https://github.com/user-attachments/assets/f96cbde6-0529-4f5b-be5b-b56f28c5d2b7" /> <img width="1566" height="128" alt="image" src="https://github.com/user-attachments/assets/be4df105-9e32-4a9d-89f0-fe973f441495" /> **Bug fixes** - Fixed flyout overlay masks not being visible for `EuiDataGrid`'s fullscreen mode by reducing the `z-index` of the fullscreen mode overlay ([elastic#9267](elastic/eui#9267)) **Accessibility** - Added information about the empty state of `EuiBasicTable` in the table caption ([elastic#9265](elastic/eui#9265)) - Improved `EuiBasicTable` accessibility by ensuring a fallback `tableCaption` is applied if none is provided ([elastic#9254](elastic/eui#9254)) ### `@elastic/eui-theme-borealis` v5.3.0 - Added component token `components.tourStepIndicatorInactiveColor` and `components.tourStepIndicatorActiveColor` ([elastic#9271](elastic/eui#9271))


Summary
closes #8942
Note
The component was initially PoC'd here and is currently implemented as custom component on Kibana side here.
This PR introduces a new component to EUI:
EuiSplitButton. This component provides its two specialized children components as sub-componentsEuiSplitButton.ActionPrimaryandEuiSplitButton.ActionSecondary. These inherit fromEuiButtonandEuiButtonIconrespectively and define expected behaviors. However,EuiSplitButtoncontrols a few props across both sub-components, overriding the sub-component props:color,size,fill.API
EuiSplitButton
children[<EuiSplitButton.ActionPrimary>, <EuiSplitButton.ActionSecondary>]undefinedcolorEuiButtonProps['color']primarysizeEuiButtonProps['size']mfillEuiButtonProps['fill']falseisDisabledEuiDisabledProps['isDisabled]falseisLoadingEuiButtonProps['isLoading]falseEuiSplitButton.ActionPrimary (inherits props from
EuiButtonorEuiButtonIcon(depending onisIconOnly))isIconOnlybooleanEuiButtonIcontooltipPropsEuiToolTipPropsundefinedEuiToolTiparound the action button and passes along the tooltip propsEuiSplitButton.ActionSecondary ( inherits props from
EuiButtonIcon)tooltipPropsEuiToolTipPropsundefinedEuiToolTiparound the action button and passes along the tooltip propspopoverPropsEuiPopoverPropsundefinedEuiPopoveraround the action button and passes along the popover propsWhy are we making this change?
✨ Serving feature request: Providing a new component based on feature requests on Kibana side.
Screenshots #
default
high contrast mode (
prefers-contrast)Windows high contrast themes (
forced-colors)Impact to users
🟢 No updates required. This is a new feature.
ℹ️ The new component was tested as stand-in replacement for the custom implementation in Kibana here
QA
📖 Documentation
General checklist
@defaultif default values are missing) and playground toggles