fix: standardize icon sizing inside Badge component

[Tharsiga-21]

Description

Fixes inconsistent icon sizing inside the Badge component.

Followed by PR #976

Changes

• Added iconSizeClasses map to standardize icon sizing based on badge size
  • sm → w-3 h-3
  • md → w-4 h-4
• Wrapped icon in <span> with inline-flex items-center justify-center
• Added text-current so icon inherits badge text color automatically
• Added shrink-0 to prevent icon from being squeezed
• Added aria-hidden="true" since icon is decorative

Impact

• Icons now auto-size and color without consumer-side styling
• Consistent appearance across all badge variants and sizes

Closes #994

[github-actions]

Thank you @, for creating the PR and contributing to our UltimateHealth project 💗.
Our team will review the PR and will reach out to you soon! 😇
Make sure that you have marked all the tasks that you are done with ✅.
Thank you for your patience! 😀

[SB2318]

/review

[github-actions]

🤖 Gemini AI Code Review

Summary

This Pull Request aims to standardize icon sizing within the Badge component, ensuring icons automatically adjust their size and color based on the badge's properties. While the specific changes related to icon styling (e.g., iconSizeClasses, text-current, aria-hidden) are well-implemented and achieve the stated goal, the PR introduces significant breaking changes and regressions to the Badge component's API and functionality.

The PR effectively rewrites the Badge component, removing its ability to render as a button or a tag (via the as prop), eliminating support for children content, and no longer accepting a className prop for custom styling on the root element. These changes go far beyond "standardizing icon sizing" and represent a fundamental alteration of the component's public interface.

🔴 High Severity

• Issue: Major API Regression: Removal of as prop functionality.
  The original Badge component supported rendering as a button, a (anchor), or span via the as prop. This PR completely removes this functionality, hardcoding the component to always render as a <span> element.

• Impact: This is a breaking change that will cause runtime errors or unexpected behavior for any existing consumers of the Badge component that rely on it being an interactive element (button or link). It fundamentally changes the component's purpose and flexibility, making it impossible to create interactive badges.

• Fix:
  The Badge component needs to retain its as prop functionality. The icon standardization logic should be integrated within the existing structure that handles different as types. This likely means reintroducing the renderBadgeContent helper or similar logic, ensuring the icon styling is applied regardless of the root element type.

  // Reintroduce the as prop and its conditional rendering logic
  const Badge = (props: BadgeProps) => {
    const {
      as: Component = "span",
      label,
      children, // Reintroduce children prop
      variant = "default",
      size = "md",
      icon,
      className, // Reintroduce className prop
      ...restProps // Capture other props like type, href, onClick etc.
    } = props;
  
    const isInteractive = Component === "button" || Component === "a";
    const baseClasses = getBadgeClasses(variant, size, isInteractive, className);
  
    const iconElement = icon && (
      <span
        className={`inline-flex items-center justify-center shrink-0 text-current ${iconSizeClasses[size]}`}
        aria-hidden="true"
      >
        {icon}
      </span>
    );
  
    return (
      <Component className={baseClasses} {...restProps}>
        <span className="inline-flex items-center gap-1.5 rounded-full font-medium transition-colors">
          {iconElement}
          {label || children} {/* Support both label and children */}
        </span>
      </Component>
    );
  };

  (Note: The getBadgeClasses function would also need to be retained/re-evaluated based on the original implementation.)

• Issue: Major API Regression: Loss of children prop support.
  The original Badge component supported rendering content via its children prop. The new implementation only uses the label prop, effectively removing the children prop.

• Impact: This is another breaking change. Consumers who pass JSX elements or complex content as children to the Badge component will find their content no longer rendered. It reduces the flexibility of the component.

• Fix: The Badge component should continue to support the children prop, potentially rendering children if provided, otherwise falling back to label.

  // Inside the Badge component's render logic:
  // ...
  return (
    // ...
    <span className="inline-flex items-center gap-1.5 rounded-full font-medium transition-colors">
      {iconElement}
      {label || children} {/* Render children if present, else label */}
    </span>
    // ...
  );

• Issue: Dead Code: Unused interactiveClasses variable.
  The interactiveClasses constant is defined but no longer used anywhere in the Badge component's rendering logic.

• Impact: This adds unnecessary code, potentially confusing future maintainers about its purpose or if it was intended to be used. It also indicates an incomplete refactor where old logic was removed without cleaning up associated variables.

