diff --git a/.agents/skills/echoes-review/SKILL.md b/.agents/skills/echoes-review/SKILL.md index c429818c..6f656fe6 100644 --- a/.agents/skills/echoes-review/SKILL.md +++ b/.agents/skills/echoes-review/SKILL.md @@ -35,39 +35,27 @@ The two modes use **different data sources**. Do not mix them. ### Mode A: `--pr` -```bash -git fetch origin pull/{pr}/head:pr-{pr} - -gh pr diff {pr} --name-only -gh pr diff {pr} -``` - -For each changed file that looks architecturally significant (component files, index exports, stories, tests, figma-connect files, design token files), read its full content from the fetched PR ref: +Run all three commands in a **single** Bash call so they require only one permission grant: ```bash -git show pr-{pr}: +git fetch origin pull/{pr}/head:pr-{pr} && gh pr diff {pr} --name-only && echo "==FULL_DIFF==" && gh pr diff {pr} ``` -### Mode B: Current branch (no `--pr`) +For each changed file that looks architecturally significant (component files, index exports, stories, tests, figma-connect files, design token files), read its full content in a **single** Bash call: ```bash -git rev-parse --abbrev-ref HEAD +for f in file1 file2 file3; do echo "=== $f ==="; git show pr-{pr}:$f; done ``` -Determine the merge-base. If `--base` was provided, use it directly; otherwise auto-detect: - -```bash -MERGE_BASE=$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD origin/master) -``` +### Mode B: Current branch (no `--pr`) -Get the diff from the merge-base to the **working tree** (includes committed, staged, and unstaged changes): +Run branch detection, merge-base resolution, file list, and full diff in a **single** Bash call so they require only one permission grant: ```bash -git diff $MERGE_BASE --name-only -git diff $MERGE_BASE +git rev-parse --abbrev-ref HEAD && MERGE_BASE=$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD origin/master) && echo "Merge base: $MERGE_BASE" && git diff $MERGE_BASE --name-only && echo "==FULL_DIFF==" && git diff $MERGE_BASE ``` -For each architecturally significant changed file, read the full file from disk for context beyond the diff. +For each architecturally significant changed file that is untracked (not in the diff), read its full content from disk using the Read tool — not a separate Bash call. --- diff --git a/.gitar/review/component-patterns.md b/.gitar/review/component-patterns.md index 4ea0431b..3f3d6161 100644 --- a/.gitar/review/component-patterns.md +++ b/.gitar/review/component-patterns.md @@ -3,9 +3,11 @@ - **No `forwardRef`**: React 19 makes `ref` a regular prop — `forwardRef` is deprecated and must not be used in new components. Accept `ref` directly in the props destructuring (e.g., `function MyComponent({ ref, ...props }: MyProps)`). Flag any newly introduced `forwardRef` wrapper. Existing components that still use `forwardRef` should be flagged as candidates for migration, but are not a hard blocker if the PR is not touching that component's signature. - **`displayName`**: Every component and every styled component must have `.displayName` set explicitly. Flag any missing `displayName`. - **Enums for variants**: Varieties, sizes, and other closed sets must be expressed as exported TypeScript enums (e.g., `ButtonVariety`, `BadgeSize`), not as plain string unions. The enum must be exported from the component file and re-exported from the folder's `index.ts`. +- **Template-literal types in props**: Enum-typed props must use the template-literal form (e.g., `size?: \`${BadgeSize}\``) rather than the raw enum type (e.g., `size?: BadgeSize`). This allows consumers to pass either the enum member (`BadgeSize.Small`) or its string value (`"small"`) without importing the enum. Flag any enum prop that uses the raw enum type directly in the props interface. - **CSS custom properties pattern**: Multi-variant styled components must use CSS custom properties (e.g., `var(--component-color)`) with a variant style map, not inline conditionals or multiple styled-component variants. - **Props interface**: Each component must export a `{ComponentName}Props` interface. Flag components that don't export their props type. -- **Rest props forwarding**: Components must spread remaining props (`...otherProps` / `...restProps`) onto the root DOM node, after any internal props are destructured. Flag components that destructure all props without forwarding the rest, as this silently swallows `data-*`, `aria-*`, `className`, and other valid HTML attributes consumers may pass. +- **Rest props forwarding**: Components must spread remaining props (`...restProps`) onto the root DOM node, after any internal props are destructured. Flag components that destructure all props without forwarding the rest, as this silently swallows `data-*`, `aria-*`, `className`, and other valid HTML attributes consumers may pass. Flag when using a different naming convention than `restProps` for the remaining props object. +- **HTML attribute extension — intentional omission for Radix trigger compatibility**: Props interfaces do **not** need to extend `React.ComponentPropsWithoutRef<'div'>` (or similar) even when `...restProps` is forwarded. Echoes components are frequently used as Radix trigger children (e.g. ``), and Radix injects its own event handlers and ARIA attributes via prop cloning. Extending a concrete HTML element type is done only as needed and filtering only the props we want to keep. The deliberate pattern is: declare only the component's own props (plus `className` and `ref`), spread `...restProps` at runtime, and keep the interface element-type-agnostic. Do **not** flag missing HTML attribute extension as a violation; do flag it if a reviewer is unaware of this intent. - **`className` prop**: Every component's props interface must include an optional `className?: string` field. Flag any component that omits it, even if it otherwise spreads rest props — the explicit declaration is required so consumers know it is supported. - **`isDefined()` for truthiness checks**: Use the `isDefined()` helper from `~common/helpers/types` for all `!= null` / `!== undefined` / conditional-render guards. Flag raw `=== undefined`, `!= null`, or `Boolean(x)` checks where `isDefined(x)` should be used instead. - **Type predicate functions**: When a component needs to narrow a prop type (e.g., distinguishing `ButtonAsLinkProps` from `ButtonProps`), write a standalone type predicate function (`function isFoo(props): props is FooProps`) rather than inline `as` casts or `instanceof` checks. diff --git a/.gitar/review/naming-and-api.md b/.gitar/review/naming-and-api.md index 23ca7d31..ba1ee923 100644 --- a/.gitar/review/naming-and-api.md +++ b/.gitar/review/naming-and-api.md @@ -6,6 +6,6 @@ - **Event handler props**: Must start with `on` (e.g., `onClick`, `onValueChange`). Flag handlers named `handle*`, `click*`, or similar. - **Internal sub-components**: Sub-components within a folder must be prefixed with the parent component name (e.g., `SelectOption`, not just `Option`) to avoid collisions and keep them grouped in search results. - **Predictability**: Prop names should be obvious and consistent with existing Echoes components. Flag novel naming when a standard equivalent exists in the library (e.g., using `type` where similar components use `variety`). -- **Props interface ordering**: Props must follow this order — required/semantic props first, then boolean flags (`is*`/`has*`/`enable*`/`disable*`), then callbacks (`on*`), then layout/style props (`className` last). Flag interfaces that mix callbacks before flags or put semantic props at the end. +- **Props interface ordering**: Props must follow the alphabetical order. Flag interfaces that don't respect it. - **TSDoc on every public prop**: Every field in a public props interface must have a `/** */` doc comment. Optional props that have a default value must document it with `@defaultValue`. Enum members must also be individually documented. Flag any undocumented public prop or enum member. - **Union types for mutually exclusive props**: Props that are mutually exclusive (e.g., `ariaLabel` vs `ariaLabelledBy`, controlled vs uncontrolled open state) must be expressed as a TypeScript union type (`PropsA | PropsB`) rather than making both optional and relying on runtime checks. Flag components where two props are documented as "use one or the other" but the interface permits both simultaneously. diff --git a/.gitar/review/stories-and-figma.md b/.gitar/review/stories-and-figma.md index e290708a..ac385c63 100644 --- a/.gitar/review/stories-and-figma.md +++ b/.gitar/review/stories-and-figma.md @@ -2,4 +2,3 @@ - **Stories file**: Every new component must have a corresponding `stories/{category}/{ComponentName}-stories.tsx` file. Flag missing story files. When an existing component gains a new prop, variety, or behaviour, the matching story must be updated or a new story added to cover it — flag changes to component files with no corresponding change to the stories file. - **Controls coverage**: Stories must expose all meaningful props as Storybook controls. Flag stories that omit enum props from `argTypes`. -- **Figma Connect file**: If a Figma design exists for the component, a `.figma.tsx` file should be present in `figma-connect/`. Flag missing Figma Connect files when the component clearly maps to a Figma component (this is a soft flag — confirm with the author). diff --git a/.gitignore b/.gitignore index bb157cc5..94d09bb4 100644 --- a/.gitignore +++ b/.gitignore @@ -61,6 +61,7 @@ dist/ .sonar-code-context/ .serena/ .claude/hooks/ +jira/ .mcp.json CLAUDE-personal.md CLAUDE.local.md diff --git a/design-tokens/tokens/component/base.json b/design-tokens/tokens/component/base.json index 37440d8c..7aadd4b6 100644 --- a/design-tokens/tokens/component/base.json +++ b/design-tokens/tokens/component/base.json @@ -22,6 +22,18 @@ } } }, + "filter": { + "tag": { + "sizes": { + "maxWidth": { + "default": { + "$type": "sizing", + "$value": "200px" + } + } + } + } + }, "form-control": { "borderRadius": { "default": { @@ -303,4 +315,4 @@ } } } -} \ No newline at end of file +} diff --git a/i18n/keys.json b/i18n/keys.json index 3e2278d6..f5b3ae38 100644 --- a/i18n/keys.json +++ b/i18n/keys.json @@ -19,6 +19,10 @@ "defaultMessage": "Warning banner:", "description": "Screen reader only text that expresses the type of the Warning banner" }, + "filter.tag.dismiss": { + "defaultMessage": "Remove {filter} filter", + "description": "Accessible label for the dismiss button of a filter tag, where {filter} is the name of the filter to be removed" + }, "global_navigation.account": { "defaultMessage": "Account", "description": "aria-label text for the account button (in the GlobalNavigation)" diff --git a/src/common/components/DismissButton.tsx b/src/common/components/DismissButton.tsx index ec12e45d..5b155c1a 100644 --- a/src/common/components/DismissButton.tsx +++ b/src/common/components/DismissButton.tsx @@ -18,7 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ import styled from '@emotion/styled'; -import { forwardRef } from 'react'; +import { Ref } from 'react'; import { cssVar } from '~utils/design-tokens'; import { useButtonClickHandler } from '../../components/buttons/Button'; import { ButtonStyled } from '../../components/buttons/ButtonStyles'; @@ -26,12 +26,39 @@ import { ButtonBaseProps } from '../../components/buttons/ButtonTypes'; import { IconX } from '../../components/icons'; import { Tooltip } from '../../components/tooltip'; +export enum DismissButtonSize { + /** Standard 24×24 px icon button (default). */ + Medium = 'medium', + /** Compact 16×16 px icon button. */ + Small = 'small', +} + interface DismissButtonProps extends Pick { + /** The aria-label for the dismiss button. */ ariaLabel: string; + /** + * Whether the dismiss button has visible hover/focus states or not. + * @defaultValue false + */ + hasVisibleStateStyles?: boolean; + /** React ref forwarded to the root button element. */ + ref?: Ref; + /** + * The size of the dismiss button. + * @defaultValue DismissButtonSize.Medium + */ + size?: `${DismissButtonSize}`; } -export const DismissButton = forwardRef((props, ref) => { - const { ariaLabel, onClick, ...htmlProps } = props; +export function DismissButton(props: Readonly) { + const { + ariaLabel, + hasVisibleStateStyles = false, + onClick, + ref, + size = DismissButtonSize.Medium, + ...htmlProps + } = props; const handleClick = useButtonClickHandler(props); @@ -41,6 +68,8 @@ export const DismissButton = forwardRef(( aria-label={ariaLabel} // Everything above this line can be overridden by the `htmlProps` object {...htmlProps} + css={DISMISS_BUTTON_SIZE_STYLES[size]} + data-visible-state={hasVisibleStateStyles || undefined} onClick={handleClick} ref={ref} type="button"> @@ -48,18 +77,46 @@ export const DismissButton = forwardRef(( ); -}); +} DismissButton.displayName = 'DismissButton'; +const DISMISS_BUTTON_SIZE_STYLES = { + [DismissButtonSize.Small]: { + '--dismiss-button-height': cssVar('dimension-height-400'), + '--dismiss-button-width': cssVar('dimension-width-200'), + '--dismiss-button-font-size': cssVar('font-size-10'), + }, + [DismissButtonSize.Medium]: { + '--dismiss-button-height': cssVar('dimension-height-600'), + '--dismiss-button-width': cssVar('dimension-width-300'), + '--dismiss-button-font-size': cssVar('font-size-20'), + }, +}; + const DismissButtonStyled = styled(ButtonStyled)` flex: 0 0 auto; - height: ${cssVar('dimension-height-600')}; - width: ${cssVar('dimension-width-300')}; + height: var(--dismiss-button-height); + width: var(--dismiss-button-width); + font-size: var(--dismiss-button-font-size); justify-content: center; background-color: ${cssVar('color-background-utility-transparent')}; border-radius: ${cssVar('border-radius-200')}; + + &[data-visible-state='true'] { + background-color: ${cssVar('color-surface-default')}; + + &:hover { + background-color: ${cssVar('color-surface-hover')}; + } + + &:focus, + &:focus-visible { + background-color: ${cssVar('color-surface-active')}; + outline: none; + } + } `; DismissButtonStyled.displayName = 'DismissButtonStyled'; diff --git a/src/components/filters/FilterTag.tsx b/src/components/filters/FilterTag.tsx new file mode 100644 index 00000000..2b44b178 --- /dev/null +++ b/src/components/filters/FilterTag.tsx @@ -0,0 +1,118 @@ +/* + * Echoes React + * Copyright (C) 2023-2025 SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +import styled from '@emotion/styled'; +import { Ref } from 'react'; +import { useIntl } from 'react-intl'; +import { DismissButton } from '~common/components/DismissButton'; +import { truncate } from '~common/helpers/styles'; +import { cssVar } from '~utils/design-tokens'; +import { Label } from '../typography'; + +export interface FilterTagProps { + /** + * The label content displayed inside the filter tag. + */ + children: string; + + /** + * Optional CSS class name applied to the root element. + */ + className?: string; + + /** + * Accessible label for the dismiss button. Should describe what filter will be removed, + * e.g. "Remove severity filter". + * @defaultValue "Remove {filter} filter" where {filter} is the children prop + */ + labelDismiss?: string; + + /** + * Called when the user clicks the dismiss button to remove the filter. + */ + onDismiss: () => void; + + /** React ref forwarded to the root element. */ + ref?: Ref; +} + +/** + * FilterTag displays an active filter criterion with a dismiss button. + * Use it to show currently applied filters that users can remove. + */ +export function FilterTag(props: Readonly) { + const { children, labelDismiss, onDismiss, className, ref, ...restProps } = props; + const { formatMessage } = useIntl(); + + return ( + + {children} + + + ); +} + +FilterTag.displayName = 'FilterTag'; + +const FilterTagWrapper = styled.div` + display: inline-flex; + align-items: center; + gap: ${cssVar('dimension-space-50')}; + + max-width: ${cssVar('filter-tag-sizes-max-width-default')}; + + padding: ${cssVar('dimension-space-50')} ${cssVar('dimension-space-100')} + ${cssVar('dimension-space-50')} ${cssVar('dimension-space-150')}; + + border: ${cssVar('border-width-default')} solid ${cssVar('color-border-bold')}; + border-radius: ${cssVar('border-radius-full')}; + + background-color: ${cssVar('color-surface-default')}; +`; + +FilterTagWrapper.displayName = 'FilterTagWrapper'; + +const FilterTagLabel = styled(Label)` + flex: 1 0 0; + min-width: 0; + + font-size: ${cssVar('font-size-10')}; + line-height: ${cssVar('line-height-10')}; + + ${truncate} +`; + +FilterTagLabel.displayName = 'FilterTagLabel'; diff --git a/src/components/filters/__tests__/FilterTag-test.tsx b/src/components/filters/__tests__/FilterTag-test.tsx new file mode 100644 index 00000000..1603413f --- /dev/null +++ b/src/components/filters/__tests__/FilterTag-test.tsx @@ -0,0 +1,78 @@ +/* + * Echoes React + * Copyright (C) 2023-2025 SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +import { screen } from '@testing-library/react'; +import { render } from '~common/helpers/test-utils'; +import { FilterTag, FilterTagProps } from '../FilterTag'; + +describe('FilterTag', () => { + it('renders the label and dismiss button', async () => { + const { container } = renderFilterTag({ labelDismiss: 'Remove severity filter' }); + + expect(screen.getByText('Severity')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Remove severity filter' })).toBeInTheDocument(); + await expect(container).toHaveNoA11yViolations(); + }); + + it('generates the dismiss button label from children when labelDismiss is not provided', () => { + renderFilterTag(); + + expect(screen.getByRole('button', { name: 'Remove Severity filter' })).toBeInTheDocument(); + }); + + it('calls onDismiss when the dismiss button is clicked', async () => { + const onDismiss = jest.fn(); + const { user } = renderFilterTag({ onDismiss, labelDismiss: 'Remove filter' }); + + await user.click(screen.getByRole('button', { name: 'Remove filter' })); + + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it('activates the dismiss button via keyboard', async () => { + const onDismiss = jest.fn(); + const { user } = renderFilterTag({ onDismiss, labelDismiss: 'Remove filter' }); + + await user.tab(); + await user.keyboard('{Enter}'); + + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it('forwards extra props to the root element', () => { + renderFilterTag({ 'data-testid': 'my-tag' } as Partial); + + expect(screen.getByTestId('my-tag')).toBeInTheDocument(); + }); + + it('accepts a custom className', () => { + renderFilterTag({ className: 'my-tag', 'data-testid': 'root' } as Partial); + + expect(screen.getByTestId('root')).toHaveClass('my-tag'); + }); +}); + +function renderFilterTag(props: Partial = {}) { + return render( + + {props.children ?? 'Severity'} + , + ); +} diff --git a/src/components/filters/index.ts b/src/components/filters/index.ts new file mode 100644 index 00000000..b9f7fe9d --- /dev/null +++ b/src/components/filters/index.ts @@ -0,0 +1,23 @@ +/* + * Echoes React + * Copyright (C) 2023-2025 SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +export { FilterTag } from './FilterTag'; + +export type { FilterTagProps } from './FilterTag'; diff --git a/src/components/index.ts b/src/components/index.ts index fd0126b2..738cfa33 100644 --- a/src/components/index.ts +++ b/src/components/index.ts @@ -27,6 +27,7 @@ export * from './checkbox-group'; export * from './divider'; export * from './dropdown-menu'; export * from './echoes-provider'; +export * from './filters'; export * from './form'; export * from './icons'; export * from './indicators'; diff --git a/src/generated/design-tokens-base.css b/src/generated/design-tokens-base.css index 36d2306b..33c8d672 100644 --- a/src/generated/design-tokens-base.css +++ b/src/generated/design-tokens-base.css @@ -123,6 +123,7 @@ --echoes-banner-sizes-height: 2.75rem; --echoes-badge-sizes-height-small: 1.25rem; --echoes-badge-sizes-height-medium: 1.75rem; + --echoes-filter-tag-sizes-max-width-default: 200px; --echoes-form-control-border-radius-default: 8px; --echoes-form-control-sizes-height-default: 2.25rem; --echoes-indicator-sizes-width-xsmall: 1rem; diff --git a/src/generated/design-tokens-base.json b/src/generated/design-tokens-base.json index 666884e3..f0d5f242 100644 --- a/src/generated/design-tokens-base.json +++ b/src/generated/design-tokens-base.json @@ -101,6 +101,7 @@ "echoes-banner-sizes-height": "2.75rem", "echoes-badge-sizes-height-small": "1.25rem", "echoes-badge-sizes-height-medium": "1.75rem", + "echoes-filter-tag-sizes-max-width-default": "200px", "echoes-form-control-border-radius-default": "8px", "echoes-form-control-sizes-height-default": "2.25rem", "echoes-indicator-sizes-width-xsmall": "1rem", diff --git a/stories/filters/FilterTag-stories.tsx b/stories/filters/FilterTag-stories.tsx new file mode 100644 index 00000000..eb6b73a7 --- /dev/null +++ b/stories/filters/FilterTag-stories.tsx @@ -0,0 +1,52 @@ +/* + * Echoes React + * Copyright (C) 2023-2025 SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +import type { Meta, StoryObj } from '@storybook/react-vite'; +import { FilterTag } from '../../src'; +import { basicWrapperDecorator } from '../helpers/BasicWrapper'; + +const meta: Meta = { + component: FilterTag, + decorators: [basicWrapperDecorator], + + parameters: { + controls: { exclude: ['className', 'onDismiss'] }, + }, + + title: 'Echoes/Filters/FilterTag', +}; + +export default meta; + +type Story = StoryObj; + +export const Default: Story = { + args: { + children: 'Severity', + onDismiss: () => {}, + }, +}; + +export const LongLabel: Story = { + args: { + children: 'A very long filter label that gets truncated', + onDismiss: () => {}, + }, +};