feat(gamut): forward data-*/aria-* attributes to the right DOM nodes

.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.

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<Props extends React.JSX.IntrinsicAttributes>(
   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<T> = {
+  [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<T> = {
+  [K in keyof T]-?: Record<string, unknown> extends { [P in K]: T[K] }
+    ? never
+    : K;
+}[keyof T];
+
+type TestProps<
+  Component extends SetupComponentType,
+  BaseProps extends Partial<FullProps<Component>>
+> = RequiredKeys<
+  StripDataAttrs<Omit<FullProps<Component>, keyof BaseProps>>
+> extends never
+  ? [RemainingPropsAndTestOverrides<Component, BaseProps>?]
+  : [RemainingPropsAndTestOverrides<Component, BaseProps>];
+
+/*
+ * 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<FullProps<Component>>
+> = ReturnType<RenderRtl<Component, BaseProps>>;
+
+export type GamutRenderRtl<
+  Component extends SetupComponentType,
+  BaseProps extends Partial<FullProps<Component>>
+> = {
+  (...testProps: TestProps<Component, BaseProps>): GamutRenderRtlReturn<
+    Component,
+    BaseProps
+  >;
+  options: (
+    options: Parameters<RenderRtl<Component, BaseProps>['options']>[0]
+  ) => GamutRenderRtl<Component, BaseProps>;
+};
+
 // 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<FullProps<Component>> = {}
+>(
+  Component: Component,
+  baseProps?: BaseProps
+) => GamutRenderRtl<Component, BaseProps>;

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<typeof FillButton>` 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<typeof FillButton>,
       'variant' | 'mode' | 'size'

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<typeof
+    // FillButton>` 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');
   });
 });

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');
+    });
+  });
 });

packages/gamut/src/BarChart/index.tsx

@@ -44,6 +44,7 @@ export const BarChart = <
   translations,
   unit = '',
   scaleInterval,
+  ...rest
 }: BarChartProps<TBarValues>) => {
   const mergedTranslations = useMemo(
     () => ({
@@ -140,6 +141,7 @@ export const BarChart = <
         containerType="inline-size"
         position="relative"
         width="100%"
+        {...rest}
       >
         <ScaleChartHeader maxScaleValue={maxScaleValue} tickCount={tickCount} />
         <Box position="relative" width="100%">

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> = 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<InferBarType<TBarValues>>
-  )[];
-  /**
-   * 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<InferBarType<TBarValues>>
+    )[];
+    /**
+     * 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<BarChartProps, 'unit'>;

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'
+    );
+  });
 });

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 = <T extends string | object>(
   crumb: Breadcrumb<T>
 ): crumb is ClickableCrumb<T> => !!(crumb as ClickableCrumb<T>).href;
 
-export type BreadcrumbsProps<T extends string | object> = {
+export type BreadcrumbsProps<T extends string | object> = Omit<
+  ComponentPropsWithoutRef<'nav'>,
+  'onClick' | 'className'
+> & {
   crumbs: Breadcrumb<T>[];
   onClick?: (event: React.MouseEvent, crumb: ClickableCrumb<T>) => void;
   className?: string;
@@ -57,8 +61,10 @@ export const Breadcrumbs = <T extends string | object>({
   crumbs,
   onClick,
   className,
+  ...rest
 }: BreadcrumbsProps<T>) => (
-  <nav aria-label="breadcrumbs" className={className}>
+  // rest spreads last so a consumer can override the defaults, per house style
+  <nav aria-label="breadcrumbs" className={className} {...rest}>
     <FlexBox as="ol" m={0} p={0}>
       {crumbs.map((crumb, index) => (
         <BreadcrumbPart as="li" key={crumb.title}>

packages/gamut/src/Button/__tests__/TextButton.test.tsx

@@ -1,7 +1,9 @@
 import { StarIcon } from '@codecademy/gamut-icons';
 import userEvent from '@testing-library/user-event';
 import { setupRtl } from 'component-test-setup';
+import { ComponentProps } from 'react';
 
+import { FillButton } from '../FillButton';
 import { TextButton } from '../TextButton';
 
 const onClick = jest.fn();
@@ -26,4 +28,15 @@ describe('TextButton', () => {
 
     view.getByRole('img', { hidden: true });
   });
+
+  it('resolves aria-label by name off ComponentProps<typeof FillButton> (compile-time only)', () => {
+    /*
+     * `ButtonBaseProps` extends `Omit<ComponentPropsWithoutRef<'button'>,
+     * 'size' | 'onClick'>`, so named access to `aria-label` here would be a
+     * TS2339 error if that type regressed.
+     */
+    const ariaLabel: ComponentProps<typeof FillButton>['aria-label'] = 'Submit';
+
+    expect(ariaLabel).toBe('Submit');
+  });
 });