• Fix: Remove the interactiveClasses constant if the interactive functionality is truly being deprecated (which it shouldn't be, as per the first high-severity issue). If the as prop is reintroduced, this variable should be integrated back into the getBadgeClasses logic.

🟡 Medium Severity

• Issue: API Regression: Loss of className prop on root element.
  The original Badge component allowed consumers to pass a className prop to apply custom styles to the root badge element. The refactored component no longer explicitly accepts or applies this className to the outer <span>.

• Impact: This is a breaking change for consumers who rely on custom styling via className. It limits the component's extensibility and ability to integrate with external styling systems or conditional classes.

• Fix: The className prop should be reintroduced and applied to the root element of the Badge component, similar to how it was handled in the original getBadgeClasses function.

  // Ensure className is destructured and passed to the root element
  const Badge = (props: BadgeProps) => {
    const {
      // ... other props
      className,
      ...restProps
    } = props;
  
    // ...
    return (
      <Component
        className={getBadgeClasses(variant, size, isInteractive, className)} // Pass className here
        {...restProps}
      >
        {/* ... */}
      </Component>
    );
  };

🟢 Low Severity / Nits

• Issue: Implicit removal of getBadgeClasses usage.
  The getBadgeClasses function is no longer called in the new Badge component implementation. While not explicitly removed from the file, its usage has been deprecated by this PR.
• Impact: If getBadgeClasses is intended to be a shared utility, its non-use here might lead to inconsistencies if other components still use it, or it might become dead code if Badge was its sole consumer.
• Fix: If the as prop functionality is restored, getBadgeClasses should be reinstated to ensure consistent class application. If the intent was to remove getBadgeClasses entirely, it should be explicitly removed from the file.

What's Good ✅

1. Standardized Icon Sizing: The introduction of iconSizeClasses is a clean and effective way to manage icon dimensions based on badge size, centralizing styling logic.
2. Accessibility for Decorative Icons: Adding aria-hidden="true" to the icon wrapper is a good practice for accessibility, ensuring screen readers ignore purely decorative elements.
3. Inherited Text Color: Using text-current for the icon ensures it automatically inherits the badge's text color, simplifying styling and maintaining visual consistency.

Verdict

Request Changes

The PR introduces several high-severity breaking changes by fundamentally altering the Badge component's API, most critically by removing the as prop functionality. This goes far beyond the stated intent of standardizing icon sizing and would break existing implementations. The core functionality of the Badge component (e.g., being a button or link) must be preserved while integrating the icon styling improvements.

[SB2318]

The icon sizing improvements look good, but this PR also removes existing Badge functionality (as, children, className, button/anchor rendering). These are API-breaking changes unrelated to the stated scope of standardizing icon sizing. Please preserve the existing Badge API and integrate the icon sizing updates into the current implementation instead. Also remove or reuse the unused interactiveClasses variable.

[Tharsiga-21]

Hi @SB2318, thank you for the detailed review and merging the latest web branch into my PR! I have updated Badge.tsx to address all the feedback:

✅ Restored as prop for button/anchor/span rendering
✅ Added children prop support alongside label
✅ Added className prop for custom styling
✅ Spread ...restProps to support additional HTML attributes
✅ Moved interactiveClasses and getBadgeClasses outside the component
✅ Used getBadgeClasses for consistent class application
✅ Kept icon sizing improvements (iconSizeClasses, text-current, aria-hidden)

Please review the updated implementation!

[SB2318]

/review

[github-actions]

🤖 Gemini AI Code Review

Summary

This Pull Request aims to standardize icon sizing and styling within the Badge component, making it more consistent and easier to use by abstracting icon-related styles. The PR introduces an iconSizeClasses map, wraps the icon in a dedicated <span> with appropriate utility classes (inline-flex, shrink-0, text-current, aria-hidden), and refactors the component to consolidate rendering logic for span, button, and a elements.

While the refactoring significantly improves code maintainability and the icon styling is well-executed, there are critical regressions introduced in the component's core functionality and styling due to the consolidation.

🔴 High Severity

1. Missing Base Layout Styles for Most Badge Variants

• Issue: The previous implementation applied common layout styles (inline-flex items-center gap-1.5 rounded-full font-medium transition-colors) to an inner <span> that wrapped the badge content. In the refactored code, these styles are expected to be part of variantClasses or sizeClasses applied to the main Component element. However, variantClasses only includes these base layout styles for the outline and ghost variants. For default, destructive, success, warning, and info variants, these crucial styles are now missing.
• Impact: This is a significant visual regression. Badges using default, destructive, success, warning, or info variants will lose their flexbox layout, the gap between the icon and text, rounded corners, correct font weight, and transition effects. This fundamentally breaks the intended appearance of most badges.
• Fix: Introduce a baseClasses constant for these common layout styles and ensure getBadgeClasses always applies them, regardless of the variant.

--- a/src/components/ui/Badge.tsx
+++ b/src/components/ui/Badge.tsx
@@ -32,6 +32,10 @@ const variantClasses: Record<BadgeVariant, string> = {
   info: "bg-blue-100 text-blue-800 dark:bg-blue-900 dark:text-blue-200",
 };
 
+const baseClasses =
+  "inline-flex items-center gap-1.5 rounded-full font-medium transition-colors";
+
 const sizeClasses: Record<BadgeSize, string> = {
   sm: "px-2.5 py-0.5 text-xs",
   md: "px-3.5 py-1 text-sm",
@@ -50,7 +54,7 @@ const getBadgeClasses = (
   className?: string
 ) =>
   [
-    variantClasses[variant],
+    baseClasses, // Apply common layout styles first
+    variantClasses[variant], // Then apply variant-specific styles
     sizeClasses[size],
     isInteractive && interactiveClasses,
     className,

2. Missing type="button" for as="button" Component

• Issue: When props.as is "button", the type attribute is no longer explicitly set to "button". HTML <button> elements default to type="submit" if not specified.
• Impact: If a badge with as="button" is used inside a <form> element, clicking it will trigger a form submission, which is likely an unintended side effect for a decorative or informational badge. This can lead to unexpected user experience and data handling issues.
• Fix: Explicitly set type="button" when Component is "button", unless a type prop is already provided by the consumer.

--- a/src/components/ui/Badge.tsx
+++ b/src/components/ui/Badge.tsx
@@ -133,16 +75,26 @@ const Badge = (props: BadgeProps) => {
     icon,
-    className,
-    ...spanProps
-  } = props;
+    className = "",
+    ...restProps
+  } = props as BadgeSpanProps & BadgeButtonProps & BadgeAnchorProps;
+
+  const isInteractive = Component === "button" || Component === "a";
+  const isButton = Component === "button";
+
+  // Ensure type="button" for buttons if not explicitly provided
+  const finalProps = { ...restProps };
+  if (isButton && !finalProps.type) {
+    finalProps.type = "button";
+  }
 
   return (
     <Component
-      className={getBadgeClasses(variant, size, false, className)}
-      {...spanProps}
+      className={getBadgeClasses(variant, size, isInteractive, className)}
+      {...finalProps}
     >
       {icon && (
         <span
           className={`inline-flex items-center justify-center shrink-0 text-current ${iconSizeClasses[size]}`}
           aria-hidden="true"
         >
           {icon}
         </span>
       )}
       {children ?? label}
     </Component>
   );
 };

🟡 Medium Severity

1. Loose Type Casting for BadgeProps

• Issue: The component uses props as BadgeSpanProps & BadgeButtonProps & BadgeAnchorProps to cast the incoming props. This forces TypeScript to believe that props has all properties from all three types simultaneously, which is incorrect. A Badge component can only be one of these types at a time (e.g., it cannot have both href and type="button"). This bypasses TypeScript's type safety and can lead to runtime errors if incorrect props are passed (e.g., href on a button component).
• Impact: Reduced type safety, potential for runtime errors, and makes the component harder to maintain and debug as its API evolves. Consumers might pass invalid props without TypeScript catching it.
• Fix: Refactor BadgeProps to use a discriminated union. This allows TypeScript to correctly infer the available props based on the as prop, providing strong type safety.

// Example of how BadgeProps should be structured (simplified for brevity)
type BadgeBaseProps = {
  label?: string;
  children?: React.ReactNode;
  variant?: BadgeVariant;
  size?: BadgeSize;
  icon?: React.ReactNode;
  className?: string;
};

type BadgeSpanProps = BadgeBaseProps & {
  as?: "span"; // 'as' is optional for span, defaults to span
};

type BadgeButtonProps = BadgeBaseProps &
  React.ComponentPropsWithoutRef<"button"> & {
    as: "button"; // 'as' is required for button
  };

type BadgeAnchorProps = BadgeBaseProps &
  React.ComponentPropsWithoutRef<"a"> & {
    as: "a"; // 'as' is required for anchor
  };

export type BadgeProps = BadgeSpanProps | BadgeButtonProps | BadgeAnchorProps;

// Then in the component, no 'as' cast is needed:
const Badge = (props: BadgeProps) => {
  const {
    as: Component = "span",
    // ... rest of props
  } = props; // TypeScript will correctly infer types based on 'as'
  // ...
};

🟢 Low Severity / Nits

1. Explicit type in BadgeButtonProps Interface

• Issue: While the runtime fix for type="button" is crucial, it's good practice to reflect this in the component's type definition.
• Fix: If you implement the discriminated union for BadgeProps, ensure BadgeButtonProps explicitly includes type?: "button" | "submit" | "reset"; to guide consumers and provide better type hints.

2. className Default Value

• Issue: Setting className = "" as a default is generally fine.
• Note: The getBadgeClasses function uses filter(Boolean) which gracefully handles undefined or null values passed for className, so this is robust. No change needed, just an observation.

What's Good ✅

1. Excellent Refactoring for Consolidation: The decision to consolidate the rendering logic for span, button, and a into a single Badge component is a significant improvement for maintainability, reducing code duplication and making future enhancements easier.
2. Standardized Icon Sizing: The introduction of iconSizeClasses is a clean and effective way to standardize icon dimensions based on badge size, ensuring visual consistency across the component.
3. Improved Icon Styling and Accessibility: Wrapping the icon with inline-flex items-center justify-center shrink-0 text-current and adding aria-hidden="true" demonstrates a thorough understanding of styling best practices and accessibility for decorative elements.

Verdict

Request Changes

The PR introduces critical regressions in the badge's visual layout and interactive behavior (type="button"), which need to be addressed before merging. The most critical issue is the missing base layout styles for most badge variants, which will cause a significant visual break.

[SB2318]

Thanks for your contribution!

[github-actions]

Congratulations, Your pull request has been successfully merged 🥳🎉 Thank you for your contribution to the project 🚀 Keep Contributing!! ✨