feat(button-group): add mobile variant #448#606
Merged
Conversation
… feat/448-button-group-missing-mobile-button
21 tasks
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds mobile dropdown support to ButtonGroup with new props and responsive rendering via Changes
Sequence DiagramsequenceDiagram
participant User
participant ButtonGroup as ButtonGroup Component
participant Breakpoint as useBreakpoint Hook
participant Dropdown as Dropdown Component
participant Button as Child Button Elements
User->>ButtonGroup: Mount/render (props include enableMobileDropdown)
ButtonGroup->>Breakpoint: Query viewport / isBreakpointBelow(mobileBreakpoint)
Breakpoint-->>ButtonGroup: Return breakpoint info
alt Mobile view & enableMobileDropdown
ButtonGroup->>Dropdown: Render Dropdown with trigger (label from active/static)
User->>Dropdown: Click trigger
Dropdown->>Dropdown: Toggle open state
User->>Dropdown: Click dropdown item (Button X)
Dropdown->>Button: Invoke child onClick(event)
Button-->>Dropdown: onClick handled
Dropdown->>ButtonGroup: Call onSelectionChange(Button X id)
ButtonGroup-->>User: Selection updated
else Desktop / not mobile
ButtonGroup->>Button: Render inline Buttons
User->>Button: Click Button X
Button->>ButtonGroup: Invoke child onClick(event)
Button->>ButtonGroup: Call onSelectionChange(Button X id)
ButtonGroup-->>User: Selection updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/buttons/button-group/button-group.tsx (1)
73-90: 🛠️ Refactor suggestion | 🟠 MajorResolve responsive props through
useBreakpointProps.This new responsive rendering path only reads raw props plus
useBreakpoint(), so consumers cannot pass breakpoint-specific prop overrides consistently with other TEDI components.As per coding guidelines, Responsive props are supported via
useBreakpointPropshook — components must accept per-breakpoint prop overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/buttons/button-group/button-group.tsx` around lines 73 - 90, The ButtonGroup currently ignores per-breakpoint overrides because it reads raw props and calls useBreakpoint() directly; update ButtonGroup to resolve responsive props via the useBreakpointProps hook by passing the original props into useBreakpointProps (instead of using useBreakpoint() + raw destructured values), then derive type, stretch, size, enableMobileDropdown, mobileBreakpoint, dropdownLabel, and dropdownLabelMode from the returned responsiveProps; keep the existing fallback defaults (e.g., type = 'primary', stretch = false, size = 'default', dropdownLabelMode = 'static') if useBreakpointProps doesn't provide them, and continue using the isBreakpointBelow check with the resolved mobileBreakpoint and breakpoint from useBreakpoint() as before so per-breakpoint overrides are honored.
🧹 Nitpick comments (1)
src/tedi/components/buttons/button-group/button-group.spec.tsx (1)
80-82: Query dropdown items by role.These assertions should target the actual interactive menu item instead of only matching text content.
🧪 Proposed test cleanup
- const dropdownItem = screen.getByText('Button 1'); + const dropdownItem = screen.getByRole('menuitem', { name: 'Button 1' }); fireEvent.click(dropdownItem);- const dropdownItem = screen.getByText('Button 1'); + const dropdownItem = screen.getByRole('menuitem', { name: 'Button 1' }); fireEvent.click(dropdownItem);As per coding guidelines, Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId).Also applies to: 101-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/buttons/button-group/button-group.spec.tsx` around lines 80 - 82, Replace the text-based queries for dropdown items with semantic role queries: locate the test block in button-group.spec.tsx where dropdownItem is fetched (currently using screen.getByText('Button 1')) and change it to query the interactive menu item by role (e.g., screen.getByRole('menuitem', { name: 'Button 1' }) or getAllByRole('menuitem') and pick the matching name), then fireEvent.click on that element; also update the similar occurrences referenced around the other assertions (the uses at the later lines mentioned) to use getByRole/getAllByRole with the appropriate accessible name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tedi/components/buttons/button-group/button-group.module.scss`:
- Around line 144-152: The collapsed-state border token in the &--secondary rule
is using the selected token (var(--button-group-secondary-selected-border));
update that to use the secondary default token from the button-main-secondary
family (e.g., var(--button-main-secondary-border-default)) in the &--secondary
rule so the non-expanded trigger doesn't appear selected, keeping the existing
&[aria-expanded='true'] background and border
(var(--button-main-secondary-border-active)) unchanged.
In `@src/tedi/components/buttons/button-group/button-group.tsx`:
- Around line 99-122: The current logic forces activeIconLeft to default to
'menu', which prevents the activeIcon branch from ever rendering and dropdown
items ignore icon-only buttons; change the default for activeIconLeft (the const
activeIconLeft = ... line) to undefined/null so it doesn't shadow
activeButton.props.icon, then update the Dropdown.Trigger and the Dropdown item
render path (where you map buttons into Dropdown.Items) to check
activeButton.props.icon and btn.props.icon respectively—use typeof checks like
you already do for activeIcon to resolve name vs object—and render Icon when
either iconLeft or icon is present so icon-only buttons keep their icons in
mobile dropdown mode.
- Around line 48-70: The JSDoc defaults are out of sync with runtime: update the
comments for enableMobileDropdown to "@default false", change dropdownLabel's
description to state its default comes from getLabel('sidenav.submenu') (and
that ariaLabel is only used if that computed label is unavailable), and ensure
dropdownLabelMode's `@default` is set to 'static' (and remove conflicting text).
Apply the same corrections for the duplicate prop block later in the file (same
props: enableMobileDropdown, dropdownLabel, dropdownLabelMode).
- Around line 177-183: The cloning currently overwrites each child's className;
update the cloneElement call (used with buttonsArray and cloneElement) to merge
the child's existing className with the group classes instead of replacing
it—use cn(child.props.className, styles['tedi-button-group__item'], {
[styles['tedi-button-group__item--active']]: child.props.isActive,
[styles['tedi-button-group__item--disabled']]: child.props.disabled,
[styles[`tedi-button-group__item--size-${size}`]]: size }) so consumer-provided
per-button classes are preserved when cloning.
---
Outside diff comments:
In `@src/tedi/components/buttons/button-group/button-group.tsx`:
- Around line 73-90: The ButtonGroup currently ignores per-breakpoint overrides
because it reads raw props and calls useBreakpoint() directly; update
ButtonGroup to resolve responsive props via the useBreakpointProps hook by
passing the original props into useBreakpointProps (instead of using
useBreakpoint() + raw destructured values), then derive type, stretch, size,
enableMobileDropdown, mobileBreakpoint, dropdownLabel, and dropdownLabelMode
from the returned responsiveProps; keep the existing fallback defaults (e.g.,
type = 'primary', stretch = false, size = 'default', dropdownLabelMode =
'static') if useBreakpointProps doesn't provide them, and continue using the
isBreakpointBelow check with the resolved mobileBreakpoint and breakpoint from
useBreakpoint() as before so per-breakpoint overrides are honored.
---
Nitpick comments:
In `@src/tedi/components/buttons/button-group/button-group.spec.tsx`:
- Around line 80-82: Replace the text-based queries for dropdown items with
semantic role queries: locate the test block in button-group.spec.tsx where
dropdownItem is fetched (currently using screen.getByText('Button 1')) and
change it to query the interactive menu item by role (e.g.,
screen.getByRole('menuitem', { name: 'Button 1' }) or getAllByRole('menuitem')
and pick the matching name), then fireEvent.click on that element; also update
the similar occurrences referenced around the other assertions (the uses at the
later lines mentioned) to use getByRole/getAllByRole with the appropriate
accessible name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23bb20a4-0a48-4bae-a9eb-2f3fdb5046fc
📒 Files selected for processing (5)
src/tedi/components/buttons/button-group/button-group.module.scsssrc/tedi/components/buttons/button-group/button-group.spec.tsxsrc/tedi/components/buttons/button-group/button-group.stories.tsxsrc/tedi/components/buttons/button-group/button-group.tsxsrc/tedi/components/overlays/dropdown/dropdown-item/dropdown-item.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tedi/components/buttons/button-group/button-group.tsx (1)
48-90:⚠️ Potential issue | 🟡 MinorWire the new mobile props through
useBreakpointProps.The new responsive behavior is resolved with
useBreakpoint()and scalar props only, so consumers cannot pass breakpoint-specific overrides forenableMobileDropdown/mobileBreakpointlike other TEDI components. Please model these as breakpoint-aware props and resolve them viauseBreakpointPropsbefore computingisMobileView.As per coding guidelines, Responsive props are supported via
useBreakpointPropshook — components must accept per-breakpoint prop overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/buttons/button-group/button-group.tsx` around lines 48 - 90, ButtonGroup currently reads enableMobileDropdown and mobileBreakpoint directly from props and computes isMobileView with useBreakpoint, so breakpoint-specific overrides are ignored; call useBreakpointProps to resolve those responsive props before computing isMobileView and use the resolved values in place of the raw props. Specifically, import and call useBreakpointProps with the incoming props and the keys ['enableMobileDropdown','mobileBreakpoint'] (e.g. const bpResolved = useBreakpointProps(props, ['enableMobileDropdown','mobileBreakpoint'])), then derive const resolvedEnableMobileDropdown = bpResolved.enableMobileDropdown ?? enableMobileDropdown and const resolvedMobileBreakpoint = bpResolved.mobileBreakpoint ?? mobileBreakpoint (or use bpResolved values directly), and pass resolvedMobileBreakpoint into isBreakpointBelow(breakpoint, resolvedMobileBreakpoint) and resolvedEnableMobileDropdown into any mobile-conditional logic in ButtonGroup (keep dropdownLabel and dropdownLabelMode unchanged).
♻️ Duplicate comments (1)
src/tedi/components/buttons/button-group/button-group.tsx (1)
99-122:⚠️ Potential issue | 🟡 MinorPreserve
iconbuttons in mobile dropdown mode.
activeIconLeftalways falls back to'menu', so theactiveIconbranch is unreachable; dropdown items also ignorebtn.props.icon. Icon-only buttons therefore show the fallback menu icon in the trigger and lose their own icon in the item list.🐛 Proposed fix
- const activeIconLeft = activeButton?.props.iconLeft || 'menu'; - const activeIcon = activeButton?.props.icon; + const triggerIcon = activeButton?.props.iconLeft ?? activeButton?.props.icon ?? 'menu'; @@ - {activeIconLeft && ( - <Icon name={typeof activeIconLeft === 'string' ? activeIconLeft : activeIconLeft.name} color="inherit" /> - )} - - {!activeIconLeft && activeIcon && ( - <Icon name={typeof activeIcon === 'string' ? activeIcon : activeIcon.name} color="inherit" /> + {triggerIcon && ( + <Icon name={typeof triggerIcon === 'string' ? triggerIcon : triggerIcon.name} color="inherit" /> )} @@ - {btn.props.iconLeft && ( + {(btn.props.iconLeft ?? btn.props.icon) && ( <Icon - name={typeof btn.props.iconLeft === 'string' ? btn.props.iconLeft : btn.props.iconLeft.name} + name={ + typeof (btn.props.iconLeft ?? btn.props.icon) === 'string' + ? (btn.props.iconLeft ?? btn.props.icon) + : (btn.props.iconLeft ?? btn.props.icon)?.name + } color="inherit" /> )}Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/buttons/button-group/button-group.tsx` around lines 99 - 122, activeIconLeft is incorrectly defaulting to 'menu' which makes the activeIcon branch unreachable and causes icon-only buttons to show the menu icon; remove the hard fallback and instead compute icons from activeButton.props without a default (use const activeIconLeft = activeButton?.props.iconLeft; const activeIcon = activeButton?.props.icon), then in Dropdown.Trigger render Icon using activeIconLeft if defined else activeIcon (checking typeof for both), and update the Dropdown item rendering logic to prefer btn.props.icon (falling back to btn.props.iconLeft only if icon is absent) so item list preserves each button's own icon; apply the same change pattern to the similar code around the 146-151 region and keep checks defensive for null/undefined when enableMobileDropdown is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tedi/components/buttons/button-group/button-group.tsx`:
- Around line 135-139: The onClick wrapper is silencing a real type mismatch by
casting to UnknownType; update the handler for Dropdown.Item -> Button
interaction so it properly accepts and normalizes React.MouseEvent |
React.KeyboardEvent instead of any. Locate the inline onClick that calls
btn.props.onClick (and the UnknownType cast) and replace it with a small typed
wrapper that narrows the union (e.g., type-guard on event.type or check for
'key'/'keyCode' vs mouse properties), extract the needed properties, and then
call btn.props.onClick with a correctly-typed
React.MouseEvent<HTMLButtonElement> or change btn.props.onClick to accept the
union type—ensure the change touches the onClick wrapper and btn.props.onClick
signature (or add a normalizeEvent helper) so KeyboardEvent activation is
handled safely without using UnknownType.
---
Outside diff comments:
In `@src/tedi/components/buttons/button-group/button-group.tsx`:
- Around line 48-90: ButtonGroup currently reads enableMobileDropdown and
mobileBreakpoint directly from props and computes isMobileView with
useBreakpoint, so breakpoint-specific overrides are ignored; call
useBreakpointProps to resolve those responsive props before computing
isMobileView and use the resolved values in place of the raw props.
Specifically, import and call useBreakpointProps with the incoming props and the
keys ['enableMobileDropdown','mobileBreakpoint'] (e.g. const bpResolved =
useBreakpointProps(props, ['enableMobileDropdown','mobileBreakpoint'])), then
derive const resolvedEnableMobileDropdown = bpResolved.enableMobileDropdown ??
enableMobileDropdown and const resolvedMobileBreakpoint =
bpResolved.mobileBreakpoint ?? mobileBreakpoint (or use bpResolved values
directly), and pass resolvedMobileBreakpoint into isBreakpointBelow(breakpoint,
resolvedMobileBreakpoint) and resolvedEnableMobileDropdown into any
mobile-conditional logic in ButtonGroup (keep dropdownLabel and
dropdownLabelMode unchanged).
---
Duplicate comments:
In `@src/tedi/components/buttons/button-group/button-group.tsx`:
- Around line 99-122: activeIconLeft is incorrectly defaulting to 'menu' which
makes the activeIcon branch unreachable and causes icon-only buttons to show the
menu icon; remove the hard fallback and instead compute icons from
activeButton.props without a default (use const activeIconLeft =
activeButton?.props.iconLeft; const activeIcon = activeButton?.props.icon), then
in Dropdown.Trigger render Icon using activeIconLeft if defined else activeIcon
(checking typeof for both), and update the Dropdown item rendering logic to
prefer btn.props.icon (falling back to btn.props.iconLeft only if icon is
absent) so item list preserves each button's own icon; apply the same change
pattern to the similar code around the 146-151 region and keep checks defensive
for null/undefined when enableMobileDropdown is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b9b367b-fab5-4c1b-b1c9-d81927f15aeb
📒 Files selected for processing (2)
src/tedi/components/buttons/button-group/button-group.module.scsssrc/tedi/components/buttons/button-group/button-group.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tedi/components/buttons/button-group/button-group.module.scss
Contributor
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
ly-tempel-bitweb
approved these changes
Apr 23, 2026
mart-sessman
approved these changes
Apr 24, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Apr 27, 2026
# [17.0.0-rc.8](react-17.0.0-rc.7...react-17.0.0-rc.8) (2026-04-27) ### Features * **button-group:** add mobile variant [#448](#448) ([#606](#606)) ([54dee90](54dee90)), closes [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94)
github-actions Bot
pushed a commit
that referenced
this pull request
Apr 29, 2026
# [17.0.0](react-16.1.0...react-17.0.0) (2026-04-29) ### Bug Fixes * **checkbox:** invalid indicator hover border fix [#605](#605) ([#609](#609)) ([f1d62c6](f1d62c6)) * **select:** select placeholder no longer blocks context menu interactions [#584](#584) ([#585](#585)) ([e8d86ab](e8d86ab)) * **variables:** update core version, update variable names [#592](#592) ([#598](#598)) ([1f15b36](1f15b36)) ### Features * **button-group:** add mobile variant [#448](#448) ([#606](#606)) ([54dee90](54dee90)), closes [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) [#94](#94) * **card:** add more control to borderRadius usage, add examples [#444](#444) ([#597](#597)) ([deac9db](deac9db)) * **print:** introduce PrintingProvider + context-based usePrint [#99](#99) ([#497](#497)) ([a58cb70](a58cb70)) * **spinner:** add new sizes [#586](#586) ([#589](#589)) ([fbea0c3](fbea0c3)) * **tabs:** new tedi-ready component [#555](#555) ([#557](#557)) ([9c06c51](9c06c51)) * **textarea:** add autoGrow, height and maxHeight props [#588](#588) ([#593](#593)) ([2c86740](2c86740)) * **toggle:** new TEDI-Ready component [#305](#305) ([#594](#594)) ([6f28045](6f28045)) ### BREAKING CHANGES * **print:** usePrint hook removed. Replace with usePrint from the new PrintingProvider context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style
Documentation
Tests
Refactor