From dfc6de69c359284c9a0a9f3128f236c91e987ab3 Mon Sep 17 00:00:00 2001 From: yyh <92089059+lyzno1@users.noreply.github.com> Date: Wed, 4 Mar 2026 13:55:13 +0800 Subject: [PATCH] refactor(web): migrate Button to Base UI with focus-visible (#32941) Signed-off-by: yyh --- .../base/button/__tests__/index.spec.tsx | 222 +++++++++++------- web/app/components/base/button/index.css | 27 ++- .../components/base/button/index.stories.tsx | 45 +++- web/app/components/base/button/index.tsx | 42 +++- .../create/step-two/__tests__/index.spec.tsx | 6 +- .../hit-testing/__tests__/index.spec.tsx | 48 +++- .../header/account-dropdown/compliance.tsx | 3 +- web/app/styles/globals.css | 4 - 8 files changed, 267 insertions(+), 130 deletions(-) diff --git a/web/app/components/base/button/__tests__/index.spec.tsx b/web/app/components/base/button/__tests__/index.spec.tsx index b43ae89403..4fe0ab3570 100644 --- a/web/app/components/base/button/__tests__/index.spec.tsx +++ b/web/app/components/base/button/__tests__/index.spec.tsx @@ -1,110 +1,156 @@ -import { cleanup, fireEvent, render } from '@testing-library/react' -import * as React from 'react' +import { cleanup, fireEvent, render, screen } from '@testing-library/react' import Button from '../index' afterEach(cleanup) -// https://testing-library.com/docs/queries/about + describe('Button', () => { - describe('Button text', () => { - it('Button text should be same as children', async () => { - const { getByRole, container } = render() - expect(getByRole('button').textContent).toBe('Click me') - expect(container.querySelector('button')?.textContent).toBe('Click me') + describe('rendering', () => { + it('renders children text', () => { + render() + expect(screen.getByRole('button')).toHaveTextContent('Click me') + }) + + it('renders as a native button element by default', () => { + render() + expect(screen.getByRole('button').tagName).toBe('BUTTON') + }) + + it('defaults to type="button"', () => { + render() + expect(screen.getByRole('button')).toHaveAttribute('type', 'button') + }) + + it('allows type override to submit', () => { + render() + expect(screen.getByRole('button')).toHaveAttribute('type', 'submit') + }) + + it('renders custom element via render prop', () => { + render() + const link = screen.getByRole('link') + expect(link).toHaveTextContent('Link') + expect(link).toHaveAttribute('href', '/test') }) }) - describe('Button loading', () => { - it('Loading button text should include same as children', async () => { - const { getByRole } = render() - expect(getByRole('button').textContent?.includes('Loading')).toBe(true) - }) - it('Not loading button text should include same as children', async () => { - const { getByRole } = render() - expect(getByRole('button').textContent?.includes('Loading')).toBe(false) + describe('variants', () => { + it('applies default secondary variant', () => { + render() + expect(screen.getByRole('button').className).toContain('btn-secondary') }) - it('Loading button should have loading classname', async () => { + it.each([ + 'primary', + 'warning', + 'secondary', + 'secondary-accent', + 'ghost', + 'ghost-accent', + 'tertiary', + ] as const)('applies %s variant', (variant) => { + render() + expect(screen.getByRole('button').className).toContain(`btn-${variant}`) + }) + + it('applies destructive modifier', () => { + render() + expect(screen.getByRole('button').className).toContain('btn-destructive') + }) + }) + + describe('sizes', () => { + it('applies default medium size', () => { + render() + expect(screen.getByRole('button').className).toContain('btn-medium') + }) + + it.each(['small', 'medium', 'large'] as const)('applies %s size', (size) => { + render() + expect(screen.getByRole('button').className).toContain(`btn-${size}`) + }) + }) + + describe('loading', () => { + it('shows spinner when loading', () => { + render() + expect(screen.getByRole('button').querySelector('.animate-spin')).toBeInTheDocument() + }) + + it('hides spinner when not loading', () => { + render() + expect(screen.getByRole('button').querySelector('.animate-spin')).not.toBeInTheDocument() + }) + + it('auto-disables when loading', () => { + render() + expect(screen.getByRole('button')).toBeDisabled() + }) + + it('sets aria-busy when loading', () => { + render() + expect(screen.getByRole('button')).toHaveAttribute('aria-busy', 'true') + }) + + it('does not set aria-busy when not loading', () => { + render() + expect(screen.getByRole('button')).not.toHaveAttribute('aria-busy') + }) + + it('applies custom spinnerClassName', () => { const animClassName = 'anim-breath' - const { getByRole } = render() - expect(getByRole('button').getElementsByClassName('animate-spin')[0]?.className).toContain(animClassName) + render() + expect(screen.getByRole('button').querySelector('.animate-spin')?.className).toContain(animClassName) }) }) - describe('Button style', () => { - it('Button should have default variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-secondary') + describe('disabled', () => { + it('disables button when disabled prop is set', () => { + render() + expect(screen.getByRole('button')).toBeDisabled() }) - it('Button should have primary variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-primary') - }) - - it('Button should have warning variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-warning') - }) - - it('Button should have secondary variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-secondary') - }) - - it('Button should have secondary-accent variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-secondary-accent') - }) - it('Button should have ghost variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-ghost') - }) - it('Button should have ghost-accent variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-ghost-accent') - }) - - it('Button disabled should have disabled variant', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-disabled') + it('keeps focusable when loading with focusableWhenDisabled', () => { + render() + const button = screen.getByRole('button') + expect(button).toHaveAttribute('aria-disabled', 'true') }) }) - describe('Button size', () => { - it('Button should have default size', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-medium') - }) - - it('Button should have small size', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-small') - }) - - it('Button should have medium size', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-medium') - }) - - it('Button should have large size', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-large') - }) - }) - - describe('Button destructive', () => { - it('Button should have destructive classname', async () => { - const { getByRole } = render() - expect(getByRole('button').className).toContain('btn-destructive') - }) - }) - - describe('Button events', () => { - it('onClick should been call after clicked', async () => { + describe('events', () => { + it('fires onClick when clicked', () => { const onClick = vi.fn() - const { getByRole } = render() - fireEvent.click(getByRole('button')) - expect(onClick).toHaveBeenCalled() + render() + fireEvent.click(screen.getByRole('button')) + expect(onClick).toHaveBeenCalledTimes(1) + }) + + it('does not fire onClick when disabled', () => { + const onClick = vi.fn() + render() + fireEvent.click(screen.getByRole('button')) + expect(onClick).not.toHaveBeenCalled() + }) + + it('does not fire onClick when loading', () => { + const onClick = vi.fn() + render() + fireEvent.click(screen.getByRole('button')) + expect(onClick).not.toHaveBeenCalled() + }) + }) + + describe('ref forwarding', () => { + it('forwards ref to the button element', () => { + let buttonRef: HTMLButtonElement | null = null + render( + , + ) + expect(buttonRef).toBeInstanceOf(HTMLButtonElement) }) }) }) diff --git a/web/app/components/base/button/index.css b/web/app/components/base/button/index.css index 5899c027d3..6360ed9d0c 100644 --- a/web/app/components/base/button/index.css +++ b/web/app/components/base/button/index.css @@ -2,10 +2,11 @@ @layer components { .btn { - @apply inline-flex justify-center items-center cursor-pointer whitespace-nowrap; + @apply inline-flex justify-center items-center cursor-pointer whitespace-nowrap + outline-none focus-visible:ring-2 focus-visible:ring-state-accent-solid; } - .btn-disabled { + .btn:is(:disabled, [data-disabled]) { @apply cursor-not-allowed; } @@ -40,7 +41,7 @@ text-components-button-destructive-primary-text; } - .btn-primary.btn-disabled { + .btn-primary:is(:disabled, [data-disabled]) { @apply shadow-none bg-components-button-primary-bg-disabled @@ -48,7 +49,7 @@ text-components-button-primary-text-disabled; } - .btn-primary.btn-destructive.btn-disabled { + .btn-primary.btn-destructive:is(:disabled, [data-disabled]) { @apply shadow-none bg-components-button-destructive-primary-bg-disabled @@ -68,7 +69,7 @@ text-components-button-secondary-text; } - .btn-secondary.btn-disabled { + .btn-secondary:is(:disabled, [data-disabled]) { @apply backdrop-blur-sm bg-components-button-secondary-bg-disabled @@ -85,7 +86,7 @@ text-components-button-destructive-secondary-text; } - .btn-secondary.btn-destructive.btn-disabled { + .btn-secondary.btn-destructive:is(:disabled, [data-disabled]) { @apply bg-components-button-destructive-secondary-bg-disabled border-components-button-destructive-secondary-border-disabled @@ -104,7 +105,7 @@ text-components-button-secondary-accent-text; } - .btn-secondary-accent.btn-disabled { + .btn-secondary-accent:is(:disabled, [data-disabled]) { @apply bg-components-button-secondary-bg-disabled border-components-button-secondary-border-disabled @@ -120,7 +121,7 @@ text-components-button-destructive-primary-text; } - .btn-warning.btn-disabled { + .btn-warning:is(:disabled, [data-disabled]) { @apply bg-components-button-destructive-primary-bg-disabled border-components-button-destructive-primary-border-disabled @@ -134,7 +135,7 @@ text-components-button-tertiary-text; } - .btn-tertiary.btn-disabled { + .btn-tertiary:is(:disabled, [data-disabled]) { @apply bg-components-button-tertiary-bg-disabled text-components-button-tertiary-text-disabled; @@ -147,7 +148,7 @@ text-components-button-destructive-tertiary-text; } - .btn-tertiary.btn-destructive.btn-disabled { + .btn-tertiary.btn-destructive:is(:disabled, [data-disabled]) { @apply bg-components-button-destructive-tertiary-bg-disabled text-components-button-destructive-tertiary-text-disabled; @@ -159,7 +160,7 @@ text-components-button-ghost-text; } - .btn-ghost.btn-disabled { + .btn-ghost:is(:disabled, [data-disabled]) { @apply text-components-button-ghost-text-disabled; } @@ -170,7 +171,7 @@ text-components-button-destructive-ghost-text; } - .btn-ghost.btn-destructive.btn-disabled { + .btn-ghost.btn-destructive:is(:disabled, [data-disabled]) { @apply text-components-button-destructive-ghost-text-disabled; } @@ -181,7 +182,7 @@ text-components-button-secondary-accent-text; } - .btn-ghost-accent.btn-disabled { + .btn-ghost-accent:is(:disabled, [data-disabled]) { @apply text-components-button-secondary-accent-text-disabled; } diff --git a/web/app/components/base/button/index.stories.tsx b/web/app/components/base/button/index.stories.tsx index 25bd5957e1..5a7ec55e8f 100644 --- a/web/app/components/base/button/index.stories.tsx +++ b/web/app/components/base/button/index.stories.tsx @@ -1,6 +1,5 @@ import type { Meta, StoryObj } from '@storybook/nextjs-vite' -import { RocketLaunchIcon } from '@heroicons/react/20/solid' import { Button } from '.' const meta = { @@ -12,10 +11,16 @@ const meta = { tags: ['autodocs'], argTypes: { loading: { control: 'boolean' }, + destructive: { control: 'boolean' }, + disabled: { control: 'boolean' }, variant: { control: 'select', options: ['primary', 'warning', 'secondary', 'secondary-accent', 'ghost', 'ghost-accent', 'tertiary'], }, + size: { + control: 'select', + options: ['small', 'medium', 'large'], + }, }, args: { variant: 'ghost', @@ -29,11 +34,7 @@ type Story = StoryObj export const Default: Story = { args: { variant: 'primary', - loading: false, children: 'Primary Button', - styleCss: {}, - spinnerClassName: '', - destructive: false, }, } @@ -95,14 +96,46 @@ export const Loading: Story = { }, } +export const Destructive: Story = { + args: { + variant: 'primary', + destructive: true, + children: 'Delete', + }, +} + export const WithIcon: Story = { args: { variant: 'primary', children: ( <> - + Launch ), }, } + +export const SmallSize: Story = { + args: { + variant: 'secondary', + size: 'small', + children: 'Small', + }, +} + +export const LargeSize: Story = { + args: { + variant: 'primary', + size: 'large', + children: 'Large Button', + }, +} + +export const AsLink: Story = { + args: { + variant: 'ghost-accent', + render: , + children: 'Link Button', + }, +} diff --git a/web/app/components/base/button/index.tsx b/web/app/components/base/button/index.tsx index 0de57617af..047ced4c53 100644 --- a/web/app/components/base/button/index.tsx +++ b/web/app/components/base/button/index.tsx @@ -1,12 +1,12 @@ import type { VariantProps } from 'class-variance-authority' -import type { CSSProperties } from 'react' +import { Button as BaseButton } from '@base-ui/react/button' import { cva } from 'class-variance-authority' import * as React from 'react' import { cn } from '@/utils/classnames' import Spinner from '../spinner' const buttonVariants = cva( - 'btn disabled:btn-disabled', + 'btn', { variants: { variant: { @@ -23,6 +23,9 @@ const buttonVariants = cva( medium: 'btn-medium', large: 'btn-large', }, + destructive: { + true: 'btn-destructive', + }, }, defaultVariants: { variant: 'secondary', @@ -32,25 +35,44 @@ const buttonVariants = cva( ) export type ButtonProps = { - destructive?: boolean loading?: boolean - styleCss?: CSSProperties spinnerClassName?: string ref?: React.Ref + render?: React.ReactElement + focusableWhenDisabled?: boolean } & React.ButtonHTMLAttributes & VariantProps -const Button = ({ className, variant, size, destructive, loading, styleCss, children, spinnerClassName, ref, ...props }: ButtonProps) => { +const Button = ({ + className, + variant, + size, + destructive, + loading, + children, + spinnerClassName, + ref, + render, + focusableWhenDisabled, + disabled, + type = 'button', + ...props +}: ButtonProps) => { + const isDisabled = disabled || loading + return ( - + ) } Button.displayName = 'Button' diff --git a/web/app/components/datasets/create/step-two/__tests__/index.spec.tsx b/web/app/components/datasets/create/step-two/__tests__/index.spec.tsx index 9a0a9630ea..86e8ec2ab5 100644 --- a/web/app/components/datasets/create/step-two/__tests__/index.spec.tsx +++ b/web/app/components/datasets/create/step-two/__tests__/index.spec.tsx @@ -1772,16 +1772,14 @@ describe('StepTwoFooter', () => { render() const nextButton = screen.getByText(/nextStep/i).closest('button') - // Button has disabled:btn-disabled class which handles the loading state - expect(nextButton).toHaveClass('disabled:btn-disabled') + expect(nextButton).toBeDisabled() }) it('should show loading state on Save button when creating in setting mode', () => { render() const saveButton = screen.getByText(/save/i).closest('button') - // Button has disabled:btn-disabled class which handles the loading state - expect(saveButton).toHaveClass('disabled:btn-disabled') + expect(saveButton).toBeDisabled() }) }) }) diff --git a/web/app/components/datasets/hit-testing/__tests__/index.spec.tsx b/web/app/components/datasets/hit-testing/__tests__/index.spec.tsx index 0a5a55b744..fe7510b498 100644 --- a/web/app/components/datasets/hit-testing/__tests__/index.spec.tsx +++ b/web/app/components/datasets/hit-testing/__tests__/index.spec.tsx @@ -579,10 +579,20 @@ describe('HitTestingPage', () => { }) describe('Integration: Hit Testing Flow', () => { - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks() mockHitTestingMutateAsync.mockReset() mockExternalHitTestingMutateAsync.mockReset() + + const { useHitTesting, useExternalKnowledgeBaseHitTesting } = await import('@/service/knowledge/use-hit-testing') + vi.mocked(useHitTesting).mockReturnValue({ + mutateAsync: mockHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) + vi.mocked(useExternalKnowledgeBaseHitTesting).mockReturnValue({ + mutateAsync: mockExternalHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) }) it('should complete a full hit testing flow', async () => { @@ -781,8 +791,18 @@ describe('Integration: Hit Testing Flow', () => { // Drawer and Modal Interaction Tests describe('Drawer and Modal Interactions', () => { - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks() + + const { useHitTesting, useExternalKnowledgeBaseHitTesting } = await import('@/service/knowledge/use-hit-testing') + vi.mocked(useHitTesting).mockReturnValue({ + mutateAsync: mockHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) + vi.mocked(useExternalKnowledgeBaseHitTesting).mockReturnValue({ + mutateAsync: mockExternalHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) }) it('should save retrieval config when ModifyRetrievalModal onSave is called', async () => { @@ -828,9 +848,19 @@ describe('Drawer and Modal Interactions', () => { // renderHitResults Coverage Tests describe('renderHitResults Coverage', () => { - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks() mockHitTestingMutateAsync.mockReset() + + const { useHitTesting, useExternalKnowledgeBaseHitTesting } = await import('@/service/knowledge/use-hit-testing') + vi.mocked(useHitTesting).mockReturnValue({ + mutateAsync: mockHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) + vi.mocked(useExternalKnowledgeBaseHitTesting).mockReturnValue({ + mutateAsync: mockExternalHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) }) it('should render hit results panel with records count', async () => { @@ -952,10 +982,20 @@ describe('ModifyRetrievalModal onSave Coverage', () => { // Direct Component Coverage Tests describe('HitTestingPage Internal Functions Coverage', () => { - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks() mockHitTestingMutateAsync.mockReset() mockExternalHitTestingMutateAsync.mockReset() + + const { useHitTesting, useExternalKnowledgeBaseHitTesting } = await import('@/service/knowledge/use-hit-testing') + vi.mocked(useHitTesting).mockReturnValue({ + mutateAsync: mockHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) + vi.mocked(useExternalKnowledgeBaseHitTesting).mockReturnValue({ + mutateAsync: mockExternalHitTestingMutateAsync, + isPending: false, + } as unknown as ReturnType) }) it('should trigger renderHitResults when mutation succeeds with records', async () => { diff --git a/web/app/components/header/account-dropdown/compliance.tsx b/web/app/components/header/account-dropdown/compliance.tsx index 1627a5a052..c048f25c1e 100644 --- a/web/app/components/header/account-dropdown/compliance.tsx +++ b/web/app/components/header/account-dropdown/compliance.tsx @@ -46,9 +46,10 @@ function ComplianceDocActionVisual({ return (
diff --git a/web/app/styles/globals.css b/web/app/styles/globals.css index cf183cad4e..f99371d180 100644 --- a/web/app/styles/globals.css +++ b/web/app/styles/globals.css @@ -121,10 +121,6 @@ a { outline: none; } -button:focus-within { - outline: none; -} - /* @media (prefers-color-scheme: dark) { html { color-scheme: dark;