From 45af7db4d4e93f6af4a244dda2f62ac7a6a697d8 Mon Sep 17 00:00:00 2001
From: Jules Belveze <32683010+JulesBelveze@users.noreply.github.com>
Date: Fri, 22 Nov 2024 11:44:15 +0100
Subject: [PATCH] [sparkle] - refacto: Checkbox / RadioGroup (#8752)

* [sparkle/src/components] - refactor: centralize Checkbox size styles

- Extract size variant mapping from the Checkbox component into a separate constant for better maintainability
- Replace inlined size variant definitions in the component with the new centralized `checkboxSizeVariant` mapping

[sparkle/src/stories] - feature: update Checkbox stories with enhanced controls and structure

- Introduce controls for size, checked state, and disabled state with descriptions and default values in the Checkbox story
- Add `text` and `description` properties with conditional rendering logic for more comprehensive story examples
- Implement action handlers for onChange events in Checkbox stories to better demonstrate component interaction

* [sparkle] - feature: integrate labels directly into RadioGroupItems

 - Implemented direct label support within the RadioGroupItem component for better encapsulation
 - Updated RadioGroupItem usage in stories to leverage the new label prop, simplifying layout
 - Enhanced RadioGroupItem styling to accommodate label inclusion
 - Adjusted RadioGroupChoice to a flex-column layout to align with new item structure

* [sparkle] - refactor: improve readability and syntax conformity in RadioGroup components

 - Reorder imports to maintain consistent structure across files
 - Add missing semicolons for code consistency
 - Reformat props spreading for better readability
 - Simplify JSX structure by removing redundant divs and merging classNames
 - Adjust stories for RadioGroup to provide clearer examples with proper htmlFor attributes

* [sparkle] - refactor: simplify label prop usage in RadioGroup components

 - Refactor RadioGroup stories to use string labels directly instead of wrapping them with a Label component

* [sparkle] - refactor: update `RadioGroup` to support ReactNode labels

 - Removed `Label` component import, allowing more flexible `label` prop types in `RadioGroupItem`
 - Replaced static text labels with JSX labels in `RadioGroup.stories`
 - Simplified the wrapper div class logic for RadioGroupItem components
 - Enhanced `RadioGroupWithChildrenExample` with additional icon and style adjustments

* [sparkle] - fix: ensure RadioGroup items use full width

 - Added a class to make RadioGroup items use the full available width for better layout control

* [sparkle] - feature: bump package version to 0.2.322

 - Increment package version for a new release with latest changes and fixes
---
 sparkle/package-lock.json                  |   4 +-
 sparkle/package.json                       |   2 +-
 sparkle/src/components/Checkbox.tsx        |  13 +-
 sparkle/src/components/RadioGroup.tsx      |  25 +++-
 sparkle/src/stories/Checkbox.stories.tsx   | 144 +++++++++++++--------
 sparkle/src/stories/RadioGroup.stories.tsx |  47 +++++--
 6 files changed, 152 insertions(+), 83 deletions(-)

diff --git a/sparkle/package-lock.json b/sparkle/package-lock.json
index e6defe726daf..c79a5af5ba6b 100644
--- a/sparkle/package-lock.json
+++ b/sparkle/package-lock.json
@@ -1,12 +1,12 @@
 {
   "name": "@dust-tt/sparkle",
-  "version": "0.2.321",
+  "version": "0.2.322",
   "lockfileVersion": 3,
   "requires": true,
   "packages": {
     "": {
       "name": "@dust-tt/sparkle",
-      "version": "0.2.321",
+      "version": "0.2.322",
       "license": "ISC",
       "dependencies": {
         "@emoji-mart/data": "^1.1.2",
diff --git a/sparkle/package.json b/sparkle/package.json
index 1c255aa971a2..4fb419a2ee86 100644
--- a/sparkle/package.json
+++ b/sparkle/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@dust-tt/sparkle",
-  "version": "0.2.321",
+  "version": "0.2.322",
   "scripts": {
     "build": "rm -rf dist && npm run tailwind && npm run build:esm && npm run build:cjs",
     "tailwind": "tailwindcss -i ./src/styles/tailwind.css -o dist/sparkle.css",
diff --git a/sparkle/src/components/Checkbox.tsx b/sparkle/src/components/Checkbox.tsx
index d82e996d0a6e..75eb1ae59ae7 100644
--- a/sparkle/src/components/Checkbox.tsx
+++ b/sparkle/src/components/Checkbox.tsx
@@ -8,6 +8,14 @@ import { cn } from "@sparkle/lib/utils";
 import { Icon } from "./Icon";
 import { Label } from "./Label";
 
+export const CHECKBOX_SIZES = ["xs", "sm"] as const;
+type CheckboxSizeType = (typeof CHECKBOX_SIZES)[number];
+
+const checkboxSizeVariant: Record<CheckboxSizeType, string> = {
+  xs: "s-h-4 s-w-4 s-rounded",
+  sm: "s-h-5 s-w-5 s-rounded-md",
+};
+
 const checkboxStyles = cva(
   cn(
     "s-shrink-0 s-peer s-border s-text-foreground s-border-border-darker",
@@ -22,10 +30,7 @@ const checkboxStyles = cva(
         partial: "data-[state=checked]:s-bg-muted-foreground",
         false: "",
       },
-      size: {
-        xs: "s-h-4 s-w-4 s-rounded",
-        sm: "s-h-5 s-w-5 s-rounded-md",
-      },
+      size: checkboxSizeVariant,
     },
     defaultVariants: {
       size: "sm",
diff --git a/sparkle/src/components/RadioGroup.tsx b/sparkle/src/components/RadioGroup.tsx
index f2d1512b85d8..23edb9702fd4 100644
--- a/sparkle/src/components/RadioGroup.tsx
+++ b/sparkle/src/components/RadioGroup.tsx
@@ -60,6 +60,7 @@ interface RadioGroupItemProps
     VariantProps<typeof radioStyles> {
   tooltipMessage?: string;
   tooltipAsChild?: boolean;
+  label?: React.ReactNode;
 }
 
 const RadioGroupItem = React.forwardRef<
@@ -67,7 +68,14 @@ const RadioGroupItem = React.forwardRef<
   RadioGroupItemProps
 >(
   (
-    { tooltipMessage, className, size, tooltipAsChild = false, ...props },
+    {
+      tooltipMessage,
+      className,
+      size,
+      tooltipAsChild = false,
+      label,
+      ...props
+    },
     ref
   ) => {
     const item = (
@@ -81,10 +89,9 @@ const RadioGroupItem = React.forwardRef<
         />
       </RadioGroupPrimitive.Item>
     );
-    return (
-      <div
-        className={cn("s-group", size === "sm" ? "s-h-5 s-w-5" : "s-h-4 s-w-4")}
-      >
+
+    const wrappedItem = (
+      <div className="s-flex s-items-center s-gap-2">
         {tooltipMessage ? (
           <Tooltip
             triggerAsChild={tooltipAsChild}
@@ -94,11 +101,13 @@ const RadioGroupItem = React.forwardRef<
         ) : (
           item
         )}
+        {label}
       </div>
     );
+
+    return <div className="s-w-full s-group">{wrappedItem}</div>;
   }
 );
-RadioGroupItem.displayName = RadioGroupPrimitive.Item.displayName;
 
 type IconPosition = "start" | "center" | "end";
 
@@ -111,7 +120,9 @@ const RadioGroupChoice = React.forwardRef<
   RadioGroupChoiceProps
 >(({ className, size, iconPosition = "center", children, ...props }, ref) => {
   return (
-    <div className={cn("s-flex", className, `s-items-${iconPosition}`)}>
+    <div
+      className={cn("s-flex s-flex-col", className, `s-items-${iconPosition}`)}
+    >
       <RadioGroupItem
         ref={ref}
         className={cn(radioStyles({ size }))}
diff --git a/sparkle/src/stories/Checkbox.stories.tsx b/sparkle/src/stories/Checkbox.stories.tsx
index 2bba4452247b..54a02f71480c 100644
--- a/sparkle/src/stories/Checkbox.stories.tsx
+++ b/sparkle/src/stories/Checkbox.stories.tsx
@@ -1,72 +1,102 @@
-import type { Meta } from "@storybook/react";
+import type { Meta, StoryObj } from "@storybook/react";
 import React from "react";
 
+import { CHECKBOX_SIZES } from "@sparkle/components/Checkbox";
+
 import {
   Checkbox,
+  type CheckboxProps,
   CheckboxWithText,
   CheckBoxWithTextAndDescription,
 } from "../index_with_tw_base";
 
+const CHECKED_STATES = {
+  unchecked: false,
+  checked: true,
+  partial: "partial",
+} as const;
+
+type ExtendedCheckboxProps = CheckboxProps & {
+  text?: string;
+  description?: string;
+};
+
 const meta = {
   title: "Primitives/Checkbox",
-  component: Checkbox,
-} satisfies Meta<typeof Checkbox>;
+  // We need to cast here as the component expects stricter props
+  component: Checkbox as React.ComponentType<ExtendedCheckboxProps>,
+  parameters: {
+    layout: "centered",
+  },
+  argTypes: {
+    size: {
+      description: "The size of the checkbox",
+      options: CHECKBOX_SIZES,
+      control: { type: "select" },
+      table: {
+        defaultValue: { summary: "sm" },
+      },
+    },
+    checked: {
+      description: "The checked state of the checkbox",
+      options: Object.keys(CHECKED_STATES),
+      mapping: CHECKED_STATES,
+      control: { type: "select" },
+      table: {
+        type: { summary: "boolean | 'partial'" },
+        defaultValue: { summary: "false" },
+      },
+    },
+    disabled: {
+      description: "Whether the checkbox is disabled",
+      control: "boolean",
+      table: {
+        defaultValue: { summary: false },
+      },
+    },
+    className: {
+      description: "Additional CSS classes to apply",
+      control: "text",
+    },
+    text: {
+      description: "Optional text label to display next to the checkbox",
+      control: "text",
+    },
+    description: {
+      description:
+        "Optional description text (only shown when text is provided)",
+      control: "text",
+      if: { arg: "text" },
+    },
+    onChange: {
+      description: "Callback when checkbox state changes",
+      action: "changed",
+    },
+  },
+} satisfies Meta<ExtendedCheckboxProps>;
 
 export default meta;
+type Story = StoryObj<typeof meta>;
 
-const handleChange = () => {
-  // This function intentionally left blank
-};
-
-export const CheckBoxSizesExample = () => {
-  return (
-    <div className="s-flex s-flex-col s-gap-10">
-      <div className="s-flex s-gap-10">
-        SM
-        <Checkbox onChange={handleChange} />
-        <Checkbox disabled onChange={handleChange} />
-        <Checkbox checked onChange={handleChange} />
-        <Checkbox checked disabled onChange={handleChange} />
-        <Checkbox checked="partial" onChange={handleChange} />
-        <Checkbox checked="partial" disabled onChange={handleChange} />
-      </div>
-      <div className="s-flex s-gap-10">
-        XS
-        <Checkbox size="xs" onChange={handleChange} />
-        <Checkbox size="xs" disabled onChange={handleChange} />
-        <Checkbox size="xs" checked onChange={handleChange} />
-        <Checkbox size="xs" checked disabled onChange={handleChange} />
-        <Checkbox size="xs" checked="partial" onChange={handleChange} />
-        <Checkbox
-          size="xs"
-          checked="partial"
-          disabled
-          onChange={handleChange}
+export const Default: Story = {
+  args: {
+    size: "sm",
+    checked: false,
+    disabled: false,
+  },
+  render: ({ text, description, ...args }) => {
+    if (text && description) {
+      return (
+        <CheckBoxWithTextAndDescription
+          text={text}
+          description={description}
+          {...args}
         />
-      </div>
-    </div>
-  );
-};
-
-export const CheckBoxWithTextExample = () => {
-  return (
-    <div className="s-flex s-gap-10">
-      <CheckboxWithText text="Google Drive" />
-    </div>
-  );
-};
-
-export const CheckBoxWithTextAndDescriptionExample = () => {
-  return (
-    <div className="s-flex s-flex-col s-gap-3">
-      <CheckBoxWithTextAndDescription
-        text="Google Drive"
-        description="This is a nice Google Drive description."
-      />
-      <CheckBoxWithTextAndDescription
-        text="Microsoft"
-        description="This is a nice Microsoft description."
-      />
-    </div>
-  );
+      );
+    }
+    if (text) {
+      return <CheckboxWithText text={text} {...args} />;
+    }
+    return <Checkbox {...args} />;
+  },
 };
diff --git a/sparkle/src/stories/RadioGroup.stories.tsx b/sparkle/src/stories/RadioGroup.stories.tsx
index 6b48026d6b90..8808892acd38 100644
--- a/sparkle/src/stories/RadioGroup.stories.tsx
+++ b/sparkle/src/stories/RadioGroup.stories.tsx
@@ -3,6 +3,7 @@ import React from "react";
 
 import {
   Button,
+  FolderIcon,
   Icon,
   Label,
   LockIcon,
@@ -21,16 +22,25 @@ export const RadioGroupExample = () => {
     <div className="s-flex s-flex-col s-gap-10">
       <RadioGroup defaultValue="option-one">
         <div className="s-flex s-items-center s-space-x-2">
-          <RadioGroupItem value="option-one" id="option-one" />
-          <Label htmlFor="option-one">Option One</Label>
+          <RadioGroupItem
+            value="option-one"
+            id="option-one"
+            label={<Label htmlFor="option-one">Option One</Label>}
+          />
         </div>
         <div className="s-flex s-items-center s-space-x-2">
-          <RadioGroupItem value="option-two" id="option-two" />
-          <Label htmlFor="option-two">Option Two</Label>
+          <RadioGroupItem
+            value="option-two"
+            id="option-two"
+            label={<Label htmlFor="option-two">Option Two</Label>}
+          />
         </div>
         <div className="s-flex s-items-center s-space-x-2">
-          <RadioGroupItem value="option-three" id="option-three" />
-          <Label htmlFor="option-two">Option Three</Label>
+          <RadioGroupItem
+            value="option-three"
+            id="option-three"
+            label={<Label htmlFor="option-two">Option Three</Label>}
+          />
         </div>
       </RadioGroup>
       <RadioGroup defaultValue="option-one">
@@ -40,8 +50,8 @@ export const RadioGroupExample = () => {
             id="option-four"
             size="sm"
             tooltipMessage="This is a nice tooltip message"
+            label={<Label htmlFor="option-one">Option One</Label>}
           />
-          <Label htmlFor="option-one">Option One</Label>
         </div>
         <div className="s-flex s-items-center s-space-x-2">
           <RadioGroupItem
@@ -49,12 +59,16 @@ export const RadioGroupExample = () => {
             id="option-five"
             size="sm"
             disabled
+            label={<Label htmlFor="option-two">Option Two</Label>}
           />
-          <Label htmlFor="option-two">Option Two</Label>
         </div>
         <div className="s-flex s-items-center s-space-x-2">
-          <RadioGroupItem value="option-six" id="option-three" size="sm" />
-          <Label htmlFor="option-three">Option Three</Label>
+          <RadioGroupItem
+            value="option-six"
+            id="option-three"
+            size="sm"
+            label={<Label htmlFor="option-three">Option Three</Label>}
+          />
         </div>
       </RadioGroup>
     </div>
@@ -77,9 +91,18 @@ export const RadioGroupWithChildrenExample = () => {
         onValueChange={(value) => setSelectedChoice(value)}
       >
         {choices.map((choice) => (
-          <RadioGroupChoice value={choice.id} iconPosition="start">
+          <RadioGroupChoice
+            value={choice.id}
+            iconPosition="start"
+            label={
+              <div className="s-flex s-items-center s-gap-2">
+                <Icon visual={LockIcon} />
+                <Label>{choice.label}</Label>
+              </div>
+            }
+          >
             <div className="s-flex s-items-center s-gap-2 s-border s-border-red-500 s-p-2">
-              <Icon visual={LockIcon} />
+              <Icon visual={FolderIcon} />
               <Label>{choice.label}</Label>
               <Button label="Click me" />
             </div>