feat(CheckboxGroup/RadioGroup): add icon field in items - #6726
feat(CheckboxGroup/RadioGroup): add icon field in items#6726solidusite wants to merge 17 commits into
icon field in items#6726Conversation
- Introduced `leadingIcon` prop in Checkbox and RadioGroup components to display an icon next to the label. - Updated CheckboxGroup and RadioGroup to handle the new `icon` property in items. - Enhanced styles in themes to accommodate leading icons. - Added tests to verify rendering of leading icons in CheckboxGroup and RadioGroup.
- Updated documentation to reflect the new `icon` property and its usage in examples. - Modified playground examples to demonstrate the new icon functionality in Checkbox and RadioGroup components.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughCheckbox and radio components now support leading icons. Checkbox and radio group items accept optional icon values, which are passed to or rendered by the underlying controls. Theme slots and size variants define icon styling. Documentation, playgrounds, and render matrices demonstrate the new APIs, including icon-based card grids. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds optional per-item icons while preserving existing rendering when icons are omitted; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
src/runtime/components/RadioGroup.vue (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider typing
iconasIconProps['name']for consistency with Checkbox.Checkbox.vue types
leadingIconasIconProps['name'](string | any), allowing both Iconify string names and component references. RadioGroupItem'sicon?: stringis more restrictive, preventing component-based icons thatUIconotherwise supports via<component :is="name" />. Aligning the type would ensure cross-component consistency and unlock the fullUIconcontract.Based on learnings,
data-slotattributes should only be added to elements with corresponding theme-slot class bindings — theleadingIconslot at line 222 satisfies this.♻️ Proposed type alignment
export type RadioGroupItem = RadioGroupValue | { label?: string description?: string disabled?: boolean value?: RadioGroupValue /** * The icon displayed next to the label. * `@IconifyIcon` */ - icon?: string + icon?: IconProps['name'] class?: any ui?: Pick<RadioGroup['slots'], 'item' | 'container' | 'base' | 'indicator' | 'wrapper' | 'label' | 'leadingIcon' | 'description'> [key: string]: any }This would also require adding
import type { IconProps } from './Icon.vue'to the type imports.🤖 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/runtime/components/RadioGroup.vue` around lines 18 - 24, Update the RadioGroupItem `icon` prop type to use `IconProps['name']` instead of `string`, adding the corresponding type-only import from `Icon.vue` and preserving the existing UI contract.Source: Learnings
🤖 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 `@test/components/CheckboxGroupIcon.a4cec7.spec.ts`:
- Around line 8-12: In test/components/CheckboxGroupIcon.a4cec7.spec.ts lines
8-12, define a non-optional firstItem from items[0] and replace all listed
items[0] usages with firstItem. Apply the same change in
test/components/RadioGroupIcon.781254.spec.ts lines 7-11, replacing all listed
indexed usages so mountSuspended receives a CheckboxGroupItem or RadioGroupItem
rather than an undefined-capable value.
---
Nitpick comments:
In `@src/runtime/components/RadioGroup.vue`:
- Around line 18-24: Update the RadioGroupItem `icon` prop type to use
`IconProps['name']` instead of `string`, adding the corresponding type-only
import from `Icon.vue` and preserving the existing UI contract.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de8b0df1-0fd7-4ad1-82a6-b4a22664615e
⛔ Files ignored due to path filters (4)
test/components/__snapshots__/Checkbox-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/CheckboxGroup-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/RadioGroup-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Table-vue.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
docs/content/docs/2.components/checkbox-group.mddocs/content/docs/2.components/checkbox.mddocs/content/docs/2.components/radio-group.mdplaygrounds/nuxt/app/pages/components/checkbox-group.vueplaygrounds/nuxt/app/pages/components/checkbox.vueplaygrounds/nuxt/app/pages/components/radio-group.vuesrc/runtime/components/Checkbox.vuesrc/runtime/components/CheckboxGroup.vuesrc/runtime/components/RadioGroup.vuesrc/theme/checkbox.tssrc/theme/radio-group.tstest/components/CheckboxGroupIcon.a4cec7.spec.tstest/components/RadioGroupIcon.781254.spec.ts
- Deleted test files for CheckboxGroup and RadioGroup components that were previously verifying leading icon functionality. - This cleanup is part of a broader effort to streamline test coverage and focus on essential components.
- Changed the type of the `icon` property in CheckboxGroup and RadioGroup components from `string` to `IconProps['name']` for better type safety. - Updated tests to include scenarios for rendering items with icons in both CheckboxGroup and RadioGroup components. - Added snapshot tests to verify correct rendering of icons in the respective components.
… and Table components - Adjusted snapshots to reflect changes in label rendering, ensuring labels now include conditional rendering comments. - Updated multiple test files to maintain consistency across Checkbox, CheckboxGroup, RadioGroup, and Table components.
commit: |
The per-item `icon` was spread onto the underlying `UCheckbox`, so a checked item rendered the item icon in place of its check indicator. `useForwardProps` strips `undefined`, so the group-level `icon` did not shadow it. Omit `icon` from the forwarded props and bind it only to `leading-icon`. - Type item `icon` as `IconProps['name']` - Add a regression test asserting an item `icon` maps to `leadingIcon` and leaves the checkbox `icon` untouched
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/components/CheckboxGroup.spec.ts (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
VueWrapperas a type to satisfy@typescript-eslint/consistent-type-imports.
VueWrapperis only used in a type cast (as unknown as VueWrapper<any>), so ESLint flags it as a value import used only as a type.♻️ Proposed fix
-import { flushPromises, mount, VueWrapper } from '`@vue/test-utils`' +import { flushPromises, mount, type VueWrapper } from '`@vue/test-utils`'🤖 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 `@test/components/CheckboxGroup.spec.ts` around lines 3 - 5, Update the imports in CheckboxGroup.spec.ts so VueWrapper is imported as a type, while keeping flushPromises, mount, and Checkbox as regular imports.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@test/components/CheckboxGroup.spec.ts`:
- Around line 3-5: Update the imports in CheckboxGroup.spec.ts so VueWrapper is
imported as a type, while keeping flushPromises, mount, and Checkbox as regular
imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06f4f3f2-9adc-4ba0-9f74-ea929976c460
⛔ Files ignored due to path filters (4)
test/components/__snapshots__/Checkbox.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/CheckboxGroup.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/RadioGroup.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Table.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/runtime/components/CheckboxGroup.vuetest/components/CheckboxGroup.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/components/CheckboxGroup.vue
benjamincanac
changed the title
icon field in items
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…bel when `indicator` is hidden Replaces the `leadingIcon` prop with the existing `icon` one, which is dead weight when the indicator is hidden since `base` is `sr-only` there.
Adds hover, moves focus onto the card since that is the click target for these variants, and gives the checked state a background so it stays distinct from focus. This applies to every `card` and `table` usage, not only when `indicator` is hidden.
`UIcon` is `aria-hidden`, so an item with an icon and no label left the control with no accessible name. It now falls back to the value like reka-ui's `Radio` already does, so both components behave the same. Also shares the hover and focus theme helpers across the three theme files, which fixes a drift where the checkbox copy had lost `has-focus-visible:z-[1]`.
|
Thanks for the PR @solidusite! I've made a few changes to reuse the |
2 participants
🔗 Linked issue
Resolves #5990
❓ Type of change
📚 Description
Adds an optional
iconper item onUCheckboxGroupandURadioGroupso you can build visual pickers.There's no new prop. The item
iconflows intoUCheckbox's existingiconprop, so it keeps working as the check mark while the indicator is visible, and renders above the label whenindicator="hidden". A radio has no icon in its indicator, so thereicononly renders in the hidden mode.It also reworks the
cardandtablestates the picker leans on:highlightnow shows underindicator="hidden", where it previously styled thesr-onlycontrol and was invisibleThat part applies to every
cardandtablecheckbox and radio, not onlyindicator="hidden".📝 Checklist