-
Notifications
You must be signed in to change notification settings - Fork 13
Feature: Component Tag #200
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export { default as Tag } from "./tag"; | ||
| export * from "./types"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| 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> | ||
| ), | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||
| /** | ||||||
| * External dependencies. | ||||||
| */ | ||||||
| import { useCallback, useState } from "react"; | ||||||
| import clsx from "clsx"; | ||||||
| import { X } from "lucide-react"; | ||||||
|
|
||||||
| /** | ||||||
| * Internal dependencies. | ||||||
| */ | ||||||
| import type { TagProps } from "./types"; | ||||||
| import Button from "../button/button"; | ||||||
|
|
||||||
| const Tag = ({ | ||||||
| size, | ||||||
| variant, | ||||||
| label, | ||||||
| prefixIcon, | ||||||
| suffixIcon: SuffixIcon = X, | ||||||
| className, | ||||||
| disabled = false, | ||||||
| visible: controlledVisible, | ||||||
| onVisibleChange, | ||||||
| onRemove, | ||||||
| }: TagProps) => { | ||||||
| const [internalVisible, setInternalVisible] = useState(true); | ||||||
|
|
||||||
| const isControlled = controlledVisible !== undefined; | ||||||
| const visible = isControlled ? controlledVisible : internalVisible; | ||||||
|
|
||||||
| const handleRemove = useCallback(() => { | ||||||
| if (isControlled) { | ||||||
| onVisibleChange?.(false); | ||||||
| } else { | ||||||
| setInternalVisible(false); | ||||||
| } | ||||||
| onRemove?.(); | ||||||
| }, [isControlled, onVisibleChange, onRemove]); | ||||||
|
|
||||||
| if (!visible) return null; | ||||||
|
|
||||||
| return ( | ||||||
| <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!", | ||||||
|
||||||
| "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!", |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { render, screen, fireEvent } from "@testing-library/react"; | ||
| import "@testing-library/jest-dom"; | ||
| import Tag from "../tag"; | ||
|
|
||
| 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"); | ||
| }); | ||
| }); | ||
|
Comment on lines
+5
to
+77
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import type { ButtonVariant } from "../button/types"; | ||
|
|
||
| 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>; | ||
|
Comment on lines
+8
to
+10
|
||
| className?: string; | ||
| disabled?: boolean; | ||
| visible?: boolean; | ||
| onVisibleChange?: (visible: boolean) => void; | ||
| onRemove?: () => void; | ||
| } | ||
|
Comment on lines
+3
to
+16
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stories don't include an example of the controlled mode (using the
visibleandonVisibleChangeprops). 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).