diff --git a/.nx/version-plans/version-plan-1788964410158.md b/.nx/version-plans/version-plan-1788964410158.md new file mode 100644 index 00000000000..b3b01b91b60 --- /dev/null +++ b/.nx/version-plans/version-plan-1788964410158.md @@ -0,0 +1,14 @@ +--- +gamut: minor +gamut-tests: minor +--- + +Lets consumers pass `data-*` and `aria-*` attributes through to the DOM node that matters, for Pendo and other SKS tooling. + +Nineteen components previously accepted these attributes at the call site (TypeScript never checks hyphenated JSX attribute names) and then silently dropped them, including Modal, Dialog, Overlay, Popover, DataList/DataTable, Pagination, Disclosure, DatePicker, BarChart and Video. They now forward to their root element. + +Composites where the root is not the element users click gain an explicit slot instead of re-routing existing props: `buttonProps` on Disclosure and InfoTip, `buttonProps` plus `dismissButtonProps` on Tag, `labelProps` on Checkbox and Radio, and `inputProps` on Toggle. `List`, `DataList` and `DataTable` place `data-*` on their outer wrapper so the marked element contains the header row as well as the body. + +`aria-label` is now overrideable on Pagination and Breadcrumbs, with their existing defaults unchanged when nothing is passed. Adds a shared `DataAttributes` type so `data-*` keys can be named in nested prop bags, and a `ReservedDataAttributes` guard preventing consumers from replacing `data-floating`, which Popover reads back for outside-click detection. + +For gamut-tests, `setupRtl` no longer treats a `data-*` index signature as a required prop, which would otherwise break every `renderView()` call on a component carrying one. diff --git a/packages/gamut-tests/src/index.tsx b/packages/gamut-tests/src/index.tsx index 8d9c73cadf2..399d0922c3d 100644 --- a/packages/gamut-tests/src/index.tsx +++ b/packages/gamut-tests/src/index.tsx @@ -1,5 +1,11 @@ import { GamutProvider, theme } from '@codecademy/gamut-styles'; -import { setupRtl as setupRtlBase } from 'component-test-setup'; +import { + FullProps, + RemainingPropsAndTestOverrides, + RenderRtl, + SetupComponentType, + setupRtl as setupRtlBase, +} from 'component-test-setup'; import overArgs from 'lodash/overArgs'; import * as React from 'react'; @@ -33,9 +39,65 @@ function withMockGamutProvider( return WithBoundaryComponent; } +/* + * component-test-setup decides whether `renderView`'s argument is required by + * asking, for each key of the props type, whether an arbitrary object would + * satisfy it. A `data-*` index signature (`DataAttributes` in + * @codecademy/gamut) answers no, since `unknown` isn't assignable to its value + * type, so the key gets treated as required and `renderView()` with no + * arguments stops compiling. Index signatures are never genuinely required, so + * drop them before that decision is made. Types only; no runtime effect. + */ +type StripDataAttrs = { + [K in keyof T as K extends `data-${string}` ? never : K]: T[K]; +}; + +// mirrors component-test-setup's internal RequiredKeys, which it doesn't export +type RequiredKeys = { + [K in keyof T]-?: Record extends { [P in K]: T[K] } + ? never + : K; +}[keyof T]; + +type TestProps< + Component extends SetupComponentType, + BaseProps extends Partial> +> = RequiredKeys< + StripDataAttrs, keyof BaseProps>> +> extends never + ? [RemainingPropsAndTestOverrides?] + : [RemainingPropsAndTestOverrides]; + +/* + * component-test-setup doesn't export the type of what a render returns, so + * alias it here. Without an exported name, anything that re-exports a + * `renderView` fails declaration emit with TS4023. + */ +export type GamutRenderRtlReturn< + Component extends SetupComponentType, + BaseProps extends Partial> +> = ReturnType>; + +export type GamutRenderRtl< + Component extends SetupComponentType, + BaseProps extends Partial> +> = { + (...testProps: TestProps): GamutRenderRtlReturn< + Component, + BaseProps + >; + options: ( + options: Parameters['options']>[0] + ) => GamutRenderRtl; +}; + // overArgs isn't fully typed yet for lack of curried generics, so we have to cast it... -export const setupRtl = overArgs( - setupRtlBase, - withMockGamutProvider -) as typeof setupRtlBase; +export const setupRtl = overArgs(setupRtlBase, withMockGamutProvider) as < + Component extends SetupComponentType, + // eslint-disable-next-line @typescript-eslint/no-empty-object-type -- mirrors component-test-setup's own default + BaseProps extends Partial> = {} +>( + Component: Component, + baseProps?: BaseProps +) => GamutRenderRtl; diff --git a/packages/gamut/src/Alert/Alert.tsx b/packages/gamut/src/Alert/Alert.tsx index 7ae02e8cfb5..38578a7acae 100644 --- a/packages/gamut/src/Alert/Alert.tsx +++ b/packages/gamut/src/Alert/Alert.tsx @@ -44,7 +44,19 @@ export type AlertProps = WithChildrenProp & className?: string; /** Callback to be called when the close icon is clicked */ onClose?: () => void; - /** Call to Action Configuration */ + /* + * Call to Action Configuration. Deliberately does NOT intersect + * `DataAttributes`. `ComponentProps` is already a + * large type (`ButtonBaseProps` intersected with `ButtonBase`'s + * anchor|button prop union), and adding a `data-*` index signature on + * top makes any consumer-side `keyof`/mapped type over `cta` (e.g. an + * `ExtractableCTAProps` picking a few keys off it) degrade into a + * signature that constrains every property, plus TS2590 "union type too + * complex" - exactly the failure this caused downstream in the mono + * repo's `ExtractableCTAProps`. `data-*` passed via `cta` still reaches + * the DOM at runtime through `{...cta}` below; only the named type is + * gone, so a `cta={{ 'data-foo': 'bar' }}` literal needs a cast. + */ cta?: Exclude< React.ComponentProps, 'variant' | 'mode' | 'size' diff --git a/packages/gamut/src/Alert/__tests__/Alert.test.tsx b/packages/gamut/src/Alert/__tests__/Alert.test.tsx index 73dbd43d556..295c61f7478 100644 --- a/packages/gamut/src/Alert/__tests__/Alert.test.tsx +++ b/packages/gamut/src/Alert/__tests__/Alert.test.tsx @@ -3,7 +3,7 @@ import { fireEvent } from '@testing-library/dom'; import { act } from '@testing-library/react'; import * as React from 'react'; -import { Alert } from '../Alert'; +import { Alert, AlertProps } from '../Alert'; const children = 'Hello'; const onClose = jest.fn(); @@ -140,5 +140,53 @@ describe('Alert', () => { const closeButton = view.getByRole('button', { name: 'Close alert' }); expect(closeButton).not.toBeDisabled(); }); + + it('accepts a data-* attribute on closeButtonProps as a type (compile-time only)', () => { + /* + * `closeButtonProps` intersects `DataAttributes` (inherited from + * `CloseButtonProps` in Modals/types.ts), so `data-marker` here would + * be a TS2353 error if that type regressed. Alert doesn't currently + * forward arbitrary closeButtonProps keys to the close button at + * runtime - only the type accepting the key is asserted here. + */ + const closeButtonProps: AlertProps['closeButtonProps'] = { + 'data-marker': 'alert-close', + }; + + expect(closeButtonProps).toEqual({ 'data-marker': 'alert-close' }); + }); + }); + + it('forwards aria-* attributes passed via cta to the cta button', () => { + // Compile-time assertion: `cta` intersects `ComponentProps` directly, so `aria-keyshortcuts` here would be a TS2353 + // error if that type regressed. + const { view } = renderView({ + cta: { + children: 'Click Me!', + 'aria-keyshortcuts': 'c', + }, + }); + + const cta = view.getByRole('button', { name: 'Click Me!' }); + expect(cta).toHaveAttribute('aria-keyshortcuts', 'c'); + }); + + it('still forwards data-* attributes passed via cta to the cta button at runtime', () => { + /* + * `cta` deliberately no longer intersects `DataAttributes` (see the note + * on `AlertProps['cta']`), so `data-marker` needs a cast here. This only + * asserts the runtime `{...cta}` spread still forwards it - the named + * type is gone, not the forwarding. + */ + const { view } = renderView({ + cta: { + children: 'Click Me!', + 'data-marker': 'alert-cta', + } as AlertProps['cta'], + }); + + const cta = view.getByRole('button', { name: 'Click Me!' }); + expect(cta).toHaveAttribute('data-marker', 'alert-cta'); }); }); diff --git a/packages/gamut/src/BarChart/__tests__/BarChart.test.tsx b/packages/gamut/src/BarChart/__tests__/BarChart.test.tsx index f4813e7c23b..c8ed449d8a6 100644 --- a/packages/gamut/src/BarChart/__tests__/BarChart.test.tsx +++ b/packages/gamut/src/BarChart/__tests__/BarChart.test.tsx @@ -727,4 +727,17 @@ describe('BarChart', () => { expect(title.tagName).toBe('H2'); }); }); + + describe('Attribute passthrough', () => { + it('forwards data-* and aria-* attributes to the figure element', () => { + const { view } = renderView({ + 'data-marker': 'probe', + 'aria-keyshortcuts': 'probeAria', + } as any); + + const figure = view.getByRole('figure'); + expect(figure).toHaveAttribute('data-marker', 'probe'); + expect(figure).toHaveAttribute('aria-keyshortcuts', 'probeAria'); + }); + }); }); diff --git a/packages/gamut/src/BarChart/index.tsx b/packages/gamut/src/BarChart/index.tsx index c8dbbf18a6e..6c4a39a00c1 100644 --- a/packages/gamut/src/BarChart/index.tsx +++ b/packages/gamut/src/BarChart/index.tsx @@ -44,6 +44,7 @@ export const BarChart = < translations, unit = '', scaleInterval, + ...rest }: BarChartProps) => { const mergedTranslations = useMemo( () => ({ @@ -140,6 +141,7 @@ export const BarChart = < containerType="inline-size" position="relative" width="100%" + {...rest} > diff --git a/packages/gamut/src/BarChart/shared/types.tsx b/packages/gamut/src/BarChart/shared/types.tsx index 953b360584d..516e01507bd 100644 --- a/packages/gamut/src/BarChart/shared/types.tsx +++ b/packages/gamut/src/BarChart/shared/types.tsx @@ -1,6 +1,6 @@ import { GamutIconProps } from '@codecademy/gamut-icons'; import { ColorAlias } from '@codecademy/gamut-styles'; -import { ComponentProps, HTMLProps } from 'react'; +import { ComponentProps, ComponentPropsWithoutRef, HTMLProps } from 'react'; import { ButtonProps } from '../../Button'; import { Text } from '../../Typography/Text'; @@ -93,39 +93,43 @@ export type InferBarType = T extends readonly (infer U)[] export type BarChartProps< TBarValues extends BarProps[] | readonly BarProps[] = BarProps[] -> = BarChartLabel & { - /** Whether to animate bars on mount */ - animate?: boolean; - /** Array of bar data to render */ - barValues: TBarValues; - /** Figure caption for the BarChart. This should be a summary of the information or the overall takeaway of the information in the chart */ - description: string; - /** Hides the visual figcaption */ - hideDescription?: boolean; - /** Hides the visual title for the chart UL */ - hideTitle?: boolean; - /** Maximum value for the value scale */ - maxScaleValue: MaxScaleValue; - /** Unit label to display (e.g., "XP") */ - unit?: string; - /** Style configuration for colors */ - styleConfig?: BarChartStyles; - /** Interval for the value scale markers */ - scaleInterval?: number; - /** Array of sort options to display in the dropdown. Can include string literals ('alphabetically', 'numerically', 'none') or custom sort functions. If not provided, the Select dropdown will not render. */ - sortFns?: ( - | 'alphabetically' - | 'numerically' - | 'none' - | CustomSortOption> - )[]; - /** - * Translations for internationalization. Partial translations are merged with defaults. - * Accessibility is function-only. Two optional keys: stackedBarSummary, singleValueBarSummary. - * stackedBarSummary: used for stacked (two-value) rows; context includes gained (seriesTwoValue - seriesOneValue). - * singleValueBarSummary: used for all single-value rows; the returned string is set as aria-label on the row's link/button when interactive, or rendered in screenreader-only text when not. - */ - translations?: PartialBarChartTranslations; -}; +> = Omit< + ComponentPropsWithoutRef<'figure'>, + 'title' | 'aria-labelledby' | 'color' +> & + BarChartLabel & { + /** Whether to animate bars on mount */ + animate?: boolean; + /** Array of bar data to render */ + barValues: TBarValues; + /** Figure caption for the BarChart. This should be a summary of the information or the overall takeaway of the information in the chart */ + description: string; + /** Hides the visual figcaption */ + hideDescription?: boolean; + /** Hides the visual title for the chart UL */ + hideTitle?: boolean; + /** Maximum value for the value scale */ + maxScaleValue: MaxScaleValue; + /** Unit label to display (e.g., "XP") */ + unit?: string; + /** Style configuration for colors */ + styleConfig?: BarChartStyles; + /** Interval for the value scale markers */ + scaleInterval?: number; + /** Array of sort options to display in the dropdown. Can include string literals ('alphabetically', 'numerically', 'none') or custom sort functions. If not provided, the Select dropdown will not render. */ + sortFns?: ( + | 'alphabetically' + | 'numerically' + | 'none' + | CustomSortOption> + )[]; + /** + * Translations for internationalization. Partial translations are merged with defaults. + * Accessibility is function-only. Two optional keys: stackedBarSummary, singleValueBarSummary. + * stackedBarSummary: used for stacked (two-value) rows; context includes gained (seriesTwoValue - seriesOneValue). + * singleValueBarSummary: used for all single-value rows; the returned string is set as aria-label on the row's link/button when interactive, or rendered in screenreader-only text when not. + */ + translations?: PartialBarChartTranslations; + }; export type BarChartUnit = Pick; diff --git a/packages/gamut/src/Breadcrumbs/__tests__/Breadcrumbs.test.tsx b/packages/gamut/src/Breadcrumbs/__tests__/Breadcrumbs.test.tsx index 73ae0d31a42..d618ac4924c 100644 --- a/packages/gamut/src/Breadcrumbs/__tests__/Breadcrumbs.test.tsx +++ b/packages/gamut/src/Breadcrumbs/__tests__/Breadcrumbs.test.tsx @@ -46,4 +46,37 @@ describe('Breadcrumbs', () => { expect.objectContaining({ payload }) ); }); + + it('forwards data-* and aria-* attributes to the nav element', () => { + const { view } = renderView({ + crumbs: [{ title: 'one' }], + 'data-marker': 'probe', + 'aria-keyshortcuts': 'probeAria', + } as any); + + const nav = view.getByRole('navigation'); + expect(nav).toHaveAttribute('data-marker', 'probe'); + expect(nav).toHaveAttribute('aria-keyshortcuts', 'probeAria'); + }); + + it('defaults aria-label when the consumer does not supply one', () => { + const { view } = renderView({ crumbs: [{ title: 'one' }] }); + + expect(view.getByRole('navigation')).toHaveAttribute( + 'aria-label', + 'breadcrumbs' + ); + }); + + it('lets a consumer override the default aria-label', () => { + const { view } = renderView({ + crumbs: [{ title: 'one' }], + 'aria-label': 'my trail', + }); + + expect(view.getByRole('navigation')).toHaveAttribute( + 'aria-label', + 'my trail' + ); + }); }); diff --git a/packages/gamut/src/Breadcrumbs/index.tsx b/packages/gamut/src/Breadcrumbs/index.tsx index 9f8c66ce9c0..b53ba7605df 100644 --- a/packages/gamut/src/Breadcrumbs/index.tsx +++ b/packages/gamut/src/Breadcrumbs/index.tsx @@ -1,5 +1,6 @@ import { css } from '@codecademy/gamut-styles'; import styled from '@emotion/styled'; +import { ComponentPropsWithoutRef } from 'react'; import * as React from 'react'; import { Anchor } from '../Anchor'; @@ -47,7 +48,10 @@ export const isClickableCrumb = ( crumb: Breadcrumb ): crumb is ClickableCrumb => !!(crumb as ClickableCrumb).href; -export type BreadcrumbsProps = { +export type BreadcrumbsProps = Omit< + ComponentPropsWithoutRef<'nav'>, + 'onClick' | 'className' +> & { crumbs: Breadcrumb[]; onClick?: (event: React.MouseEvent, crumb: ClickableCrumb) => void; className?: string; @@ -57,8 +61,10 @@ export const Breadcrumbs = ({ crumbs, onClick, className, + ...rest }: BreadcrumbsProps) => ( -