feat(ui): Add Badge component with tests and stories #40
Conversation
run all the test cases , fixes the error and delete errornious test cases
WalkthroughAdded six Storybook stories for Badge, a comprehensive test suite exercising rendering, variants, removable behavior, accessibility, and edge cases, and applied Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/Badge/Badge.tsx (1)
12-14: Update BadgeProps interface to reflect optional props with defaults.The type contract mismatch is confirmed.
BadgePropsdeclaresvariantandremovableas required, but the component implementation treats them as optional with default values via??operators (lines 13-14 of Badge.tsx). This contradicts the type contract.Update
src/components/ui/Badge/BadgeProps.tsto make these props optional:export interface BadgeProps { text: string, color: string, variant?: 'primary' | 'secondary' | 'accent' | 'ghost', removable?: boolean, onRemove?: () => void }
🧹 Nitpick comments (4)
src/components/ui/Badge/Badge.test.tsx (2)
155-176: Consider simpler approach for undefined prop testing.The tests use
delete (props as BadgeProps).onRemoveto test missing callbacks. SinceonRemoveis already optional in the interface, you can simply passundefinedor omit it from the object for cleaner, more idiomatic code.it('renders without onRemove callback when removable is true', () => { - const props = makeProps({ removable: true }); - delete (props as BadgeProps).onRemove; + const props = makeProps({ removable: true, onRemove: undefined }); expect(() => { render(<Badge {...props} />); }).not.toThrow();
298-331: Consider testing for accessible button labels.While the accessibility tests verify button presence and keyboard focus, they don't validate screen reader experience. The remove button currently only contains "✕" without an
aria-label, which may not be sufficiently descriptive for screen reader users.Consider adding a test to verify accessible labeling:
it('remove button has accessible label for screen readers', () => { const props = makeProps({ removable: true, onRemove: vi.fn() }); render(<Badge {...props} />); const removeButton = screen.getByRole('button'); expect(removeButton).toHaveAccessibleName(/remove/i); });Then update the Badge component to include proper labeling:
{removable && ( - <button onClick={props.onRemove} className="btn btn-ghost btn-xs"> + <button onClick={props.onRemove} className="btn btn-ghost btn-xs" aria-label="Remove badge"> ✕ </button> )}src/components/ui/Badge/Badge.stories.tsx (2)
21-73: Remove unusedcolorprop from stories.The Badge component explicitly ignores the
colorprop in favor ofvariant(as noted in Badge.tsx lines 15-16). Includingcolorin the story args is misleading and adds no value.Remove the
colorprop from all story definitions:export const BadgeComponentRenderingPass: Story = { args: { text: "sale 50% off", - color: "color", variant: "primary", removable: false } };Apply this change to all stories that include the
colorprop.
13-19: Make Default story more representative.The Default story is missing
variantandremovableprops, relying on component defaults. For better documentation, explicitly show all key props.export const Default: Story = { args: { text: "text", - color: "color", + variant: "primary", + removable: false, - onRemove: () => {} } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/components/ui/Badge/Badge.stories.tsx(1 hunks)src/components/ui/Badge/Badge.test.tsx(1 hunks)src/components/ui/Badge/Badge.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/Badge/Badge.test.tsx (2)
src/components/ui/Badge/BadgeProps.ts (1)
BadgeProps(1-7)src/components/ui/Badge/Badge.tsx (1)
Badge(12-31)
🔇 Additional comments (4)
src/components/ui/Badge/Badge.tsx (1)
25-25: LGTM! Appropriate styling for text preservation.The
whitespace-pre-wrapclass correctly preserves whitespace while allowing text to wrap, which aligns with the test coverage and the "WhitespaceErrorSolved" story.src/components/ui/Badge/Badge.test.tsx (3)
1-6: LGTM! Clean test setup.All necessary testing utilities are correctly imported.
9-15: LGTM! Well-designed test helper.The
makePropshelper provides sensible defaults and allows flexible overrides via the spread operator.
369-374: LGTM! Good edge case coverage.The whitespace test correctly validates the
whitespace-pre-wrapbehavior added to the component.
correct the erroneous test code
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ui/Badge/Badge.test.tsx (1)
279-295: Potential DOM pollution in loop test.Rendering multiple Badge components in a loop without unmounting previous instances may cause DOM pollution. The
screen.getByRole('button')query could match buttons from earlier iterations if cleanup doesn't occur between renders.Consider either:
- Creating separate test cases for each variant
- Using
unmount()between iterations- Using
within()to scope queries to the specific containervariants.forEach((variant) => { const props = makeProps({ variant, removable: true, onRemove: vi.fn() }); - const { container } = render(<Badge {...props} />); + const { container, unmount } = render(<Badge {...props} />); const badge = container.firstElementChild as HTMLElement; expect(badge).toHaveClass(`badge-${variant}`); - expect(screen.getByRole('button')).toBeInTheDocument(); + expect(within(container).getByRole('button')).toBeInTheDocument(); + unmount(); });
🧹 Nitpick comments (5)
src/components/ui/Badge/Badge.test.tsx (5)
103-120: Consider consolidating redundant color tests.Both tests verify the same behavior (color prop is ignored in favor of variant) with different values. One test would suffice, reducing maintenance overhead.
Example consolidation:
describe('Color Prop', () => { it('ignores color prop in favor of variant', () => { const props = makeProps({ color: 'red', variant: 'primary' }); const { container } = render(<Badge {...props} />); const badge = container.firstElementChild as HTMLElement; expect(badge).toHaveClass('badge-primary'); expect(badge).not.toHaveClass('red'); }); - - it('variant takes precedence over color', () => { - const props = makeProps({ color: 'blue', variant: 'accent' }); - const { container } = render(<Badge {...props} />); - - const badge = container.firstElementChild as HTMLElement; - expect(badge).toHaveClass('badge-accent'); - }); });
155-176: Consider cleaner approach for testing undefined onRemove.Using
deleteto remove the property works but is less idiomatic. You can achieve the same result by explicitly passingonRemove: undefinedinmakeProps.Apply this diff:
it('renders without onRemove callback when removable is true', () => { - const props = makeProps({ removable: true }); - delete (props as BadgeProps).onRemove; + const props = makeProps({ removable: true, onRemove: undefined }); expect(() => { render(<Badge {...props} />); }).not.toThrow(); const removeButton = screen.getByRole('button'); expect(removeButton).toBeInTheDocument(); }); it('does not throw error when clicking remove button without onRemove callback', () => { - const props = makeProps({ removable: true }); - delete (props as BadgeProps).onRemove; + const props = makeProps({ removable: true, onRemove: undefined }); render(<Badge {...props} />); const removeButton = screen.getByRole('button'); expect(() => { fireEvent.click(removeButton); }).not.toThrow(); });
369-374: Whitespace test doesn't verify whitespace preservation.The test uses a partial regex match
/Spaced/which will pass regardless of whether thewhitespace-pre-wrapclass actually preserves leading/trailing whitespace. Since the component explicitly applies this class, the test should verify that whitespace is preserved in the rendered output.Apply this diff to properly test whitespace preservation:
it('handles whitespace in text', () => { const props = makeProps({ text: ' Spaced ' }); - render(<Badge {...props} />); + const { container } = render(<Badge {...props} />); - expect(screen.getByText(/Spaced/)).toBeInTheDocument(); + const span = container.querySelector('span'); + expect(span).toHaveTextContent(' Spaced '); + expect(span).toHaveClass('whitespace-pre-wrap'); });
356-367: Consider removing redundant rapid clicks test.This test is functionally identical to the "calls onRemove multiple times" test at lines 134-145. Both verify that
onRemoveis called the correct number of times on multiple clicks. The difference in click count (10 vs 3) doesn't add meaningful coverage.
377-393: Consider removing redundant default value tests.Both tests duplicate coverage from earlier sections:
- Lines 378-384 duplicate the "renders with default props" test (lines 18-25)
- Lines 386-392 duplicate the "does not render remove button when removable is false" test (lines 43-49)
While documenting default behavior is valuable, these specific assertions are already covered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/Badge/Badge.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/Badge/Badge.test.tsx (2)
src/components/ui/Badge/BadgeProps.ts (1)
BadgeProps(1-7)src/components/ui/Badge/Badge.tsx (1)
Badge(12-31)
🔇 Additional comments (6)
src/components/ui/Badge/Badge.test.tsx (6)
1-15: LGTM! Clean test setup.The imports and
makePropshelper follow testing best practices. The factory function with overrides makes tests readable and maintainable.
17-59: LGTM! Comprehensive rendering tests.The rendering tests appropriately cover default props, custom text, and conditional remove button rendering. Good use of
queryByRolefor negative assertions.
61-101: LGTM! Variant tests are thorough.All variant options are properly tested. The explicit test for base classes (lines 94-100) serves as useful documentation even though similar assertions appear elsewhere.
179-219: LGTM! Text content tests cover various scenarios.The tests appropriately handle edge cases including empty strings, numeric values, long text, special characters, and emoji.
221-259: LGTM! DOM structure tests are well-designed.The tests verify the component's DOM hierarchy and element relationships correctly.
298-331: LGTM! Good accessibility coverage.The tests appropriately verify keyboard accessibility, focus behavior, and semantic HTML roles.
Description
This pull request introduces a new, reusable
Badgecomponent to the UI library. This component is designed to display short pieces of information, such as tags or statuses, with styling based on DaisyUI variants.Key Features:
primary,secondary,accent, andghostvariants for different visual styles.removableprop that, when true, displays a close button and triggers anonRemovecallback.colorprop is intentionally ignored in favor of thevariantprop to align with DaisyUI's class-based styling system.whitespace-pre-wrap.To ensure quality and document usage, this PR also includes:
Badgecomponent using Vitest and React Testing Library, covering rendering, variants, functionality, accessibility, and edge cases.Type of Change
How Has This Been Tested?
The
Badgecomponent has been thoroughly tested to ensure its reliability and correctness:Badge.test.tsxwith 100% coverage of all logic, including:onRemoveevent handler functionality.Badge.stories.tsxto allow for visual regression testing and to serve as living documentation for the component's appearance and behavior across different props.All tests are passing in the CI pipeline.
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Documentation