From 2473fb2f44154f6d56697c76d71172df670bd599 Mon Sep 17 00:00:00 2001 From: dreamwasp Date: Wed, 9 Sep 2026 10:33:51 -0400 Subject: [PATCH 1/8] feat(gamut): forward data-*/aria-* attributes to the right DOM nodes Pendo and other SKS tooling target elements by attribute, but nineteen components accepted data-*/aria-* at the call site and silently dropped them. TypeScript never checks hyphenated JSX attribute names, so these compiled fine and did nothing. Runtime forwarding, previously dropped entirely: Modal and Dialog (now on the role="dialog" node rather than the shroud), Overlay, Popover, Flyout, Coachmark, InfoTip, ToolTip, DataList/DataTable, Pagination, Breadcrumbs, BarChart, DatePicker, Disclosure, Video, ProgressBar, Toast, Toaster, ConnectedFormGroup and SelectDropdown. Explicit slots where the root is not the element users click, rather than re-routing existing props: buttonProps on Disclosure and InfoTip, buttonProps and dismissButtonProps on Tag, labelProps on Checkbox and Radio, inputProps on Toggle. Each is declared `never` on variants it does not apply to, so misuse is a compile error rather than a dead prop. List, DataList and DataTable place data-* on the outer wrapper, the node that already receives id, so the marked element contains the header row and not just the body. aria-label is overrideable on Pagination and Breadcrumbs, with existing defaults unchanged when nothing is passed. Modal, Dialog, Popover and Overlay deliberately keep rest spread first so role, aria-modal and data-floating still win. Adds a shared DataAttributes type for naming data-* keys in nested prop bags, and ReservedDataAttributes to stop consumers replacing data-floating, which PopoverContainer reads back via closest() for outside-click detection. In gamut-tests, setupRtl no longer counts a data-* index signature as a required prop, which would otherwise break every no-argument renderView() call on a component carrying one. Also fixes pre-existing type errors in Disclosure's test defaultProps, which used a header prop that does not exist. --- .../version-plan-1788964410158.md | 14 ++ packages/gamut-tests/src/index.tsx | 72 ++++++++++- packages/gamut/src/Alert/Alert.tsx | 4 +- .../gamut/src/Alert/__tests__/Alert.test.tsx | 33 ++++- .../src/BarChart/__tests__/BarChart.test.tsx | 13 ++ packages/gamut/src/BarChart/index.tsx | 2 + packages/gamut/src/BarChart/shared/types.tsx | 74 ++++++----- .../__tests__/Breadcrumbs.test.tsx | 33 +++++ packages/gamut/src/Breadcrumbs/index.tsx | 10 +- .../src/Button/__tests__/TextButton.test.tsx | 13 ++ packages/gamut/src/Button/shared/types.ts | 13 +- .../Coachmark/__tests__/Coachmark.test.tsx | 12 ++ packages/gamut/src/Coachmark/index.tsx | 64 +++++----- .../src/ConnectedForm/ConnectedFormGroup.tsx | 7 +- .../__tests__/ConnectedForm.test.tsx | 59 +++++++++ packages/gamut/src/DataList/DataGrid.tsx | 7 + .../src/DataList/__tests__/DataGrid.test.tsx | 62 +++++++++ packages/gamut/src/DatePicker/DatePicker.tsx | 25 ++++ .../DatePicker/__tests__/DatePicker.test.tsx | 20 +++ packages/gamut/src/DatePicker/sharedTypes.ts | 5 +- .../src/Disclosure/DisclosureButton/index.tsx | 2 + .../Disclosure/__tests__/Disclosure.test.tsx | 27 +++- packages/gamut/src/Disclosure/index.tsx | 4 + packages/gamut/src/Disclosure/types.ts | 11 +- .../src/Flyout/__tests__/Flyout.test.tsx | 15 +++ packages/gamut/src/Flyout/index.tsx | 7 +- .../SelectDropdown/elements/containers.tsx | 18 ++- .../SelectDropdown/types/component-props.ts | 4 +- .../src/Form/SelectDropdown/types/internal.ts | 5 +- .../src/Form/__tests__/Checkbox.test.tsx | 26 ++++ .../gamut/src/Form/__tests__/Radio.test.tsx | 25 ++++ .../Form/__tests__/SelectDropdown.test.tsx | 31 ++++- packages/gamut/src/Form/inputs/Checkbox.tsx | 33 ++++- packages/gamut/src/Form/inputs/Radio.tsx | 38 +++++- packages/gamut/src/List/List.tsx | 18 ++- .../gamut/src/List/__tests__/List.test.tsx | 26 ++++ packages/gamut/src/Modals/Dialog.tsx | 15 ++- packages/gamut/src/Modals/Modal.tsx | 16 ++- .../src/Modals/__tests__/Dialog.test.tsx | 12 ++ .../gamut/src/Modals/__tests__/Modal.test.tsx | 12 ++ packages/gamut/src/Modals/types.ts | 3 +- .../src/Overlay/__tests__/Overlay.test.tsx | 13 ++ packages/gamut/src/Overlay/index.tsx | 73 ++++++----- .../Pagination/__tests__/Pagination.test.tsx | 34 +++++ packages/gamut/src/Pagination/index.tsx | 8 +- packages/gamut/src/Popover/Popover.tsx | 5 +- .../src/Popover/__tests__/Popover.test.tsx | 13 ++ packages/gamut/src/Popover/types.tsx | 2 + .../src/PopoverContainer/PopoverContainer.tsx | 7 +- .../__tests__/PopoverContainer.test.tsx | 34 +++++ packages/gamut/src/PopoverContainer/types.ts | 3 +- .../__tests__/ProgressBar.test.tsx | 11 ++ packages/gamut/src/ProgressBar/index.tsx | 2 + packages/gamut/src/Tag/__tests__/Tag.test.tsx | 29 +++++ packages/gamut/src/Tag/index.tsx | 5 + packages/gamut/src/Tag/types.tsx | 63 ++++++++- packages/gamut/src/Tip/InfoTip/index.tsx | 8 ++ .../gamut/src/Tip/__tests__/InfoTip.test.tsx | 26 ++++ .../gamut/src/Tip/__tests__/ToolTip.test.tsx | 12 ++ packages/gamut/src/Tip/__tests__/mocks.tsx | 4 +- packages/gamut/src/Tip/shared/FloatingTip.tsx | 2 + packages/gamut/src/Tip/shared/InlineTip.tsx | 2 + packages/gamut/src/Tip/shared/types.tsx | 31 +++-- packages/gamut/src/Toast/Toast.tsx | 8 +- .../gamut/src/Toast/__tests__/Toast.test.tsx | 12 ++ .../src/Toaster/__tests__/Toaster.test.tsx | 27 ++++ packages/gamut/src/Toaster/index.tsx | 21 +-- .../src/Toggle/__tests__/Toggle.test.tsx | 39 ++++++ packages/gamut/src/Toggle/index.tsx | 2 + packages/gamut/src/Toggle/types.tsx | 20 +++ packages/gamut/src/Toggle/utils.tsx | 14 +- .../gamut/src/Video/__tests__/Video.test.tsx | 47 ++++++- packages/gamut/src/Video/index.tsx | 120 ++++++++++-------- .../src/Video/lib/VidstackPlayer/index.tsx | 4 + packages/gamut/src/utils/dataAttributes.ts | 30 +++++ packages/gamut/src/utils/types.ts | 24 ++++ 76 files changed, 1425 insertions(+), 220 deletions(-) create mode 100644 .nx/version-plans/version-plan-1788964410158.md create mode 100644 packages/gamut/src/utils/dataAttributes.ts 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..1fcb6ed926c 100644 --- a/packages/gamut/src/Alert/Alert.tsx +++ b/packages/gamut/src/Alert/Alert.tsx @@ -9,7 +9,7 @@ import { Box } from '../Box'; import { FillButton, IconButton, TextButton } from '../Button'; import { CloseButtonProps } from '../Modals/types'; import { ToolTip } from '../Tip/ToolTip'; -import { WithChildrenProp } from '../utils'; +import { DataAttributes, WithChildrenProp } from '../utils'; import { AlertBanner, AlertBox, @@ -48,7 +48,7 @@ export type AlertProps = WithChildrenProp & cta?: Exclude< React.ComponentProps, 'variant' | 'mode' | 'size' - > & { text?: string }; + > & { text?: string } & DataAttributes; /** Props for customizing the close button */ closeButtonProps?: Omit< NonNullable, diff --git a/packages/gamut/src/Alert/__tests__/Alert.test.tsx b/packages/gamut/src/Alert/__tests__/Alert.test.tsx index 73dbd43d556..c38af132d2e 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,36 @@ 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 data-* and aria-* attributes passed via cta to the cta button', () => { + // Compile-time assertion: `cta` intersects `DataAttributes`, so + // `data-marker` here would be a TS2353 error if that type regressed. + const { view } = renderView({ + cta: { + children: 'Click Me!', + 'data-marker': 'alert-cta', + 'aria-keyshortcuts': 'c', + }, + }); + + const cta = view.getByRole('button', { name: 'Click Me!' }); + expect(cta).toHaveAttribute('data-marker', 'alert-cta'); + expect(cta).toHaveAttribute('aria-keyshortcuts', 'c'); }); }); 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) => ( -