Skip to content

refactor(Badge): make BadgeProps readonly for better immutability #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Arnab-Mandal1 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(Badge): make BadgeProps readonly for better immutability #43

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 14 additions & 18 deletions src/components/ui/Badge/Badge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,19 @@ import type {BadgeProps} from "./BadgeProps.ts";
* @example
* <Badge text="New" variant="primary" removable onRemove={() => console.log('removed')} />
*/
export function Badge(props: BadgeProps) {
const variant = props.variant ?? 'primary';
const removable = props.removable ?? false;
// Note: The 'color' prop from BadgeProps is ignored in favor of the 'variant'
// prop, which aligns with DaisyUI's class-based styling.
const badgeClasses = [
'badge',
`badge-${variant}`,
'gap-2'
].join(' ');
export function Badge(props: Readonly<BadgeProps>) {
const variant = props.variant ?? 'primary';
const removable = props.removable ?? false;
Comment on lines +12 to +14
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Good use of Readonly<BadgeProps> for immutability, but verify the color prop removal.

The Readonly wrapper enforces immutability at the type level, which aligns with the PR objectives. However, the PR description states that "an unused color prop was removed from BadgeProps," yet the BadgeProps interface in BadgeProps.ts (lines 1-6) still includes the color: string property.

Run the following script to verify that the color property has been removed from the BadgeProps interface:

🏁 Script executed:

#!/bin/bash
# Description: Verify that the color property is removed from BadgeProps interface

# Check if color property still exists in BadgeProps.ts
rg -n -A 5 'interface BadgeProps' src/components/ui/Badge/BadgeProps.ts

Length of output: 172

Remove the unused color property from BadgeProps interface.

The color property still exists in BadgeProps.ts (line 3), contradicting the PR description which claims it was removed. If this property is truly unused, it should be removed from the interface to avoid confusion and reduce surface area.

Edit src/components/ui/Badge/BadgeProps.ts to remove the color: string, line.

🤖 Prompt for AI Agents 
In src/components/ui/Badge/Badge.tsx around lines 12 to 14, the BadgeProps
interface still contains an unused color property; open
src/components/ui/Badge/BadgeProps.ts and remove the line defining color:
string, from the interface so the props match actual usage, then run typecheck
to ensure no remaining references to color exist and update any callers if
necessary.

// Note: The 'color' prop from BadgeProps is ignored in favor of the 'variant'
// prop, which aligns with DaisyUI's class-based styling.
const badgeClasses = ['badge', `badge-${variant}`, 'gap-2'].join(' ');
Comment on lines +15 to +17
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: gap-2 is not applied conditionally despite PR description claiming it is.

The PR description states: "The gap-2 class is now applied conditionally only when removable is true to avoid unnecessary spacing on non-removable badges." However, line 17 shows that gap-2 is always included in the badgeClasses array, regardless of the removable flag.

Additionally, the comment on lines 15-16 acknowledges that the color prop is ignored, yet it remains in the BadgeProps interface. This is a code smell—unused properties should be removed from the interface entirely.

Apply this diff to conditionally apply gap-2 and remove the misleading comment:

-    // Note: The 'color' prop from BadgeProps is ignored in favor of the 'variant'
-    // prop, which aligns with DaisyUI's class-based styling.
-    const badgeClasses = ['badge', `badge-${variant}`, 'gap-2'].join(' ');
+    const badgeClasses = ['badge', `badge-${variant}`, removable ? 'gap-2' : ''].filter(Boolean).join(' ');

Additionally, remove the color property from the BadgeProps interface in src/components/ui/Badge/BadgeProps.ts.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Note: The 'color' prop from BadgeProps is ignored in favor of the 'variant'
// prop, which aligns with DaisyUI's class-based styling.
const badgeClasses = ['badge', `badge-${variant}`, 'gap-2'].join(' ');
const badgeClasses = ['badge', `badge-${variant}`, removable ? 'gap-2' : ''].filter(Boolean).join(' ');
🤖 Prompt for AI Agents 
In src/components/ui/Badge/Badge.tsx around lines 15 to 17, the class list
always includes 'gap-2' contrary to the PR description—change the badgeClasses
construction so 'gap-2' is only added when the removable prop is true (e.g.,
build the array conditionally or push 'gap-2' only if removable), and update or
remove the misleading comment about the color prop being ignored; in
src/components/ui/Badge/BadgeProps.ts remove the unused color property from the
BadgeProps interface and update any imports/usages of that interface accordingly
so types remain consistent.


return (
<div className={badgeClasses}>
<span className="whitespace-pre-wrap">{props.text}</span>
{removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs">
✕
</button>)}
</div>
);
return (
<div className={badgeClasses}>
<span className="whitespace-pre-wrap">{props.text}</span>
{removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs">
✕
</button>)}
Comment on lines +22 to +24
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Critical: Missing aria-label on close button despite PR description claiming it was added.

The PR description states: "Accessibility improved by adding aria-label='Remove badge' to the close button." However, the close button on lines 22-24 does not include an aria-label attribute. This is a critical discrepancy between the PR description and the actual implementation, and it leaves the component with inadequate accessibility for screen reader users.

Apply this diff to add the aria-label attribute:

-            {removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs">
+            {removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs" aria-label="Remove badge">
                 ✕
             </button>)}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs">
✕
</button>)}
{removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs" aria-label="Remove badge">
✕
</button>)}
🤖 Prompt for AI Agents 
In src/components/ui/Badge/Badge.tsx around lines 22 to 24, the removable close
button lacks the aria-label promised in the PR description; update the button
element to include an accessible name by adding aria-label="Remove badge" (or
the appropriate localized string) to the button so screen readers announce its
purpose while preserving the existing onClick handler and classes.

</div>
);
}