-
Notifications
You must be signed in to change notification settings - Fork 0
feat(TagBadge): add TagBadge component #42
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.
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
base: main
Are you sure you want to change the base?
Conversation
add TagBadge component with props for tag handling
WalkthroughIntroduces a new TagBadge component that wraps the Badge UI component to display tags with customizable properties. Simultaneously extends BadgeProps with an onClick handler to support clickable badges. The TagBadge component extracts display text from tag strings or objects and forwards styling and interaction props to the underlying Badge. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/components/features/tags/TagBadge/TagBadge.tsx (1)
17-17: JSDoc example uses non-existentidproperty.The Tag interface (TagBadgeProps.ts) only defines
name: string, but this example shows a tag object with anidproperty. Update the example to match the actual interface or extend the Tag interface to includeid.
🧹 Nitpick comments (1)
src/components/features/tags/TagBadge/TagBadge.tsx (1)
25-25: Hard-coded variant limits component flexibility.Setting
variant={"primary"}prevents TagBadge from using other badge styles ('secondary', 'accent', 'ghost'). Consider exposing variant as an optional prop in TagBadgeProps to allow customization.Add variant to TagBadgeProps:
export default interface TagBadgeProps { tag: string | Tag + variant?: 'primary' | 'secondary' | 'accent' | 'ghost' removable?: boolean onRemove?: () => void onClick?: () => void - color: string }Then use it in TagBadge:
+ const variant = props.variant ?? 'primary'; <Badge text={badgeText} - color={props.color} - variant={"primary"} + variant={variant} removable={props.removable} onRemove={props.onRemove} onClick={props.onClick}> </Badge>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/features/tags/TagBadge/TagBadge.tsx(1 hunks)src/components/features/tags/TagBadge/TagBadgeProps.ts(1 hunks)src/components/ui/Badge/BadgeProps.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/features/tags/TagBadge/TagBadge.tsx (2)
src/components/features/tags/TagBadge/TagBadgeProps.ts (1)
TagBadgeProps(1-7)src/components/ui/Badge/Badge.tsx (1)
Badge(12-31)
| return ( | ||
| <Badge | ||
| text={badgeText} | ||
| color={props.color} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unused color prop.
The Badge component explicitly ignores the color prop in favor of variant (see Badge.tsx lines 14-15 comment). Passing color here is misleading because it suggests the prop has an effect when it doesn't. Either remove the color prop from TagBadgeProps, or use it to determine the variant value.
Option 1: Remove color entirely if not needed:
export default interface TagBadgeProps {
tag: string | Tag
removable?: boolean
onRemove?: () => void
onClick?: () => void
- color: string
}Option 2: Map color to variant (if you want to keep the color prop):
- <Badge
- text={badgeText}
- color={props.color}
- variant={"primary"}
+ const variant = props.color || "primary";
+ <Badge
+ text={badgeText}
+ color={props.color}
+ variant={variant as BadgeProps['variant']}However, option 2 still leaves the color prop on Badge unused, so option 1 is cleaner.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/features/tags/TagBadge/TagBadge.tsx around line 24, the Badge
is being passed a color prop that Badge ignores; remove the unused color prop
from TagBadgeProps and stop passing color to Badge (delete the
color={props.color} prop) OR, if you intend to keep a color API, map your color
value to the appropriate Badge variant before rendering and pass only variant to
Badge (remove direct color usage). Also update the TagBadgeProps type to remove
color (preferred) or adjust it to reflect the mapped variant.
| removable: boolean | ||
| onRemove: () => void | ||
| onClick: () => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make interaction props optional for better API ergonomics.
The removable, onRemove, and onClick properties are currently required, but not all use cases need removable or clickable badges. Since the underlying Badge component treats these as optional (with ?: modifiers and fallback defaults), TagBadgeProps should follow the same pattern.
Apply this diff to make these props optional:
export default interface TagBadgeProps {
tag: string | Tag
- removable: boolean
- onRemove: () => void
- onClick: () => void
+ removable?: boolean
+ onRemove?: () => void
+ onClick?: () => void
color: string
}📝 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.
| removable: boolean | |
| onRemove: () => void | |
| onClick: () => void | |
| export default interface TagBadgeProps { | |
| tag: string | Tag | |
| removable?: boolean | |
| onRemove?: () => void | |
| onClick?: () => void | |
| color: string | |
| } |
🤖 Prompt for AI Agents
In src/components/features/tags/TagBadge/TagBadgeProps.ts around lines 3 to 5,
the props removable, onRemove, and onClick are defined as required but should be
optional to match the underlying Badge API; update their declarations to be
optional (use the `?` modifier) so the type becomes `removable?: boolean`,
`onRemove?: () => void`, and `onClick?: () => void`, ensuring callers can omit
them and existing fallback/default behavior remains valid.
| export interface Tag { | ||
| name: string | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSDoc example references non-existent id property.
The Tag interface only defines name, but the JSDoc example in TagBadge.tsx (line 17) shows { id: '1', name: 'typescript' }. Either add the id property to the Tag interface if it's needed, or update the JSDoc example to remove it.
If id is needed for tag identification:
export interface Tag {
+ id?: string
name: string
}Otherwise, correct the JSDoc example in TagBadge.tsx line 17.
📝 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.
| export interface Tag { | |
| name: string | |
| } | |
| export interface Tag { | |
| id?: string | |
| name: string | |
| } |
🤖 Prompt for AI Agents
In src/components/features/tags/TagBadge/TagBadgeProps.ts around lines 9 to 11,
the Tag interface only defines `name` but the JSDoc example in TagBadge.tsx
references a non-existent `id` property; either add an `id: string` (or
appropriate type) field to the Tag interface if tags require identifiers, or
leave the interface as-is and update the JSDoc in TagBadge.tsx (line 17) to
remove `id` from the example so it matches the interface.
| variant: 'primary' | 'secondary' | 'accent' | 'ghost', | ||
| removable: boolean, | ||
| onRemove?: () => void, | ||
| onClick?: () => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: onClick handler is not implemented in Badge component.
The onClick prop is added to the interface but the Badge component (Badge.tsx lines 11-30) does not attach this handler to any element. The component renders a <div> without an onClick attribute, so clicking the badge has no effect.
To fix this, the Badge component must be updated to attach the onClick handler:
return (
<div className={badgeClasses} onClick={props.onClick}>
<span className="whitespace-pre-wrap">{props.text}</span>
{removable && (<button onClick={props.onRemove} className="btn btn-ghost btn-xs">
✕
</button>)}
</div>
);Alternatively, if onClick is not needed for Badge itself, reconsider whether TagBadge should forward it or handle it differently.
🤖 Prompt for AI Agents
In src/components/ui/Badge/BadgeProps.ts around line 7, the onClick prop is
declared but the Badge component (Badge.tsx lines 11-30) does not use it, so
clicks do nothing; update the Badge component to attach props.onClick to the
root element (e.g., add onClick={props.onClick} to the rendered div), ensure the
handler is optional (only attached when provided) and preserve existing
removable button behavior, and also audit any consumers like TagBadge to forward
the onClick prop if they intend to expose badge clicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onClick is not needed for Badge itself, then how to solve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the TagBadge implementation to understand how onClick is being used
cat src/components/features/tags/TagBadge/TagBadge.tsx
# Also check if there are any tests that use onClick
cat src/components/features/tags/TagBadge/TagBadge.test.tsx
Length of output: 1140
🏁 Script executed:
#!/bin/bash
# Check for test files with different naming patterns
fd -e tsx -e ts "TagBadge" --type f
# Check the stories file to understand the intended usage
cat src/components/features/tags/TagBadge/TagBadge.stories.tsx 2>/dev/null || echo "Stories file not found"
# Search for any usage of TagBadge in the codebase
rg -n "TagBadge" --type tsx --type ts -C 2 2>/dev/null || rg -n "TagBadge" -g "*.tsx" -g "*.ts" -C 2
Length of output: 2318
@Arnab-Mandal1, if onClick is not needed for the Badge component itself, the solution is to remove onClick from BadgeProps and instead handle the click behavior at the TagBadge level by wrapping the Badge in a clickable element. This maintains better separation of concerns—Badge remains a simple presentational component, while TagBadge handles the interactive behavior.
Here's how to fix it:
1. Remove onClick from BadgeProps.ts:
export interface BadgeProps {
text: string,
color: string,
variant: 'primary' | 'secondary' | 'accent' | 'ghost',
removable: boolean,
onRemove?: () => void
}2. Update TagBadge.tsx to wrap Badge in a clickable element:
export function TagBadge(props: Readonly<TagBadgeProps>) {
const badgeText = typeof props.tag === 'string' ? props.tag : props.tag.name;
const badge = (
<Badge
text={badgeText}
color={props.color}
variant={"primary"}
removable={props.removable}
onRemove={props.onRemove}>
</Badge>
);
// Wrap in clickable div only if onClick is provided
if (props.onClick) {
return (
<div onClick={props.onClick} style={{ cursor: 'pointer', display: 'inline-block' }}>
{badge}
</div>
);
}
return badge;
}This approach:
- ✅ Keeps Badge as a pure UI component without click handling
- ✅ TagBadge handles its own click behavior when needed
- ✅ No breaking changes to Badge component
- ✅ Better component architecture and reusability
Description
Implements a reusable
TagBadgeUI component that renders aBadgefor a tag (string or object), forwards click and remove handlers, and supports removable badges. Includes tests and a Storybook story.Type of Change
Changes Made
src/components/features/tags/TagBadge/TagBadge.tsxsrc/components/features/tags/TagBadge/TagBadgeProps.tssrc/components/features/tags/TagBadge/TagBadge.test.tsxsrc/components/features/tags/TagBadge/TagBadge.stories.tsxTesting Done
npm run test -- --run)npm run storybook)Screenshots (if applicable)
N/A
Related Issues
Closes #issue_number
Checklist
console.logor debugging codeSummary by CodeRabbit