From 2a5326a65db216a18a6cc3b48a36fe1494ae68b1 Mon Sep 17 00:00:00 2001 From: Alexandre Philibeaux Date: Tue, 24 Dec 2024 15:35:08 +0100 Subject: [PATCH] fix(checkbox-list): event click Signed-off-by: Alexandre Philibeaux --- .github/workflows/ci.yml | 126 +++--- .github/workflows/playwright.yml | 27 -- e2e/playwright.config.ts | 60 +-- e2e/tests/default.spec.ts | 9 + e2e/tests/list-checkbox.spec.ts | 29 +- e2e/tests/list.spec.ts | 9 - .../Checkbox/__tests__/index.test.tsx | 48 ++- packages/ui/src/components/Checkbox/index.tsx | 297 +++++++------- packages/ui/src/components/List/Cell.tsx | 19 +- packages/ui/src/components/List/Row.tsx | 386 +++++++++--------- .../List/__stories__/Selectable.stories.tsx | 1 + .../components/List/__tests__/index.test.tsx | 1 + .../ui/src/components/SearchInput/index.tsx | 2 +- 13 files changed, 506 insertions(+), 508 deletions(-) delete mode 100644 .github/workflows/playwright.yml create mode 100644 e2e/tests/default.spec.ts delete mode 100644 e2e/tests/list.spec.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 99cb5b063c..8036036ebf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,36 +9,56 @@ on: - main jobs: + build: + strategy: + matrix: + node: ["22"] + runs-on: ubuntu-24.04 + env: + TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} + TURBO_CACHE: "remote:rw" + steps: + - uses: actions/checkout@v4 # v4.1.4 + - uses: pnpm/action-setup@v4.0.0 + - uses: actions/setup-node@v4.1.0 + with: + node-version: ${{ matrix.node }} + - name: build + run: | + pnpm install --frozen-lockfile + pnpm run build manypkg: runs-on: ubuntu-24.04 env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 + - uses: actions/setup-node@v4.1.0 with: node-version: 22 cache: "pnpm" - - run: | + - name: manypkg + run: | pnpm install --frozen-lockfile pnpm exec manypkg check + typecheck: runs-on: ubuntu-24.04 + needs: [build] env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 + - uses: actions/setup-node@v4.1.0 with: node-version: 22 cache: "pnpm" - - run: | + - name: typecheck + run: | pnpm install --frozen-lockfile pnpm typecheck @@ -46,48 +66,47 @@ jobs: runs-on: ubuntu-24.04 env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 + - uses: actions/setup-node@v4.1.0 with: node-version: 22 cache: "pnpm" - - run: | + - name: oxlint + run: | pnpm install --frozen-lockfile - pnpm build pnpm oxlint -c .oxlintrc.json --quiet lint: runs-on: ubuntu-24.04 + needs: [build] env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 + - uses: actions/setup-node@v4.1.0 with: node-version: 22 cache: "pnpm" - - run: | + - name: lint + run: | pnpm install --frozen-lockfile - pnpm build + pnpm run build pnpm run lint format: runs-on: ubuntu-24.04 env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 + - uses: actions/setup-node@v4.1.0 with: node-version: 22 cache: "pnpm" @@ -99,18 +118,17 @@ jobs: runs-on: ubuntu-24.04 env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" needs: [typecheck, format] strategy: matrix: - node: ["22"] + node: ["20", "22"] steps: - uses: actions/checkout@v4 with: fetch-depth: 10 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 + - uses: actions/setup-node@v4.1.0 with: node-version: ${{ matrix.node }} cache: "pnpm" @@ -139,35 +157,16 @@ jobs: # - run: pnpm run build # - run: pnpm install # - run: pnpm run test:a11y - build: - strategy: - matrix: - node: ["22"] - runs-on: ubuntu-24.04 - env: - TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true - needs: [typecheck, format] - steps: - - uses: actions/checkout@v4 # v4.1.4 - - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js - uses: actions/setup-node@v4.1.0 - with: - node-version: ${{ matrix.node }} - - run: | - pnpm install --frozen-lockfile - pnpm run build publint: runs-on: ubuntu-24.04 env: TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true + TURBO_CACHE: "remote:rw" steps: - uses: actions/checkout@v4 - uses: pnpm/action-setup@v4.0.0 - - name: Use Node.js + - name: publint uses: actions/setup-node@v4.1.0 with: node-version: 22 @@ -178,12 +177,15 @@ jobs: deploy: runs-on: ubuntu-24.04 + needs: [publint, typecheck, build] env: IMAGE_NAME: rg.fr-par.scw.cloud/ultraviolet/storybook DEPLOYMENT_NAME: "storybook" TURBO_TOKEN: ${{ secrets.TURBO_TOKEN }} - TURBO_REMOTE_ONLY: true - needs: [publint, typecheck, build] + TURBO_CACHE: "remote:rw" + # https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-usage-of-the-jobs-context + outputs: + base_url: ${{ steps.deploy.outputs.url }} steps: - uses: actions/checkout@v4 - name: Inject slug/short variables @@ -296,3 +298,31 @@ jobs: run: | rm -rf /tmp/docs/.buildx-cache mv /tmp/docs/.buildx-cache-new /tmp/docs/.buildx-cache + + e2e: + timeout-minutes: 5 + runs-on: ubuntu-latest + needs: [deploy] + env: + CI: true + TURBO_CACHE: "remote:rw" + BASE_URL: ${{ needs.deploy.outputs.base_url }} + steps: + - uses: actions/checkout@v4 + - uses: pnpm/action-setup@v4.0.0 + - uses: actions/setup-node@v4.1.0 + with: + node-version: 22 + cache: "pnpm" + - name: install pnpm deps + run: | + pnpm install --frozen-lockfile + pnpm exec playwright install --with-deps + - name: run e2e + run: pnpm run test:e2e + - uses: actions/upload-artifact@v4 + if: ${{ !cancelled() }} + with: + name: playwright-report + path: playwright-report/ + retention-days: 5 diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml deleted file mode 100644 index e35b7b91ab..0000000000 --- a/.github/workflows/playwright.yml +++ /dev/null @@ -1,27 +0,0 @@ -name: Playwright Tests -on: - push: - branches: [main, master] - pull_request: - branches: [main, master] -jobs: - test: - timeout-minutes: 60 - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - node-version: lts/* - - name: Install dependencies - run: npm install -g pnpm && pnpm install - - name: Install Playwright Browsers - run: pnpm exec playwright install --with-deps - - name: Run Playwright tests - run: pnpm run test:e2e - - uses: actions/upload-artifact@v4 - if: ${{ !cancelled() }} - with: - name: playwright-report - path: playwright-report/ - retention-days: 30 diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index 3161952c63..ce481616fc 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -6,23 +6,22 @@ const isCI = process.env['CI'] const baseURL = process.env['BASE_URL'] ?? 'http://localhost:6006' +const times = { + '1min': 60 * 1000, + '3min': 3 * 60 * 1000, +} + export default defineConfig({ testDir: './tests', - /* Run tests in files in parallel */ fullyParallel: true, - /* Fail the build on CI if you accidentally left test.only in the source code. */ forbidOnly: !!isCI, - /* Retry on CI only */ retries: isCI ? 2 : 0, - /* Opt out of parallel tests on CI. */ workers: isCI ? 1 : undefined, - /* Reporter to use. See https://playwright.dev/docs/test-reporters */ reporter: 'html', - /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ + globalTimeout: isCI ? 5 * times['1min'] : undefined, + timeout: isCI ? 1 * times['1min'] : undefined, use: { baseURL, - - /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */ trace: 'on-first-retry', }, @@ -32,42 +31,13 @@ export default defineConfig({ name: 'chromium', use: { ...devices['Desktop Chrome'] }, }, - - // { - // name: 'firefox', - // use: { ...devices['Desktop Firefox'] }, - // }, - - // { - // name: 'webkit', - // use: { ...devices['Desktop Safari'] }, - // }, - - /* Test against mobile viewports. */ - // { - // name: 'Mobile Chrome', - // use: { ...devices['Pixel 5'] }, - // }, - // { - // name: 'Mobile Safari', - // use: { ...devices['iPhone 12'] }, - // }, - - /* Test against branded browsers. */ - // { - // name: 'Microsoft Edge', - // use: { ...devices['Desktop Edge'], channel: 'msedge' }, - // }, - // { - // name: 'Google Chrome', - // use: { ...devices['Desktop Chrome'], channel: 'chrome' }, - // }, + { + name: 'firefox', + use: { ...devices['Desktop Firefox'] }, + }, + { + name: 'webkit', + use: { ...devices['Desktop Safari'] }, + }, ], - - /* Run your local dev server before starting the tests */ - // webServer: { - // command: 'npm run start', - // url: 'http://127.0.0.1:3000', - // reuseExistingServer: !process.env.CI, - // }, }) diff --git a/e2e/tests/default.spec.ts b/e2e/tests/default.spec.ts new file mode 100644 index 0000000000..8526507500 --- /dev/null +++ b/e2e/tests/default.spec.ts @@ -0,0 +1,9 @@ +import { expect, test } from '@playwright/test' + +test.describe('Default', () => { + test('title', async ({ page, baseURL }) => { + await page.goto(`${baseURL}`) + + await expect(page).toHaveTitle('Get started - Docs ⋅ Storybook') + }) +}) diff --git a/e2e/tests/list-checkbox.spec.ts b/e2e/tests/list-checkbox.spec.ts index 98b121bc8a..a32a18c993 100644 --- a/e2e/tests/list-checkbox.spec.ts +++ b/e2e/tests/list-checkbox.spec.ts @@ -2,19 +2,24 @@ import { expect, test } from '@playwright/test' const defaultLocator = 'iframe[title="storybook-preview-iframe"]' -test('checkbox list', async ({ page, baseURL }) => { - const url = `${baseURL}/?path=/story/components-data-display-list--selectable` - await page.goto(url) +const defaultURL = `/?path=/story/components-data-display-list--selectable` - const rootLocator = page.locator(defaultLocator).contentFrame() +test.describe('List', () => { + test('Checkbox Row', async ({ page, baseURL }) => { + const url = `${baseURL}${defaultURL}` - await rootLocator - .getByRole('row', { name: 'select Venus 0.718AU 0.728AU' }) - .getByLabel('select') - .check() + await page.goto(url) - const checkbox = rootLocator.locator( - `input[type='checkbox'][name='list-select-checkbox'][value="venus"]`, - ) - await expect(checkbox).toBeChecked() + const rootLocator = page.locator(defaultLocator).contentFrame() + await rootLocator + .getByRole('row', { name: 'select Venus 0.718AU 0.728AU' }) + .getByLabel('select') + .check() + + const checkbox = rootLocator.locator( + `input[type='checkbox'][name='list-select-checkbox'][value="venus"]`, + ) + + await expect(checkbox).toBeChecked() + }) }) diff --git a/e2e/tests/list.spec.ts b/e2e/tests/list.spec.ts deleted file mode 100644 index 900d6294e4..0000000000 --- a/e2e/tests/list.spec.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { expect, test } from '@playwright/test' - -test('has title', async ({ page, baseURL }) => { - const url = baseURL - await page.goto(`${url}`) - - // Expect a title "to contain" a substring. - await expect(page).toHaveTitle('Get started - Docs ⋅ Storybook') -}) diff --git a/packages/ui/src/components/Checkbox/__tests__/index.test.tsx b/packages/ui/src/components/Checkbox/__tests__/index.test.tsx index 8d92c10fcb..b3b5fe7f56 100644 --- a/packages/ui/src/components/Checkbox/__tests__/index.test.tsx +++ b/packages/ui/src/components/Checkbox/__tests__/index.test.tsx @@ -1,10 +1,24 @@ import { screen } from '@testing-library/react' import { userEvent } from '@testing-library/user-event' import { renderWithTheme, shouldMatchEmotionSnapshot } from '@utils/test' +import { useReducer } from 'react' +import type { ActionDispatch } from 'react' import { describe, expect, test } from 'vitest' import { Checkbox } from '..' +type ChildrenProps = { checked: boolean; onChange: ActionDispatch<[]> } +type Props = { + defaultChecked?: boolean + children: (options: ChildrenProps) => React.ReactNode +} + describe('Checkbox', () => { + const LocalControlValue = ({ defaultChecked = false, children }: Props) => { + const [checked, setCheked] = useReducer(check => !check, defaultChecked) + + return children({ checked, onChange: setCheked }) + } + test('renders correctly', () => shouldMatchEmotionSnapshot( { test('renders with click event', async () => { renderWithTheme( - {}} size={37} value="test"> - Checkbox Label - , + + {({ checked, onChange }) => ( + + Checkbox Label + + )} + , ) const input = screen.getByRole('checkbox', { @@ -145,9 +168,19 @@ describe('Checkbox', () => { test('renders with click event with progress', async () => { renderWithTheme( - {}} size={37} value="test" progress> - Checkbox Label - , + + {({ checked, onChange }) => ( + + Checkbox Label + + )} + , ) const input = screen.getByRole('checkbox', { @@ -155,5 +188,8 @@ describe('Checkbox', () => { }) await userEvent.click(input) expect(input.checked).toBeTruthy() + // checked value cannot change during progress/loading + await userEvent.click(input) + expect(input.checked).toBeTruthy() }) }) diff --git a/packages/ui/src/components/Checkbox/index.tsx b/packages/ui/src/components/Checkbox/index.tsx index 5510273be1..3f700e7da4 100644 --- a/packages/ui/src/components/Checkbox/index.tsx +++ b/packages/ui/src/components/Checkbox/index.tsx @@ -1,8 +1,8 @@ import { useTheme } from '@emotion/react' import styled from '@emotion/styled' import { AsteriskIcon } from '@ultraviolet/icons' -import type { ChangeEvent, InputHTMLAttributes, ReactNode, Ref } from 'react' -import { useCallback, useEffect, useId, useState } from 'react' +import type { InputHTMLAttributes, ReactNode } from 'react' +import { forwardRef, useId } from 'react' import { Loader } from '../Loader' import { Stack } from '../Stack' import { Text } from '../Text' @@ -270,7 +270,6 @@ type LabelProp = } type CheckboxProps = { - ref?: Ref error?: string | ReactNode /** * @deprecated Size prop is deprecated and will be removed in next major update. @@ -290,171 +289,159 @@ type CheckboxProps = { tooltip?: string } & Pick< InputHTMLAttributes, - | 'onFocus' - | 'onBlur' - | 'name' - | 'value' | 'autoFocus' | 'id' + | 'name' + | 'onBlur' | 'onChange' + | 'onFocus' | 'tabIndex' - | 'onClick' + | 'value' > & LabelProp /** * Checkbox is an input component used to select or deselect an option. */ -export const Checkbox = ({ - id, - checked = false, - onChange, - onFocus, - onBlur, - onClick, - error, - name, - helper, - value, - size, - children, - progress = false, - disabled = false, - autoFocus = false, - className, - 'data-visibility': dataVisibility, - 'aria-label': ariaLabel, - required, - 'data-testid': dataTestId, - tooltip, - tabIndex, - ref, -}: CheckboxProps) => { - const theme = useTheme() - const [state, setState] = useState(checked) - const uniqId = useId() - const localId = id ?? uniqId - - useEffect(() => { - setState(checked) - }, [checked]) - - const onLocalChange = useCallback( - (event: ChangeEvent) => { - if (!progress) onChange?.(event) - setState(current => - current === 'indeterminate' ? false : event.target.checked, - ) +export const Checkbox = forwardRef( + ( + { + id, + checked = false, + onChange, + onFocus, + onBlur, + error, + name, + helper, + value, + size, + children, + progress = false, + disabled = false, + autoFocus = false, + className, + 'data-visibility': dataVisibility, + 'aria-label': ariaLabel, + required, + 'data-testid': dataTestId, + tooltip, + tabIndex, }, - [onChange, progress, setState], - ) - - return ( - - - {progress ? ( - - - - ) : null} - - - {!progress ? ( - - - {state !== 'indeterminate' ? ( - - ) : ( - - )} - - - ) : null} - - - - {children ? ( - <> - {typeof children === 'string' ? ( - - {children} - + ref, + ) => { + const theme = useTheme() + const uniqId = useId() + const localId = id ?? uniqId + + return ( + + + {progress ? ( + + + + ) : null} + + + {!progress ? ( + + + {checked !== 'indeterminate' ? ( + ) : ( - {children} + )} - + + + ) : null} + + + + {children ? ( + <> + {typeof children === 'string' ? ( + + {children} + + ) : ( + {children} + )} + + ) : null} + {required ? ( + + + + ) : null} + + + {helper ? ( + + {helper} + ) : null} - {required ? ( - - - + + {error && typeof error !== 'boolean' ? ( + + {error} + ) : null} - - {helper ? ( - - {helper} - - ) : null} - - {error && typeof error !== 'boolean' ? ( - - {error} - - ) : null} - - - - ) -} + + + ) + }, +) diff --git a/packages/ui/src/components/List/Cell.tsx b/packages/ui/src/components/List/Cell.tsx index 8082059d1a..3a71e4c94e 100644 --- a/packages/ui/src/components/List/Cell.tsx +++ b/packages/ui/src/components/List/Cell.tsx @@ -1,10 +1,5 @@ import styled from '@emotion/styled' -import type { - ForwardedRef, - KeyboardEventHandler, - MouseEventHandler, - ReactNode, -} from 'react' +import type { KeyboardEventHandler, MouseEventHandler, ReactNode } from 'react' import { forwardRef } from 'react' const StyledCell = styled.td` @@ -27,16 +22,10 @@ type CellProps = { colSpan?: number } -export const Cell = forwardRef( +export const Cell = forwardRef( ( - { - children, - className, - preventClick, - 'data-testid': dataTestid, - colSpan, - }: CellProps, - ref: ForwardedRef, + { children, className, preventClick, 'data-testid': dataTestid, colSpan }, + ref, ) => { const handleClick: MouseEventHandler = event => { if (preventClick) { diff --git a/packages/ui/src/components/List/Row.tsx b/packages/ui/src/components/List/Row.tsx index 9066190c1d..7d3eb09ed6 100644 --- a/packages/ui/src/components/List/Row.tsx +++ b/packages/ui/src/components/List/Row.tsx @@ -1,7 +1,14 @@ import { useTheme } from '@emotion/react' import styled from '@emotion/styled' -import type { ReactNode, Ref } from 'react' -import { Children, useCallback, useEffect, useId, useRef } from 'react' +import type { ReactNode } from 'react' +import { + Children, + forwardRef, + useCallback, + useEffect, + useId, + useRef, +} from 'react' import type { SENTIMENTS, space } from '../../theme' import { Button } from '../Button' import { Checkbox } from '../Checkbox' @@ -183,199 +190,198 @@ type RowProps = { */ expandablePadding?: keyof typeof space 'data-testid'?: string - ref?: Ref } -export const Row = ({ - children, - id, - expandable, - disabled, - selectDisabled, - sentiment = 'neutral', - expanded, - className, - expandablePadding, - 'data-testid': dataTestid, - ref, -}: RowProps) => { - const { - selectable, - registerExpandableRow, - expandedRowIds, - expandRow, - collapseRow, - registerSelectableRow, - selectedRowIds, - selectRow, - unselectRow, - expandButton, - refList, - inRange, - columns, - } = useListContext() - - const theme = useTheme() - - const expandedRowId = useId() - - const checkboxRef = useRef(null) - - const isSelectDisabled = - disabled || (selectDisabled !== undefined && selectDisabled !== false) - - const hasExpandable = !!expandable - useEffect(() => { - if (hasExpandable) { - const unregisterCallback = registerExpandableRow(id, expanded) - - return unregisterCallback - } - - return undefined - }, [id, hasExpandable, registerExpandableRow, expanded, expandRow]) - - useEffect(() => { - if (!isSelectDisabled) { - const unregisterCallback = registerSelectableRow(id) - - return unregisterCallback - } - - return undefined - }, [id, registerSelectableRow, isSelectDisabled]) - - const toggleRowExpand = useCallback(() => { - if (expandedRowIds[id]) { - collapseRow(id) - } else { - expandRow(id) - } - }, [collapseRow, expandRow, expandedRowIds, id]) - - const canClickRowToExpand = !disabled && !!expandable && !expandButton - - useEffect(() => { - const refAtEffectStart = refList.current - const { current } = checkboxRef - - if (refAtEffectStart && current && !refAtEffectStart.includes(current)) { - refList.current.push(current) - } - }, [refList]) - - const childrenLength = - Children.count(children) + (selectable ? 1 : 0) + (expandButton ? 1 : 0) - - return ( - <> - { - // if (event.key === ' ') { - // toggleRowExpand() - // event.preventDefault() - // } - // } - // : undefined - // } - // tabIndex={canClickRowToExpand ? 0 : -1} - sentiment={sentiment} - aria-disabled={disabled} - aria-expanded={expandable ? expandedRowIds[id] : undefined} - aria-controls={ - expandable && expandedRowIds[id] ? expandedRowId : undefined - } - data-highlight={selectable && !!selectedRowIds[id]} - data-testid={dataTestid} - columns={columns} - columnsStartAt={(selectable ? 1 : 0) + (expandButton ? 1 : 0)} - > - {selectable ? ( - - - - { - if (selectedRowIds[id]) { - unselectRow(id) - } else { - selectRow(id) - } - }} - onClick={() => { - if (selectedRowIds[id]) { - unselectRow(id) - } else { - selectRow(id) - } - }} - disabled={isSelectDisabled} - inRange={inRange.includes(id)} - /> - - - - ) : null} - {expandButton ? ( - -