Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Tag component based on the Timeless Figma design. The component supports different sizes, variants, icons, and both controlled and uncontrolled visibility states with removal functionality.
Changes:
- New Tag component with controlled/uncontrolled visibility pattern
- Support for prefix icons, customizable suffix icons, and multiple size/variant options
- Comprehensive test suite covering removal behavior and disabled states
- Storybook documentation with examples for different variants and use cases
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/frappe-ui-react/src/components/tag/types.ts | TypeScript interface defining Tag component props |
| packages/frappe-ui-react/src/components/tag/tag.tsx | Main Tag component implementation wrapping Button with custom styling |
| packages/frappe-ui-react/src/components/tag/tests/tag.tsx | Test suite covering rendering, removal, disabled state, and controlled behavior |
| packages/frappe-ui-react/src/components/tag/tag.stories.tsx | Storybook stories demonstrating different Tag configurations |
| packages/frappe-ui-react/src/components/tag/index.ts | Component exports |
| packages/frappe-ui-react/src/components/index.ts | Registration of Tag in main component exports |
| prefixIcon?: React.ComponentType<any>; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| suffixIcon?: React.ComponentType<any>; |
There was a problem hiding this comment.
The icon prop types use React.ComponentType<any> which is less type-safe than the pattern used in Button component. The Button component uses React.ComponentType<unknown> for its icon props, which is more restrictive and type-safe.
Consider changing from React.ComponentType<any> to React.ComponentType<unknown> to align with the Button component's type safety pattern (packages/frappe-ui-react/src/components/button/types.ts:11-13).
| </button> | ||
| )} | ||
| className={clsx( | ||
| "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!", |
There was a problem hiding this comment.
The focus styles are hardcoded to gray color scheme (focus:border-gray-900, focus:ring-outline-gray-3) regardless of the variant or potential theme prop. This creates visual inconsistency when using non-gray variants.
The Button component has theme-specific focus classes (packages/frappe-ui-react/src/components/button/button.tsx:60-65). Consider using variant-aware focus styles or ensuring focus styles match the tag's visual appearance.
| "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!", | |
| "focus:border-current focus:outline-none focus:ring-2 focus:ring-current gap-1.25! cursor-auto!", |
| describe("Tag Component", () => { | ||
| it("renders with label", () => { | ||
| render(<Tag label="Test Tag" />); | ||
| expect(screen.getByText("Test Tag")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("renders prefix icon", () => { | ||
| const PrefixIcon = () => <span data-testid="prefix-icon" />; | ||
| render(<Tag label="Tag with Icon" prefixIcon={PrefixIcon} />); | ||
| expect(screen.getByTestId("prefix-icon")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("handles uncontrolled removal", () => { | ||
| render(<Tag label="Removable Tag" />); | ||
| // Expect the label to be present | ||
| expect(screen.getByText("Removable Tag")).toBeInTheDocument(); | ||
|
|
||
| const removeButton = screen.getByLabelText("Remove tag"); | ||
| fireEvent.click(removeButton); | ||
|
|
||
| // After click, component should return null | ||
| expect(screen.queryByText("Removable Tag")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("handles controlled removal", () => { | ||
| const handleVisibleChange = jest.fn(); | ||
| const handleRemove = jest.fn(); | ||
|
|
||
| const { rerender } = render( | ||
| <Tag | ||
| label="Controlled Tag" | ||
| visible={true} | ||
| onVisibleChange={handleVisibleChange} | ||
| onRemove={handleRemove} | ||
| /> | ||
| ); | ||
|
|
||
| const removeButton = screen.getByLabelText("Remove tag"); | ||
| fireEvent.click(removeButton); | ||
|
|
||
| expect(handleVisibleChange).toHaveBeenCalledWith(false); | ||
| expect(handleRemove).toHaveBeenCalled(); | ||
|
|
||
| rerender( | ||
| <Tag | ||
| label="Controlled Tag" | ||
| visible={false} | ||
| onVisibleChange={handleVisibleChange} | ||
| onRemove={handleRemove} | ||
| /> | ||
| ); | ||
| expect(screen.queryByText("Controlled Tag")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("respects disabled state", () => { | ||
| const handleRemove = jest.fn(); | ||
| render(<Tag label="Disabled Tag" disabled onRemove={handleRemove} />); | ||
|
|
||
| const removeButton = screen.getByLabelText("Remove tag"); | ||
| expect(removeButton).toBeDisabled(); | ||
|
|
||
| fireEvent.click(removeButton); | ||
| expect(handleRemove).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("renders with custom class name", () => { | ||
| render(<Tag label="Custom Class" className="my-custom-class" />); | ||
| const tagText = screen.getByText("Custom Class"); | ||
| // The button that contains the text | ||
| const button = tagText.closest("button"); | ||
| expect(button).toHaveClass("my-custom-class"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite is missing coverage for the suffixIcon prop. While the default X icon is tested implicitly through the remove functionality, there's no test verifying that a custom suffix icon can be provided and rendered correctly.
Add a test case that provides a custom suffixIcon component and verifies it renders instead of the default X icon.
| describe("Tag Component", () => { | ||
| it("renders with label", () => { | ||
| render(<Tag label="Test Tag" />); | ||
| expect(screen.getByText("Test Tag")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("renders prefix icon", () => { | ||
| const PrefixIcon = () => <span data-testid="prefix-icon" />; | ||
| render(<Tag label="Tag with Icon" prefixIcon={PrefixIcon} />); | ||
| expect(screen.getByTestId("prefix-icon")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("handles uncontrolled removal", () => { | ||
| render(<Tag label="Removable Tag" />); | ||
| // Expect the label to be present | ||
| expect(screen.getByText("Removable Tag")).toBeInTheDocument(); | ||
|
|
||
| const removeButton = screen.getByLabelText("Remove tag"); | ||
| fireEvent.click(removeButton); | ||
|
|
||
| // After click, component should return null | ||
| expect(screen.queryByText("Removable Tag")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("handles controlled removal", () => { | ||
| const handleVisibleChange = jest.fn(); | ||
| const handleRemove = jest.fn(); | ||
|
|
||
| const { rerender } = render( | ||
| <Tag | ||
| label="Controlled Tag" | ||
| visible={true} | ||
| onVisibleChange={handleVisibleChange} | ||
| onRemove={handleRemove} | ||
| /> | ||
| ); | ||
|
|
||
| const removeButton = screen.getByLabelText("Remove tag"); | ||
| fireEvent.click(removeButton); | ||
|
|
||
| expect(handleVisibleChange).toHaveBeenCalledWith(false); | ||
| expect(handleRemove).toHaveBeenCalled(); | ||
|
|
||
| rerender( | ||
| <Tag | ||
| label="Controlled Tag" | ||
| visible={false} | ||
| onVisibleChange={handleVisibleChange} | ||
| onRemove={handleRemove} | ||
| /> | ||
| ); | ||
| expect(screen.queryByText("Controlled Tag")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("respects disabled state", () => { | ||
| const handleRemove = jest.fn(); | ||
| render(<Tag label="Disabled Tag" disabled onRemove={handleRemove} />); | ||
|
|
||
| const removeButton = screen.getByLabelText("Remove tag"); | ||
| expect(removeButton).toBeDisabled(); | ||
|
|
||
| fireEvent.click(removeButton); | ||
| expect(handleRemove).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("renders with custom class name", () => { | ||
| render(<Tag label="Custom Class" className="my-custom-class" />); | ||
| const tagText = screen.getByText("Custom Class"); | ||
| // The button that contains the text | ||
| const button = tagText.closest("button"); | ||
| expect(button).toHaveClass("my-custom-class"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite doesn't verify that different size and variant props are applied correctly. While the stories demonstrate different sizes and variants visually, there are no automated tests ensuring these props result in the correct CSS classes being applied.
Add test cases for different sizes (sm, md, lg) and variants (solid, subtle, outline, ghost) to verify the component renders with the expected classes, similar to the Badge component tests (packages/frappe-ui-react/src/components/badge/tests/badge.tsx:29-52).
| import type { Meta, StoryObj } from "@storybook/react-vite"; | ||
| import { Plus } from "lucide-react"; | ||
|
|
||
| import Tag from "./tag"; | ||
| import type { TagProps } from "./types"; | ||
|
|
||
| export default { | ||
| title: "Components/Tag", | ||
| component: Tag, | ||
| tags: ["autodocs"], | ||
| argTypes: { | ||
| className: { | ||
| control: "text", | ||
| description: "Additional CSS classes for the tag", | ||
| }, | ||
| size: { | ||
| control: { type: "select", options: ["sm", "md", "lg"] }, | ||
| description: "Size of the tag", | ||
| }, | ||
| variant: { | ||
| control: { | ||
| type: "select", | ||
| options: ["solid", "subtle", "outline", "ghost"], | ||
| }, | ||
| description: "Variant style of the tag", | ||
| }, | ||
| label: { | ||
| control: "text", | ||
| description: "Text label displayed inside the tag", | ||
| }, | ||
| disabled: { | ||
| control: "boolean", | ||
| description: "Disables the tag when set to true", | ||
| }, | ||
| prefixIcon: { | ||
| control: false, | ||
| description: "Icon component displayed before the label", | ||
| }, | ||
| suffixIcon: { | ||
| control: false, | ||
| description: "Icon component displayed after the label", | ||
| }, | ||
| visible: { | ||
| control: "boolean", | ||
| description: "Controls the visibility of the tag (controlled mode)", | ||
| }, | ||
| onVisibleChange: { | ||
| action: "visibility changed", | ||
| description: "Callback function when the visibility of the tag changes", | ||
| }, | ||
| onRemove: { | ||
| action: "removed", | ||
| description: "Callback function when the remove icon is clicked", | ||
| }, | ||
| }, | ||
| parameters: { docs: { source: { type: "dynamic" } }, layout: "centered" }, | ||
| } as Meta<typeof Tag>; | ||
|
|
||
| type Story = StoryObj<TagProps>; | ||
|
|
||
| export const Default: Story = { | ||
| args: { | ||
| size: "sm", | ||
| variant: "solid", | ||
| label: "Discover", | ||
| }, | ||
| render: (args) => ( | ||
| <div className="min-h-20 flex justify-center items-center"> | ||
| <Tag {...args} /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const Subtle: Story = { | ||
| args: { | ||
| size: "sm", | ||
| variant: "subtle", | ||
| label: "Discover", | ||
| }, | ||
| render: (args) => ( | ||
| <div className="min-h-20 flex justify-center items-center"> | ||
| <Tag {...args} /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const Outline: Story = { | ||
| args: { | ||
| size: "sm", | ||
| variant: "outline", | ||
| label: "Discover", | ||
| }, | ||
| render: (args) => ( | ||
| <div className="min-h-20 flex justify-center items-center"> | ||
| <Tag {...args} /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const Ghost: Story = { | ||
| args: { | ||
| size: "sm", | ||
| variant: "ghost", | ||
| label: "Discover", | ||
| }, | ||
| render: (args) => ( | ||
| <div className="min-h-20 flex justify-center items-center"> | ||
| <Tag {...args} /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const Disabled: Story = { | ||
| args: { | ||
| size: "md", | ||
| variant: "solid", | ||
| label: "Discover", | ||
| disabled: true, | ||
| }, | ||
| render: (args) => ( | ||
| <div className="min-h-20 flex justify-center items-center"> | ||
| <Tag {...args} /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const WithPrefix: Story = { | ||
| args: { | ||
| size: "md", | ||
| variant: "solid", | ||
| label: "Mobile", | ||
| prefixIcon: () => <Plus className="w-3 h-3" />, | ||
| }, | ||
| render: (args) => ( | ||
| <div className="min-h-20 flex justify-center items-center"> | ||
| <Tag {...args} /> | ||
| </div> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
The stories don't include an example of the controlled mode (using the visible and onVisibleChange props). While the tests verify controlled behavior, a story demonstrating this pattern would be helpful for documentation and visual verification.
Consider adding a story that shows controlled tag removal, similar to how the tests demonstrate it (packages/frappe-ui-react/src/components/tag/tests/tag.tsx:29-57).
| </button> | ||
| )} | ||
| className={clsx( | ||
| "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!", |
There was a problem hiding this comment.
The Tag applies cursor-auto! to make the main tag area non-clickable, which is correct for a tag component. However, this conflicts with the Button component's default cursor styling and may create confusion.
Since the Tag's main body shouldn't be clickable (only the remove button should be), consider whether wrapping a Button component is the right approach. A more semantic implementation might use a span or div with button-like styling for the tag body, and a separate button only for the remove action. This would also resolve the nested button accessibility issue.
| <Button | ||
| size={size} | ||
| variant={variant} | ||
| label={label} | ||
| iconLeft={prefixIcon} | ||
| iconRight={() => ( | ||
| <button | ||
| type="button" | ||
| className={clsx( | ||
| "flex items-center justify-center", | ||
| disabled && "cursor-auto!" | ||
| )} | ||
| onClick={handleRemove} | ||
| disabled={disabled} | ||
| aria-label="Remove tag" | ||
| > | ||
| <SuffixIcon | ||
| className="w-3 h-3" | ||
| aria-hidden="true" | ||
| focusable="false" | ||
| /> | ||
| </button> | ||
| )} | ||
| className={clsx( | ||
| "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!", | ||
| size === "sm" && "text-xs! h-5! rounded-[5px]! px-1.5! py-0.75!", | ||
| size === "md" && "text-sm! h-6! rounded-[6px]! px-1.5! py-1!", | ||
| size === "lg" && "text-base! h-7! rounded-[8px]! px-2! py-1.5!", | ||
| className | ||
| )} | ||
| disabled={disabled} | ||
| /> |
There was a problem hiding this comment.
The current implementation creates nested buttons, which is invalid HTML and causes accessibility issues. The Tag wraps a Button component and passes a button element as iconRight, resulting in a button nested inside another button.
Consider one of these alternatives:
- Render the Tag as a div or span container with separate button-like styling, instead of wrapping an actual Button component
- Pass a clickable icon/div to iconRight instead of a button element
- Restructure the component to not use Button as a wrapper
The nested button structure will fail HTML validation and can cause unpredictable behavior with screen readers and keyboard navigation.
| export interface TagProps { | ||
| size?: "sm" | "md" | "lg"; | ||
| variant?: ButtonVariant; | ||
| label?: string; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| prefixIcon?: React.ComponentType<any>; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| suffixIcon?: React.ComponentType<any>; | ||
| className?: string; | ||
| disabled?: boolean; | ||
| visible?: boolean; | ||
| onVisibleChange?: (visible: boolean) => void; | ||
| onRemove?: () => void; | ||
| } |
There was a problem hiding this comment.
The Tag component is missing the theme prop that the Button component supports. The Button component accepts a theme prop (gray, blue, green, red) which controls the color scheme. Without exposing this prop, Tag users cannot customize the color theme of their tags, limiting the component's flexibility.
Add a theme prop to TagProps that accepts ButtonTheme values and pass it through to the Button component.
1 participant
Description
This PR add a new Tag component based on Espresso by Frappe design.
Screenshot/Screencast
Tag.Demo.mp4
Checklist