Skip to content

fix: update Badge component type safety and props #518

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

Closed
jdhoffa wants to merge 4 commits into from
Closed

fix: update Badge component type safety and props #518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c21a764
fix: update Badge component type safety and props
jdhoffa Nov 5, 2025
a89b1f4
prettier --write .
jdhoffa Nov 5, 2025
e474951
fix: renderLabel returns string directly
jdhoffa Nov 5, 2025
1eae118
Merge branch 'main' into fix_ts_errors_badge_component
jdhoffa Nov 5, 2025
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
91 changes: 20 additions & 71 deletions src/components/Badge.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,18 @@ describe("Badge component", () => {
expect(badge).toHaveClass("border-rmigray-200");
});

// Type assertions: children must be scalar (string|number)
it("disallows ReactNode children for Badge at compile-time", () => {
// @ts-expect-error - Badge children must be string | number
render(
<Badge>
<span>Not allowed</span>
<div>Not allowed</div>
</Badge>,
);
});

it("applies pathwayType styling when variant is 'pathwayType'", () => {
const { container } = render(
<Badge
text="Pathway Type"
variant="pathwayType"
/>,
<Badge variant="pathwayType">Pathway Type</Badge>,
);
const badge = container.firstChild as HTMLElement;

Expand All @@ -48,12 +44,7 @@ describe("Badge component", () => {
});

it("applies temperature styling when variant is 'temperature'", () => {
const { container } = render(
<Badge
text="1.5°C"
variant="temperature"
/>,
);
const { container } = render(<Badge variant="temperature">1.5°C</Badge>);
const badge = container.firstChild as HTMLElement;

expect(badge).toHaveClass("bg-rmired-100");
Expand All @@ -62,12 +53,7 @@ describe("Badge component", () => {
});

it("applies year styling when variant is 'year'", () => {
const { container } = render(
<Badge
text="2050"
variant="year"
/>,
);
const { container } = render(<Badge variant="year">2050</Badge>);
const badge = container.firstChild as HTMLElement;

expect(badge).toHaveClass("bg-rmiblue-100");
Expand All @@ -77,10 +63,7 @@ describe("Badge component", () => {

it("applies geography styling when variant is 'geographyGlobal'", () => {
const { container } = render(
<Badge
text="Global"
variant="geographyGlobal"
/>,
<Badge variant="geographyGlobal">Global</Badge>,
);
const badge = container.firstChild as HTMLElement;

Expand All @@ -91,10 +74,7 @@ describe("Badge component", () => {

it("applies geography styling when variant is 'geographyRegion'", () => {
const { container } = render(
<Badge
text="RegionName"
variant="geographyRegion"
/>,
<Badge variant="geographyRegion">RegionName</Badge>,
);
const badge = container.firstChild as HTMLElement;

Expand All @@ -105,10 +85,7 @@ describe("Badge component", () => {

it("applies geography styling when variant is 'geographyCountry'", () => {
const { container } = render(
<Badge
text="CountryName"
variant="geographyCountry"
/>,
<Badge variant="geographyCountry">CountryName</Badge>,
);
const badge = container.firstChild as HTMLElement;

Expand All @@ -118,12 +95,7 @@ describe("Badge component", () => {
});

it("applies sector styling when variant is 'sector'", () => {
const { container } = render(
<Badge
text="Energy"
variant="sector"
/>,
);
const { container } = render(<Badge variant="sector">Energy</Badge>);
const badge = container.firstChild as HTMLElement;

expect(badge).toHaveClass("bg-solar-100");
Expand All @@ -140,16 +112,9 @@ describe("Badge component", () => {
});

it("always includes base badge styling", () => {
// Testing that common styles are applied to all variants
const { container } = render(
<Badge
text="Test"
variant="pathwayType"
/>,
);
const { container } = render(<Badge variant="pathwayType">Test</Badge>);
const badge = container.firstChild as HTMLElement;

// Check for common styling classes that should be on all badges
expect(badge).toHaveClass("inline-flex");
expect(badge).toHaveClass("items-center");
expect(badge).toHaveClass("rounded-full");
Expand All @@ -161,44 +126,30 @@ describe("Badge component", () => {
});

it("renders as a span element", () => {
const { container } = render(<Badge text="Test" />);
const { container } = render(<Badge>Test</Badge>);
expect(container.firstChild?.nodeName).toBe("SPAN");
});

// Tooltip tests
it("does not render tooltip when no tooltip is provided", () => {
render(<Badge>No Tooltip</Badge>);

// Find the badge span
const badge = screen.getByText("No Tooltip");

// Check that it's a plain span without tabindex
expect(badge).toBeInTheDocument();
expect(badge).not.toHaveAttribute("tabindex");
});

it("uses TextWithTooltip when tooltip is provided", () => {
render(<Badge tooltip="This is a tooltip">With Tooltip</Badge>);

// Badge text should still be present
const badgeText = screen.getByText("With Tooltip");
expect(badgeText).toBeInTheDocument();

// The outer span from TextWithTooltip should have tabindex attribute
// We need to look for a parent element with tabindex since the badge text itself
// is wrapped in its own span
const triggerElement = badgeText.closest("span")?.parentElement;
expect(triggerElement).toHaveAttribute("tabindex", "0");

// The aria-describedby attribute is added when tooltip is visible
expect(triggerElement).not.toHaveAttribute("aria-describedby");
});

// Testing tooltip visibility requires checking document.body, since tooltips are now in portals
it("doesn't show tooltip initially", () => {
render(<Badge tooltip="Hover tooltip">Hover me</Badge>);

// Initially the tooltip shouldn't be in document.body
const tooltipElement = document.querySelector("[role='tooltip']");
expect(tooltipElement).not.toBeInTheDocument();
});
Expand Down Expand Up @@ -280,9 +231,11 @@ describe("Badge component", () => {

describe("BadgeMaybeAbsent", () => {
it("renders 'None' for undefined/null", () => {
const { rerender } = render(<BadgeMaybeAbsent text={undefined} />);
const { rerender } = render(
<BadgeMaybeAbsent>{undefined}</BadgeMaybeAbsent>,
);
expect(screen.getByText("None")).toBeInTheDocument();
rerender(<BadgeMaybeAbsent text={null} />);
rerender(<BadgeMaybeAbsent>{null}</BadgeMaybeAbsent>);
expect(screen.getByText("None")).toBeInTheDocument();
});

Expand All @@ -301,14 +254,13 @@ describe("BadgeMaybeAbsent", () => {
it("uses toLabel only for present values", () => {
const toLabel = (v: number) => `Y${v}`;
const { rerender } = render(
<BadgeMaybeAbsent<number> toLabel={toLabel}>2030</BadgeMaybeAbsent>,
<BadgeMaybeAbsent<number> toLabel={toLabel}>{2030}</BadgeMaybeAbsent>,
);
expect(screen.getByText("Y2030")).toBeInTheDocument();
rerender(
<BadgeMaybeAbsent<number>
text={undefined}
toLabel={toLabel}
/>,
<BadgeMaybeAbsent<number> toLabel={toLabel}>
{undefined}
</BadgeMaybeAbsent>,
);
expect(screen.getByText("None")).toBeInTheDocument();
});
Expand All @@ -317,17 +269,14 @@ describe("BadgeMaybeAbsent", () => {
// @ts-expect-error - BadgeMaybeAbsent children must be string | number | null | undefined
render(
<BadgeMaybeAbsent>
<span>Not allowed</span>
<div>Not allowed</div>
</BadgeMaybeAbsent>,
);
});

it("supports noneLabel override", () => {
render(
<BadgeMaybeAbsent
text={undefined}
noneLabel="No Value"
/>,
<BadgeMaybeAbsent noneLabel="No Value">{undefined}</BadgeMaybeAbsent>,
);
expect(screen.getByText("No Value")).toBeInTheDocument();
});
Expand Down
14 changes: 6 additions & 8 deletions src/components/Badge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ interface BadgeProps {
| "geographyGlobal"
| "geographyRegion"
| "geographyCountry"
| "sector";
| "sector"
| "metric"
| "keyFeature";
className?: string;
}

Expand Down Expand Up @@ -53,13 +55,10 @@ const Badge: React.FC<BadgeProps> = ({
? `${badgeStylesBase} ${className}`
: badgeStylesBase;

// If no tooltip, just return the basic badge
// If no tooltip, just return the basic badge
if (!tooltip) {
return <span className={badgeStyles}>{children}</span>;
}

// With tooltip, use the TextWithTooltip component
return (
<TextWithTooltip
text={<span className={badgeStyles}>{children}</span>}
Expand All @@ -70,7 +69,6 @@ const Badge: React.FC<BadgeProps> = ({
};

export default Badge;
// --- value-aware helpers (schema-agnostic) --------------------

export type BadgeMaybeAbsentProps<T extends string | number> = Omit<
React.ComponentProps<typeof Badge>,
Expand All @@ -83,7 +81,7 @@ export type BadgeMaybeAbsentProps<T extends string | number> = Omit<
/** Visible text for the "None" case (default "None"). */
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;
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
};

export function BadgeMaybeAbsent<T extends string | number>({
Expand All @@ -104,11 +102,11 @@ export function BadgeMaybeAbsent<T extends string | number>({
} else if (typeof children === "string" || typeof children === "number") {
base = String(children);
} else {
// Should be unreachable at runtime due to typing, but keep a safe fallback
base = String(children as unknown as string | number);
}

const content =
renderLabel && typeof base === "string" ? renderLabel(base, absent) : base;
return <Badge {...rest}>{content}</Badge>;

return <Badge {...rest}>{String(content)}</Badge>;
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
return <Badge {...rest}>{String(content)}</Badge>;
return <Badge {...rest}>{content}</Badge>;

Copilot uses AI. Check for mistakes.
}
Loading