feat(checkbox): adds checkbox component in ui with story and unit tests - #8861
feat(checkbox): adds checkbox component in ui with story and unit tests#8861sachin-thakur-bruno wants to merge 16 commits into
Conversation
|
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:
WalkthroughThe design system now defines four border hierarchy levels, including ChangesCheckbox and border token system
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Checkbox
participant StyledWrapper
participant Theme
User->>Checkbox: Interact with checkbox
Checkbox->>StyledWrapper: Render input, icon, and label
StyledWrapper->>Theme: Read themed border and state colors
Theme-->>StyledWrapper: Return color values
StyledWrapper-->>User: Display checkbox state
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
sachin-thakur-bruno
changed the title
sachin-thakur-bruno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/bruno-app/src/themes/schema/oss.js (1)
153-158: 🗄️ Data Integrity & Integration | 🔵 TrivialNo change needed for builtin theme compatibility.
Every built-in theme file already defines
border3. Any custom theme withoutborder3would fail validation and fall back to the default theme, so document or update that custom-theme requirement separately.🤖 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 `@packages/bruno-app/src/themes/schema/oss.js` around lines 153 - 158, Keep the builtin theme schema’s border3 requirement unchanged for compatibility with existing built-in themes. Handle the custom-theme requirement separately by documenting that custom themes must define border3, without modifying the validation behavior in the schema.
🤖 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 `@packages/bruno-app/src/themes/DesignSystem/Overview.jsx`:
- Line 286: Update the Border Hierarchy preview data in Overview.jsx to add a
border3 row alongside border0, border1, and border2, using the existing row
shape and theme.border.border3 color so the preview displays all four hierarchy
levels.
In `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Line 53: Remove the `'checkbox'` fallback from the `dataTestId` destructuring
in the Checkbox component so the attribute is only applied when callers
explicitly provide a value. Preserve the existing Checkbox rendering behavior
and ensure the `Sizes` and `States` stories no longer produce duplicate default
test IDs.
---
Nitpick comments:
In `@packages/bruno-app/src/themes/schema/oss.js`:
- Around line 153-158: Keep the builtin theme schema’s border3 requirement
unchanged for compatibility with existing built-in themes. Handle the
custom-theme requirement separately by documenting that custom themes must
define border3, without modifying the validation behavior in the schema.
🪄 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 Plus
Run ID: d38a2307-f555-42dc-af1c-4eca913d8a05
📒 Files selected for processing (20)
packages/bruno-app/src/themes/DesignSystem/Overview.jsxpackages/bruno-app/src/themes/dark/catppuccin-frappe.jspackages/bruno-app/src/themes/dark/catppuccin-macchiato.jspackages/bruno-app/src/themes/dark/catppuccin-mocha.jspackages/bruno-app/src/themes/dark/dark-monochrome.jspackages/bruno-app/src/themes/dark/dark-pastel.jspackages/bruno-app/src/themes/dark/dark.jspackages/bruno-app/src/themes/dark/nord.jspackages/bruno-app/src/themes/dark/vscode.jspackages/bruno-app/src/themes/light/catppuccin-latte.jspackages/bruno-app/src/themes/light/light-monochrome.jspackages/bruno-app/src/themes/light/light-pastel.jspackages/bruno-app/src/themes/light/light.jspackages/bruno-app/src/themes/light/vscode.jspackages/bruno-app/src/themes/schema/oss.jspackages/bruno-app/src/ui/Checkbox/Checkbox.spec.jsxpackages/bruno-app/src/ui/Checkbox/Checkbox.stories.jsxpackages/bruno-app/src/ui/Checkbox/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/index.jspackages/bruno-app/src/ui/index.js
| { name: 'Backgrounds', desc: '6 layered surface colors', color: theme.background.mantle }, | ||
| { name: 'Text', desc: '8 semantic text colors', color: theme.text }, | ||
| { name: 'Borders', desc: '3 hierarchy levels', color: theme.border.border2 }, | ||
| { name: 'Borders', desc: '4 hierarchy levels', color: theme.border.border2 }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add border3 to the Border Hierarchy preview.
The overview now states that four border levels exist and lists border3, but the Border Hierarchy preview still renders only border0, border1, and border2. Add a border3 row so both sections show the same hierarchy.
Also applies to: 1309-1310
🤖 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 `@packages/bruno-app/src/themes/DesignSystem/Overview.jsx` at line 286, Update
the Border Hierarchy preview data in Overview.jsx to add a border3 row alongside
border0, border1, and border2, using the existing row shape and
theme.border.border3 color so the preview displays all four hierarchy levels.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-app/src/ui/Checkbox/index.js (1)
45-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for
inputClassName.Verify that a custom input class is appended while
checkbox-inputremains present. This protects the new public prop contract.🤖 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 `@packages/bruno-app/src/ui/Checkbox/index.js` around lines 45 - 64, Add a regression test for the Checkbox component covering the inputClassName prop: render with a custom class and assert the input retains the base checkbox-input class while also including the custom class.
🤖 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 `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 45-64: Add a regression test for the Checkbox component covering
the inputClassName prop: render with a custom class and assert the input retains
the base checkbox-input class while also including the custom class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 63bb9e25-4754-4c40-807f-422d6455b80b
📒 Files selected for processing (1)
packages/bruno-app/src/ui/Checkbox/index.js
|
This is cool @sachin-thakur-bruno I recommend updating once component in the codebase to use this new Checkbox component. |
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)
packages/bruno-app/src/ui/Checkbox/index.js (1)
139-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent passthrough props from overriding the input type.
Line 150 applies
...restaftertype="checkbox". A caller can passtype="radio"ortype="text"and break the component’s form and accessibility semantics.Place the spread before invariant Checkbox attributes, or remove
typefrom the forwarded props.Proposed fix
<input + {...rest} ref={mergeRefs(inputRef, forwardedRef)} type="checkbox" id={id} name={name} value={value} checked={checked} disabled={disabled} onChange={onChange} className={`checkbox-input ${inputClassName}`.trim()} data-testid={dataTestId} aria-label={ariaLabel} aria-labelledby={ariaLabelledBy} - {...rest} />🤖 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 `@packages/bruno-app/src/ui/Checkbox/index.js` around lines 139 - 150, Update the Checkbox input JSX so the {...rest} spread cannot override the invariant type="checkbox" attribute; move the spread before the fixed Checkbox attributes in the input render block, or explicitly exclude type from forwarded props while preserving other passthrough properties.
🧹 Nitpick comments (1)
packages/bruno-app/src/ui/Checkbox/Checkbox.stories.jsx (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Tailwind classes for story layout.
Replace these inline layout styles with Tailwind layout classes. The JSX guideline requires Tailwind classes specifically for layout.
For example, use
flex items-center gap-6,flex flex-col gap-2, andpl-5.As per coding guidelines, “Use styled components for component and child styling, and Tailwind classes specifically for layout.” <coding_guidelines>
Also applies to: 122-122, 131-131
🤖 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 `@packages/bruno-app/src/ui/Checkbox/Checkbox.stories.jsx` at line 92, Replace the inline layout style objects on the story wrapper elements in the Checkbox stories with equivalent Tailwind className values, including the affected layouts at the referenced locations. Use classes such as flex, items-center, gap-6, flex-col, gap-2, and pl-5 as appropriate, while preserving the existing layout behavior.Source: Coding guidelines
🤖 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 `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 95-99: Stabilize the callback returned by mergeRefs so rerenders
reuse the same ref function instead of triggering React ref replacement with an
intermediate null. Update the Checkbox implementation and add a callback-ref
test that rerenders with a changed checked value, verifying the DOM node is not
preceded by null.
---
Outside diff comments:
In `@packages/bruno-app/src/ui/Checkbox/index.js`:
- Around line 139-150: Update the Checkbox input JSX so the {...rest} spread
cannot override the invariant type="checkbox" attribute; move the spread before
the fixed Checkbox attributes in the input render block, or explicitly exclude
type from forwarded props while preserving other passthrough properties.
---
Nitpick comments:
In `@packages/bruno-app/src/ui/Checkbox/Checkbox.stories.jsx`:
- Line 92: Replace the inline layout style objects on the story wrapper elements
in the Checkbox stories with equivalent Tailwind className values, including the
affected layouts at the referenced locations. Use classes such as flex,
items-center, gap-6, flex-col, gap-2, and pl-5 as appropriate, while preserving
the existing layout behavior.
🪄 Autofix
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 Plus
Run ID: f7c7731f-43fc-4f5a-b0a4-663d79e8a197
📒 Files selected for processing (4)
packages/bruno-app/src/ui/Checkbox/Checkbox.spec.jsxpackages/bruno-app/src/ui/Checkbox/Checkbox.stories.jsxpackages/bruno-app/src/ui/Checkbox/StyledWrapper.jspackages/bruno-app/src/ui/Checkbox/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/ui/Checkbox/StyledWrapper.js
sid-bruno
changed the title
…uno into chore/checkbox-ui
sachin-thakur-bruno
changed the title
2 participants
Description
BRU-4163
Adds a new reusable
Checkboxprimitive to the design-system location (packages/bruno-app/src/ui/Checkbox), plus a newborder3theme token added to all 13 built-in themes.https://www.figma.com/design/uDY1jX4jDj8uBDand72oBf/%F0%9F%8C%9F-New-DS-2026?node-id=30-220&p=f&t=fcNSTF7uheutLp1N-0
Problem
There was no general-purpose
Checkboxinui/. The only existingCheckbox(components/Checkbox) was a single hardcoded 16px size, noref/size/accessible-name support, and colors pulled from tokens that were never meant as a general checkbox API . It also had zero usages anywhere in the app.Fix
ui/Checkbox(index.js+StyledWrapper.js).border3to the palette/schema of every theme file so the checkbox (and future components) has a dedicated border-only token instead of reusing text-muted colors.Checkbox.stories.jsx) and a full Jest spec (Checkbox.spec.jsx) covering checked/disabled/keyboard/ref-forwarding/accessible-name behavior.Screenshots
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Documentation
Tests