-
Notifications
You must be signed in to change notification settings - Fork 216
feat: Adds dataAttributes support to ButtonDropdown items #4247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import React from 'react'; | ||
|
Check failure on line 3 in src/button-dropdown/__tests__/data-attributes.test.tsx
|
||
| import { render } from '@testing-library/react'; | ||
| import ButtonDropdown from '../../../lib/components/button-dropdown'; | ||
|
|
||
| describe('ButtonDropdown data attributes', () => { | ||
| test('renders custom data attributes on items', () => { | ||
| const { getByText } = render( | ||
| <ButtonDropdown | ||
| items={[ | ||
| { | ||
| id: 'edit', | ||
| text: 'Edit', | ||
| dataAttributes: { | ||
| 'analytics-action': 'edit-product', | ||
| 'item-key': 'product-123', | ||
| }, | ||
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
|
|
||
| const item = getByText('Edit').closest('li'); | ||
| expect(item).toHaveAttribute('data-analytics-action', 'edit-product'); | ||
| expect(item).toHaveAttribute('data-item-key', 'product-123'); | ||
| }); | ||
|
|
||
| test('automatically prepends data- prefix', () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how is that different from the prev test?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the same time, there seems to be no test to ensure that the "data-" prefix is not added (again) when the passed property already has it. |
||
| const { getByText } = render( | ||
| <ButtonDropdown | ||
| items={[ | ||
| { | ||
| id: 'delete', | ||
| text: 'Delete', | ||
| dataAttributes: { 'custom-attr': 'value' }, | ||
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
|
|
||
| const item = getByText('Delete').closest('li'); | ||
| expect(item).toHaveAttribute('data-custom-attr', 'value'); | ||
| }); | ||
|
|
||
| test('excludes testid from dataAttributes', () => { | ||
| const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); | ||
|
|
||
|
Check failure on line 48 in src/button-dropdown/__tests__/data-attributes.test.tsx
|
||
| const { getByText } = render( | ||
| <ButtonDropdown | ||
| items={[ | ||
| { | ||
| id: 'original-id', | ||
| text: 'Action', | ||
| dataAttributes: { testid: 'should-not-override' }, | ||
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
|
|
||
| const item = getByText('Action').closest('li'); | ||
| expect(item).toHaveAttribute('data-testid', 'original-id'); | ||
| expect(consoleSpy).toHaveBeenCalledWith( | ||
| 'ButtonDropdown: "testid" key is reserved and cannot be overridden via dataAttributes' | ||
| ); | ||
|
|
||
|
Check failure on line 66 in src/button-dropdown/__tests__/data-attributes.test.tsx
|
||
| consoleSpy.mockRestore(); | ||
| }); | ||
|
|
||
| test('handles undefined values', () => { | ||
| const { getByText } = render( | ||
| <ButtonDropdown | ||
| items={[ | ||
| { | ||
| id: 'action', | ||
| text: 'Action', | ||
| dataAttributes: { | ||
| defined: 'value', | ||
| undefined: undefined, | ||
| }, | ||
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
|
|
||
| const item = getByText('Action').closest('li'); | ||
| expect(item).toHaveAttribute('data-defined', 'value'); | ||
| expect(item).not.toHaveAttribute('data-undefined'); | ||
| }); | ||
|
|
||
| test('works with multiple items', () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the "works with multiple items", "works with disabled items", etc - can be combined into a single tests with multiple different items. |
||
| const { getByText } = 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', | ||
| text: 'Checkbox', | ||
| checked: true, | ||
| dataAttributes: { 'checkbox-id': 'cb-1' }, | ||
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
|
|
||
| const item = getByText('Checkbox').closest('li'); | ||
| expect(item).toHaveAttribute('data-checkbox-id', 'cb-1'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -282,6 +282,27 @@ 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" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These docs are not parsed by our docs extractor automatically. Please add it to the Let's make them less comprehensive also - I think a single sentence or two should be enough (it is important to mention the prefixing with |
||
| */ | ||
| dataAttributes?: Record<string, string>; | ||
| } | ||
|
|
||
| export interface CheckboxItem | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,32 @@ | |
| 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> => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would move this util to The util can probably take a list of properties to exclude. |
||
| if (!dataAttributes) return {}; | ||
|
Check failure on line 32 in src/button-dropdown/item-element/index.tsx
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fails with our linting rules. Please use the prettier/eslint configuration of the package. You can also run |
||
|
|
||
|
Check failure on line 33 in src/button-dropdown/item-element/index.tsx
|
||
| return Object.entries(dataAttributes).reduce((acc, [key, value]) => { | ||
|
Check failure on line 34 in src/button-dropdown/item-element/index.tsx
|
||
| // Exclude 'testid' to prevent overriding existing data-testid behavior | ||
|
Check failure on line 35 in src/button-dropdown/item-element/index.tsx
|
||
| if (key === 'testid' || key === 'data-testid') { | ||
|
Check failure on line 36 in src/button-dropdown/item-element/index.tsx
|
||
| console.warn('ButtonDropdown: "testid" key is reserved and cannot be overridden via dataAttributes'); | ||
|
Check failure on line 37 in src/button-dropdown/item-element/index.tsx
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
| return acc; | ||
|
Check failure on line 38 in src/button-dropdown/item-element/index.tsx
|
||
| } | ||
|
|
||
| // 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, | ||
|
|
@@ -73,6 +99,7 @@ | |
| role="presentation" | ||
| data-testid={item.id} | ||
| data-description={item.description} | ||
| {...getDataAttributes(item.dataAttributes)} | ||
| onClick={onClick} | ||
| onMouseEnter={onHover} | ||
| onTouchStart={onHover} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use test-utils to find button dropdown items.