fix: update Badge component type safety and props#518
Conversation
jdhoffa
requested review from
AlexAxthelm and
Copilot
and removed request for
Copilot
Expected version change and release notes:1.5.0-dev.18 (v1.5.0-dev.17...fix_ts_errors_badge_component ) (2025-11-05T13:26 UTC)Fixes |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://proud-glacier-0f640931e-518.westus2.2.azurestaticapps.net |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Badge component to use children-based API instead of text prop, adds two new badge variants ("metric" and "keyFeature"), and includes code cleanup. The changes improve the component's API consistency with React conventions while removing redundant comments.
- Removed the
textprop in favor of usingchildrenprop for badge content - Added "metric" and "keyFeature" badge variants with corresponding styles
- Updated the
BadgeMaybeAbsentcomponent'srenderLabelreturn type fromReact.ReactNodetostring | number
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/Badge.tsx | Added two new badge variants, removed redundant comments, changed renderLabel return type, and added explicit String() conversion for content rendering |
| src/components/Badge.test.tsx | Updated all test cases to use children-based API instead of text prop, changed test element from <span> to <div> for compile-time checks, removed redundant comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| noneLabel?: string; | ||
| /** Optional decorator for the final label (e.g., highlight search matches). */ | ||
| renderLabel?: (label: string, isAbsent: boolean) => React.ReactNode; | ||
| renderLabel?: (label: string, isAbsent: boolean) => string | number; |
There was a problem hiding this comment.
The return type change from React.ReactNode to string | number is a breaking change that restricts the flexibility of renderLabel. This prevents consumers from returning JSX elements (e.g., highlighted text with <mark> tags). Consider reverting this change or providing migration guidance if this restriction is intentional.
| renderLabel && typeof base === "string" ? renderLabel(base, absent) : base; | ||
| return <Badge {...rest}>{content}</Badge>; | ||
|
|
||
| return <Badge {...rest}>{String(content)}</Badge>; |
There was a problem hiding this comment.
The explicit String(content) conversion is redundant when content is already of type string | number. Since Badge accepts children: string | number, the conversion to string is unnecessary and could mask potential type issues. Consider removing the String() wrapper.
| return <Badge {...rest}>{String(content)}</Badge>; | |
| return <Badge {...rest}>{content}</Badge>; |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://proud-glacier-0f640931e-518.westus2.2.azurestaticapps.net |
1 participant
renderLabelreturns a "string" directlyRelates to #514