Address PR review comments

- Move getDataAttributes to reusable internal/utils location
- Use warnOnce utility instead of console.warn
- Move dataAttributes documentation to items prop description
- Rewrite tests to use ButtonDropdown test-utils
- Combine multiple test cases into single comprehensive test
- Add test for data- prefix not being duplicated
- Fix TypeScript and linting issues

Author: saday giridhar

src/button-dropdown/__tests__/data-attributes.test.tsx

@@ -2,11 +2,13 @@
 // SPDX-License-Identifier: Apache-2.0
 import React from 'react';
 import { render } from '@testing-library/react';
+
 import ButtonDropdown from '../../../lib/components/button-dropdown';
+import createWrapper from '../../../lib/components/test-utils/dom';
 
 describe('ButtonDropdown data attributes', () => {
   test('renders custom data attributes on items', () => {
-    const { getByText } = render(
+    const { container } = render(
       <ButtonDropdown
         items={[
           {
@@ -21,32 +23,37 @@ describe('ButtonDropdown data attributes', () => {
       />
     );
 
-    const item = getByText('Edit').closest('li');
+    const wrapper = createWrapper(container).findButtonDropdown()!;
+    wrapper.openDropdown();
+    const item = wrapper.findItemById('edit')!.getElement();
+
     expect(item).toHaveAttribute('data-analytics-action', 'edit-product');
     expect(item).toHaveAttribute('data-item-key', 'product-123');
   });
 
-  test('automatically prepends data- prefix', () => {
-    const { getByText } = render(
+  test('does not duplicate data- prefix when already present', () => {
+    const { container } = render(
       <ButtonDropdown
         items={[
           {
-            id: 'delete',
-            text: 'Delete',
-            dataAttributes: { 'custom-attr': 'value' },
+            id: 'action',
+            text: 'Action',
+            dataAttributes: { 'data-custom': 'value' },
           },
         ]}
       />
     );
 
-    const item = getByText('Delete').closest('li');
-    expect(item).toHaveAttribute('data-custom-attr', 'value');
+    const wrapper = createWrapper(container).findButtonDropdown()!;
+    wrapper.openDropdown();
+    const item = wrapper.findItemById('action')!.getElement();
+
+    expect(item).toHaveAttribute('data-custom', 'value');
+    expect(item).not.toHaveAttribute('data-data-custom');
   });
 
   test('excludes testid from dataAttributes', () => {
-    const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
-    
-    const { getByText } = render(
+    const { container } = render(
       <ButtonDropdown
         items={[
           {
@@ -58,80 +65,54 @@ describe('ButtonDropdown data attributes', () => {
       />
     );
 
-    const item = getByText('Action').closest('li');
+    const wrapper = createWrapper(container).findButtonDropdown()!;
+    wrapper.openDropdown();
+    const item = wrapper.findItemById('original-id')!.getElement();
+
     expect(item).toHaveAttribute('data-testid', 'original-id');
-    expect(consoleSpy).toHaveBeenCalledWith(
-      'ButtonDropdown: "testid" key is reserved and cannot be overridden via dataAttributes'
-    );
-    
-    consoleSpy.mockRestore();
   });
 
   test('handles undefined values', () => {
-    const { getByText } = render(
+    const dataAttrs: Record<string, string | undefined> = {
+      defined: 'value',
+      undefined: undefined,
+    };
+
+    const { container } = render(
       <ButtonDropdown
         items={[
           {
             id: 'action',
             text: 'Action',
-            dataAttributes: {
-              defined: 'value',
-              undefined: undefined,
-            },
+            dataAttributes: dataAttrs as Record<string, string>,
           },
         ]}
       />
     );
 
-    const item = getByText('Action').closest('li');
+    const wrapper = createWrapper(container).findButtonDropdown()!;
+    wrapper.openDropdown();
+    const item = wrapper.findItemById('action')!.getElement();
+
     expect(item).toHaveAttribute('data-defined', 'value');
     expect(item).not.toHaveAttribute('data-undefined');
   });
 
-  test('works with multiple items', () => {
-    const { getByText } = render(
+  test('works with multiple items, disabled items, and checkbox items', () => {
+    const { container } = render(
       <ButtonDropdown
         items={[
           {
             id: 'edit',
             text: 'Edit',
             dataAttributes: { action: 'edit' },
           },
-          {
-            id: 'delete',
-            text: 'Delete',
-            dataAttributes: { action: 'delete' },
-          },
-        ]}
-      />
-    );
-
-    expect(getByText('Edit').closest('li')).toHaveAttribute('data-action', 'edit');
-    expect(getByText('Delete').closest('li')).toHaveAttribute('data-action', 'delete');
-  });
-
-  test('works with disabled items', () => {
-    const { getByText } = render(
-      <ButtonDropdown
-        items={[
           {
             id: 'disabled',
             text: 'Disabled',
             disabled: true,
             dataAttributes: { state: 'disabled' },
           },
-        ]}
-      />
-    );
-
-    const item = getByText('Disabled').closest('li');
-    expect(item).toHaveAttribute('data-state', 'disabled');
-  });
-
-  test('works with checkbox items', () => {
-    const { getByText } = render(
-      <ButtonDropdown
-        items={[
           {
             itemType: 'checkbox',
             id: 'checkbox-item',
@@ -143,7 +124,11 @@ describe('ButtonDropdown data attributes', () => {
       />
     );
 
-    const item = getByText('Checkbox').closest('li');
-    expect(item).toHaveAttribute('data-checkbox-id', 'cb-1');
+    const wrapper = createWrapper(container).findButtonDropdown()!;
+    wrapper.openDropdown();
+
+    expect(wrapper.findItemById('edit')!.getElement()).toHaveAttribute('data-action', 'edit');
+    expect(wrapper.findItemById('disabled')!.getElement()).toHaveAttribute('data-state', 'disabled');
+    expect(wrapper.findItemById('checkbox-item')!.getElement()).toHaveAttribute('data-checkbox-id', 'cb-1');
   });
 });

src/button-dropdown/interfaces.ts

@@ -29,6 +29,7 @@ export interface ButtonDropdownProps extends BaseComponentProps, ExpandToViewpor
    * - `disabledReason` (string) - (Optional) Displays text near the `text` property when item is disabled. Use to provide additional context.
    * - `description` (string) - additional data that will be passed to a `data-description` attribute. **Deprecated**, has no effect.
    * - `ariaLabel` (string) - (Optional) - ARIA label of the item element.
+   * - `dataAttributes` (Record<string, string>) - (Optional) Custom data attributes for the item element. Attribute names are automatically prefixed with "data-". The "testid" key is reserved.
    *
    * ### action
    *
@@ -282,26 +283,6 @@ export namespace ButtonDropdownProps {
     iconUrl?: string;
     iconSvg?: React.ReactNode;
     labelTag?: string;
-    /**
-     * Custom data attributes to add to the dropdown item element.
-     * Attribute names will automatically be prefixed with "data-".
-     * The "testid" key is reserved and cannot be overridden.
-     *
-     * Use this for analytics tracking, testing selectors, or other metadata.
-     *
-     * @example
-     * items={[
-     *   {
-     *     id: 'edit',
-     *     text: 'Edit',
-     *     dataAttributes: {
-     *       'analytics-action': 'edit-product',
-     *       'item-key': 'product-123'
-     *     }
-     *   }
-     * ]}
-     * // Renders as: data-analytics-action="edit-product" data-item-key="product-123"
-     */
     dataAttributes?: Record<string, string>;
   }
 

src/button-dropdown/item-element/index.tsx

@@ -12,6 +12,7 @@ import InternalIcon, { InternalIconProps } from '../../icon/internal';
 import { useDropdownContext } from '../../internal/components/dropdown/context';
 import useHiddenDescription from '../../internal/hooks/use-hidden-description';
 import { useVisualRefresh } from '../../internal/hooks/use-visual-mode';
+import { getDataAttributes } from '../../internal/utils/data-attributes';
 import { GeneratedAnalyticsMetadataButtonDropdownClick } from '../analytics-metadata/interfaces';
 import { ButtonDropdownProps, InternalCheckboxItem, InternalItem, ItemProps, LinkItem } from '../interfaces';
 import Tooltip from '../tooltip';
@@ -22,32 +23,6 @@ import { getItemTarget } from '../utils/utils';
 import analyticsLabels from '../analytics-metadata/styles.css.js';
 import styles from './styles.css.js';
 
-/**
- * Converts dataAttributes object to DOM data-* attributes.
- * - Automatically prepends 'data-' prefix if not present
- * - Excludes 'testid' to prevent overriding existing behavior
- * - Filters out undefined values
- */
-const getDataAttributes = (dataAttributes?: Record<string, string>): Record<string, string> => {
-  if (!dataAttributes) return {};
-  
-  return Object.entries(dataAttributes).reduce((acc, [key, value]) => {
-    // Exclude 'testid' to prevent overriding existing data-testid behavior
-    if (key === 'testid' || key === 'data-testid') {
-      console.warn('ButtonDropdown: "testid" key is reserved and cannot be overridden via dataAttributes');
-      return acc;
-    }
-    
-    // Skip undefined values
-    if (value === undefined) return acc;
-    
-    // Add 'data-' prefix if not already present
-    const attrKey = key.startsWith('data-') ? key : `data-${key}`;
-    acc[attrKey] = value;
-    return acc;
-  }, {} as Record<string, string>);
-};
-
 const ItemElement = ({
   position = '1',
   index,
@@ -99,7 +74,7 @@ const ItemElement = ({
       role="presentation"
       data-testid={item.id}
       data-description={item.description}
-      {...getDataAttributes(item.dataAttributes)}
+      {...getDataAttributes(item.dataAttributes, ['testid'])}
       onClick={onClick}
       onMouseEnter={onHover}
       onTouchStart={onHover}

src/internal/utils/data-attributes.ts

@@ -0,0 +1,30 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+import { warnOnce } from '@cloudscape-design/component-toolkit/internal';
+
+export function getDataAttributes(
+  dataAttributes: Record<string, string> | undefined,
+  excludeKeys: string[] = []
+): Record<string, string> {
+  if (!dataAttributes) {
+    return {};
+  }
+
+  return Object.entries(dataAttributes).reduce(
+    (acc, [key, value]) => {
+      if (excludeKeys.includes(key) || excludeKeys.includes(`data-${key}`)) {
+        warnOnce('getDataAttributes', `"${key}" key is reserved and cannot be overridden via dataAttributes`);
+        return acc;
+      }
+
+      if (value === undefined) {
+        return acc;
+      }
+
+      const attrKey = key.startsWith('data-') ? key : `data-${key}`;
+      acc[attrKey] = value;
+      return acc;
+    },
+    {} as Record<string, string>
+  );
+}