Apply BitButtonGroup improvements (#12699) - #12703
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (25)
WalkthroughBitButtonGroup now supports explicit selection modes, synchronized multi-selection, keyboard navigation, ARIA patterns, loading and link items, selection indicators, layout variants, expanded styling hooks, comprehensive demos, and focused behavioral tests. ChangesBitButtonGroup improvements
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BitButtonGroup
participant Item
participant Callback
User->>BitButtonGroup: Click or navigate with keyboard
BitButtonGroup->>Item: Resolve loading, toggle, and focus state
BitButtonGroup->>Item: Render button or anchor with ARIA attributes
BitButtonGroup->>Callback: Notify toggle or click changes
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.samples.cs (1)
165-207: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
example6CsharpCodeis missing the definitions used by the new FixedToggle sample.The FixedToggle line added at 165 references
NameSelectors="multiNameSelectors"andItems="fixedSingleCustoms", butexample6CsharpCode(166–207) defines neither (multiNameSelectorsonly exists inexample14CsharpCode). The C# panel shown with this example won't compile if copied. The Item demo'sexample6CsharpCodeaddedfixedSingleItemsfor the analogous case; mirror that here (definefixedSingleCustoms, and either reusetoggledNameSelectorsin the razor or addmultiNameSelectors).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.samples.cs` around lines 165 - 207, Update example6CsharpCode to define the symbols referenced by the FixedToggle sample: add fixedSingleCustoms with the required Operation items and either change the sample to reuse toggledNameSelectors or define multiNameSelectors locally. Keep the copied C# example self-contained and compilable.
🧹 Nitpick comments (2)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs (2)
135-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrdering assertion doesn't actually prove item-order semantics.
The comment says the keys follow the order of the items claim being tested here, but the click order (
buttons[0]thenbuttons[2], i.e. "a" then "c") matches the item order exactly, so this test can't distinguish "ordered by item position" from "ordered by click/toggle sequence." Click out of item order (e.g.buttons[2]thenbuttons[0]) and still assert["a", "c"]to actually validate the claim.♻️ Strengthen the ordering assertion
- comp.FindAll("button")[0].Click(); - comp.FindAll("button")[2].Click(); + // Click out of item order to actually prove ordering follows item position, not click sequence. + comp.FindAll("button")[2].Click(); + comp.FindAll("button")[0].Click(); // Both stay toggled, and the keys follow the order of the items. CollectionAssert.AreEqual(new[] { "a", "c" }, toggleKeys?.ToArray());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs` around lines 135 - 171, Update BitButtonGroupMultipleSelectionShouldUseTheToolbarPatternAndToggleIndependently to click the third button before the first, while preserving the assertion that toggleKeys is ordered as ["a", "c"]. Keep the existing independent-toggle and removal assertions unchanged.
222-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeyboard navigation itself is never exercised.
This test only checks the initial
tabindexplacement (roving vs. per-button), which shows the group is configured for roving-tabindex, but no test in this file actually dispatches arrow/Home/End key events to verify focus moves between items. Given this layer's stated purpose explicitly includes "navigation," and bUnit shipsIElement.KeyDown(Key, ...)/KeyDown(string key, ...)extension methods for this, adding a keydown-driven test would meaningfully close the gap. Also noteSelectOnFocus(which toggles the focused item while navigating) has no coverage anywhere in this diff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs` around lines 222 - 256, Extend BitButtonGroupNavigableShouldMakeTheGroupASingleTabStop to dispatch keyboard events using bUnit keydown helpers and assert focus moves between buttons for arrow, Home, and End navigation. Also cover SelectOnFocus by enabling it and verifying that navigating to an item toggles the focused item, while preserving the existing roving-tabindex assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs`:
- Around line 546-611: Update HandleOnKeyDown to prevent the browser’s default
action only when handling ArrowUp, ArrowDown, Home, or End, while preserving
existing navigation behavior and allowing Space/Enter defaults. Apply the
prevention through the component’s keydown event handling rather than a blanket
`@onkeydown`:preventDefault.
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.scss`:
- Around line 194-199: Add an empty line before the badge tinting comment in the
button-group SCSS, and change every referenced `currentColor` value to lowercase
`currentcolor` in the badge styles and `.bit-btg-sin::after` `border-color`
declaration.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor`:
- Around line 655-665: Update the Links demo copy within the `DemoExample` for
`example21` to describe only the currently rendered `linkCustoms` examples, or
extend `linkCustoms` to visibly include mixed button/link and disabled or
loading-link cases. Keep the documentation aligned with what the demo actually
renders.
---
Outside diff comments:
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.samples.cs`:
- Around line 165-207: Update example6CsharpCode to define the symbols
referenced by the FixedToggle sample: add fixedSingleCustoms with the required
Operation items and either change the sample to reuse toggledNameSelectors or
define multiNameSelectors locally. Keep the copied C# example self-contained and
compilable.
---
Nitpick comments:
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs`:
- Around line 135-171: Update
BitButtonGroupMultipleSelectionShouldUseTheToolbarPatternAndToggleIndependently
to click the third button before the first, while preserving the assertion that
toggleKeys is ordered as ["a", "c"]. Keep the existing independent-toggle and
removal assertions unchanged.
- Around line 222-256: Extend
BitButtonGroupNavigableShouldMakeTheGroupASingleTabStop to dispatch keyboard
events using bUnit keydown helpers and assert focus moves between buttons for
arrow, Home, and End navigation. Also cover SelectOnFocus by enabling it and
verifying that navigating to an item toggles the focused item, while preserving
the existing roving-tabindex assertions.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0a0105f8-7fbf-4dfc-9e35-4924e02e059a
📒 Files selected for processing (25)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.scsssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupClassStyles.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupItem.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupNameSelectors.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOverflow.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupSelectionMode.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/BitButtonGroupDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/BitButtonGroupDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/BitButtonGroupDemo.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/Operation.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.samples.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupItemDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupItemDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupItemDemo.razor.samples.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupOptionDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupOptionDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupOptionDemo.razor.samples.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor`:
- Around line 30-45: The anchor branch in _BitButtonGroupItem must expose the
same toggle semantics as the button branch for selectable items. Mirror the
button branch’s role and appropriate aria-checked/aria-pressed attributes on the
<a>, using the existing item state helpers and preserving non-toggle behavior
for non-selectable groups.
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.scss`:
- Around line 43-45: Insert an empty line between the overflow-y declaration and
the explanatory comment in the BitButtonGroup styles, preserving the existing
overscroll-behavior-x declaration and comment text.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca1997d4-74cd-4d47-a6ba-a4cef75fd216
📒 Files selected for processing (25)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.scsssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupClassStyles.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupItem.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupNameSelectors.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOverflow.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupSelectionMode.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/BitButtonGroupDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/BitButtonGroupDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/BitButtonGroupDemo.razor.scsssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/Operation.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupCustomDemo.razor.samples.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupItemDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupItemDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupItemDemo.razor.samples.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupOptionDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupOptionDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Buttons/ButtonGroup/_BitButtonGroupOptionDemo.razor.samples.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 25 minutes. |
1 participant
closes #12699
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests