Add Split Button component #13
Conversation
Co-authored-by: YieldRay <24633623+YieldRay@users.noreply.github.com>
Co-authored-by: YieldRay <24633623+YieldRay@users.noreply.github.com>
WalkthroughIntroduces a new SplitButton component with Material Design 3 compliance, supporting four color variants (outlined, filled, elevated, tonal), five sizes (xs, s, m, l, xl), optional leading icons, labels, and integrated menu rendering. Includes comprehensive styling with per-variant and per-size configurations, accessibility features, and Storybook documentation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/split-button/split-button.scss (1)
110-134: Consolidate duplicate selectors for cleaner SCSS.The
.sd-split_button-leadingand.sd-split_button-trailingselectors appear twice within the same size block (lines 110-113 then 121-124 for leading, lines 115-118 then 126-129 for trailing). This pattern repeats across all size configurations.Consider merging the padding and border-radius declarations into single selector blocks:
&-size-xs { height: 24px; border-radius: 12px; button { font-size: 11px; line-height: 16px; } .sd-split_button-icon svg { width: 16px; height: 16px; } .sd-split_button-menu_icon svg { width: 20px; height: 20px; } .sd-split_button-leading { padding-left: 0.5rem; padding-right: 0.5rem; - } - - .sd-split_button-trailing { - padding-left: 0.25rem; - padding-right: 0.25rem; - } - - // Inner corner radius for XS - .sd-split_button-leading { border-top-right-radius: 4px; border-bottom-right-radius: 4px; } .sd-split_button-trailing { + padding-left: 0.25rem; + padding-right: 0.25rem; border-top-left-radius: 4px; border-bottom-left-radius: 4px; }src/components/split-button/SplitButton.stories.tsx (1)
30-36: Consider using a function forSampleMenuto avoid shared instance issues.
SampleMenuis defined as a single React element instance and reused across all stories. While this works for static content, if the Menu or MenuItem components maintain internal state, sharing the same instance could cause unexpected behavior in Storybook.-const SampleMenu = ( +const SampleMenu = () => ( <Menu> <MenuItem>Option 1</MenuItem> <MenuItem>Option 2</MenuItem> <MenuItem>Option 3</MenuItem> </Menu> ) // Update usage in stories: export const Filled: Story = { args: { variant: 'filled', size: 'm', children: 'Save', onAction: () => console.log('Action clicked'), - menu: SampleMenu, + menu: <SampleMenu />, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/index.ts(1 hunks)src/components/split-button/SplitButton.stories.tsx(1 hunks)src/components/split-button/SplitButton.tsx(1 hunks)src/components/split-button/index.ts(1 hunks)src/components/split-button/split-button.scss(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/split-button/SplitButton.tsx (3)
src/utils/type.ts (1)
ExtendProps(4-7)src/ripple/ripple-color.ts (1)
getReversedRippleColor(13-16)src/ripple/Ripple.tsx (1)
Ripple(30-57)
src/components/split-button/SplitButton.stories.tsx (3)
src/components/split-button/SplitButton.tsx (1)
SplitButton(11-171)src/components/menu/Menu.tsx (1)
Menu(11-25)src/components/menu/MenuItem.tsx (1)
MenuItem(18-63)
🔇 Additional comments (5)
src/components/split-button/SplitButton.tsx (1)
83-96: LGTM on controlled/uncontrolled state management.The pattern for handling both controlled (
selectedprop) and uncontrolled (internaluseState) states is implemented correctly. ThehandleMenuTogglefunction properly updates internal state only when uncontrolled, and always callsonMenuTogglewhen provided.src/components/index.ts (1)
23-23: LGTM!The export follows the established alphabetical ordering and consistent pattern with other component exports.
src/components/split-button/index.ts (1)
1-1: LGTM!Standard barrel export pattern consistent with other component modules.
src/components/split-button/SplitButton.stories.tsx (1)
89-98: Good accessibility practice witharia-labelfor icon-only variant.The
IconOnlystory correctly includes anaria-labelprop to ensure screen reader users understand the button's purpose when there's no visible text label.Consider adding additional stories to demonstrate:
- Controlled
selectedstate withonMenuToggle- Custom
menuIconpropsrc/components/split-button/split-button.scss (1)
311-347: The elevation and outline mixins are properly available. The@mixin elevation-level1,@mixin elevation-level2, and@mixin outlineare imported centrally insrc/style/index.scssvia@import '@/style/elevation'and@import '@/style/tokens/baseline', making them available globally to all component SCSS files including split-button.scss through Vite's build system.
| &:focus-visible { | ||
| outline: none; | ||
| opacity: unset; | ||
| filter: brightness(90%); | ||
| } |
There was a problem hiding this comment.
Ensure focus indicators meet accessibility requirements.
Setting outline: none on :focus-visible removes the default focus ring. While filter: brightness() provides some visual feedback, it may not provide sufficient contrast for all users, especially those relying on keyboard navigation.
Consider adding a more visible focus indicator that meets WCAG 2.1 guidelines (3:1 contrast ratio for focus indicators).
&:focus-visible {
- outline: none;
- opacity: unset;
- filter: brightness(90%);
+ outline: 2px solid var(--md-sys-color-primary);
+ outline-offset: 2px;
+ opacity: unset;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &:focus-visible { | |
| outline: none; | |
| opacity: unset; | |
| filter: brightness(90%); | |
| } | |
| &:focus-visible { | |
| outline: 2px solid var(--md-sys-color-primary); | |
| outline-offset: 2px; | |
| opacity: unset; | |
| } |
🤖 Prompt for AI Agents
In src/components/split-button/split-button.scss around lines 341–345, the
:focus-visible rule removes the default outline which harms keyboard
accessibility; replace the outline:none with an explicit high‑contrast focus
indicator (e.g., set a 2px solid outline or a visible box-shadow with an
outline-offset) that meets WCAG 2.1 3:1 contrast ratio, keep or combine the
existing filter: brightness(90%) if desired, and ensure the chosen
color/box-shadow has sufficient contrast against the background and works with
the component’s border-radius so keyboard focus is always clearly visible.
| <Ripple | ||
| as="button" | ||
| type="button" | ||
| className="sd-split_button-leading" | ||
| rippleColor={rippleColor} | ||
| onClick={() => !disabled && onAction?.()} | ||
| onKeyDown={(e) => | ||
| !disabled && | ||
| (e.key === 'Enter' || e.key === ' ') && | ||
| onAction?.() | ||
| } | ||
| data-sd-disabled={disabled} | ||
| aria-disabled={disabled} | ||
| aria-label={ariaLabel} | ||
| > | ||
| {leadingIcon && ( | ||
| <span className="sd-split_button-icon">{leadingIcon}</span> | ||
| )} | ||
| {children && ( | ||
| <span className="sd-split_button-label">{children}</span> | ||
| )} | ||
| </Ripple> |
There was a problem hiding this comment.
Use native disabled attribute instead of aria-disabled for true disabling.
Using aria-disabled and manual !disabled checks allows the button to still be focusable and technically clickable. For proper disabling, consider using the native disabled attribute which prevents interaction entirely.
<Ripple
as="button"
type="button"
className="sd-split_button-leading"
rippleColor={rippleColor}
- onClick={() => !disabled && onAction?.()}
- onKeyDown={(e) =>
- !disabled &&
- (e.key === 'Enter' || e.key === ' ') &&
- onAction?.()
- }
+ onClick={onAction}
+ disabled={disabled}
data-sd-disabled={disabled}
- aria-disabled={disabled}
aria-label={ariaLabel}
>If you need to keep focus on disabled buttons for accessibility reasons (e.g., tooltip on disabled), then the current approach is valid but ensure the onKeyDown handler also calls e.preventDefault() for Space to prevent double-firing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Ripple | |
| as="button" | |
| type="button" | |
| className="sd-split_button-leading" | |
| rippleColor={rippleColor} | |
| onClick={() => !disabled && onAction?.()} | |
| onKeyDown={(e) => | |
| !disabled && | |
| (e.key === 'Enter' || e.key === ' ') && | |
| onAction?.() | |
| } | |
| data-sd-disabled={disabled} | |
| aria-disabled={disabled} | |
| aria-label={ariaLabel} | |
| > | |
| {leadingIcon && ( | |
| <span className="sd-split_button-icon">{leadingIcon}</span> | |
| )} | |
| {children && ( | |
| <span className="sd-split_button-label">{children}</span> | |
| )} | |
| </Ripple> | |
| <Ripple | |
| as="button" | |
| type="button" | |
| className="sd-split_button-leading" | |
| rippleColor={rippleColor} | |
| onClick={onAction} | |
| disabled={disabled} | |
| data-sd-disabled={disabled} | |
| aria-label={ariaLabel} | |
| > | |
| {leadingIcon && ( | |
| <span className="sd-split_button-icon">{leadingIcon}</span> | |
| )} | |
| {children && ( | |
| <span className="sd-split_button-label">{children}</span> | |
| )} | |
| </Ripple> |
🤖 Prompt for AI Agents
In src/components/split-button/SplitButton.tsx around lines 110 to 131, the
Ripple button uses aria-disabled and manual checks which leaves it
focusable/clickable; replace that with the native disabled attribute on the
underlying button when disabled is true, remove the manual !disabled guards from
onClick and onKeyDown so the browser handles blocking interactions, and keep
aria-disabled only if you must preserve focus for accessibility; if you
deliberately retain aria-disabled instead of native disabled, ensure the
onKeyDown handler calls e.preventDefault() when handling Space to avoid
double-firing and keep click handlers guarded.
| {menu && selected && ( | ||
| <div className="sd-split_button-menu">{menu}</div> | ||
| )} |
There was a problem hiding this comment.
Add click-outside and Escape key handling to close the menu.
The menu opens when the trailing button is clicked but there's no mechanism to close it when clicking outside or pressing Escape. This is expected behavior for dropdown menus.
Consider adding:
- A click-outside listener to close the menu
- An Escape key handler to close the menu
- Optionally, focus management to return focus to the trailing button when closing
+import { forwardRef, useState, useEffect, useRef } from 'react'
// Inside the component:
+ const containerRef = useRef<HTMLDivElement>(null)
+
+ useEffect(() => {
+ if (!selected) return
+
+ const handleClickOutside = (e: MouseEvent) => {
+ if (containerRef.current && !containerRef.current.contains(e.target as Node)) {
+ if (controlledSelected === undefined) {
+ setInternalSelected(false)
+ }
+ onMenuToggle?.(false)
+ }
+ }
+
+ const handleEscape = (e: KeyboardEvent) => {
+ if (e.key === 'Escape') {
+ if (controlledSelected === undefined) {
+ setInternalSelected(false)
+ }
+ onMenuToggle?.(false)
+ }
+ }
+
+ document.addEventListener('mousedown', handleClickOutside)
+ document.addEventListener('keydown', handleEscape)
+ return () => {
+ document.removeEventListener('mousedown', handleClickOutside)
+ document.removeEventListener('keydown', handleEscape)
+ }
+ }, [selected, controlledSelected, onMenuToggle])Committable suggestion skipped: line range outside the PR's diff.
Implements Material Design 3 Split Button component with leading action button and trailing menu button separated by a divider.
Implementation
Component structure
Variants & sizes
elevated,filled,tonal,outlinedxs(24px),s(32px),m(40px),l(48px),xl(56px)M3 spec compliance
Accessibility
aria-disabled, andaria-pressedattributesUsage
Screenshots
Filled variant with label:
With leading icon:
Storybook documentation:
Reference
M3 spec: https://m3.material.io/components/split-button/specs
Original prompt
This section details on the original issue you should resolve
<issue_title>Split buttons</issue_title>
<issue_description># Split buttons
Split buttons open a menu to give people more options related to an action
Specs: https://m3.material.io/components/split-button/specs
Types
Configurations
Tokens & specs
Use the table's menu to select a token set. Split button token sets are organized by size. Learn about design tokens
Anatomy
The leading button in split buttons can have an icon, label text, or both. The trailing button should always have a menu icon.
Color
Color values are implemented through design tokens. Design tokens are the building blocks of all UI elements. The same tokens are used in designs, tools, and code. More on tokens.
Split buttons use the same color schemes as standard buttons. More on buttons. However, unlike toggle buttons, the split button color doesn’t change when selected—only a state layer is applied.
Split buttons use the same colors and state layers as buttons, shown in the following token module. Go to buttons for more details.
A: Unselected, B: Selected trailing icon
States
States are visual representations used to communicate the status of a component or an interactive element. More on states
Split button states use the same colors and state layers as buttons and icon buttons. Go to those specs for details.
Leading button shape
The inner corners change shape for hovered, focused, and pressed states.
Trailing button shape
The inner corners change shape for hovered, focused, and pressed states, and the icon becomes centered when selected.
Measurements
Text and icons are optically centered when the buttons are asymmetrical. They’re centered normally when symmetrical.
Menu icon offset when unselected:
The inner corner radius changes depending on button sizing. The space should always be 2dp.
![Inner padding and inner corner measurements of the...
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.