Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/young-candies-appear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
'@leafygreen-ui/confirmation-modal': patch
'@leafygreen-ui/password-input': patch
'@leafygreen-ui/progress-bar': patch
'@leafygreen-ui/form-field': patch
'@lg-tools/eslint-plugin': patch
'@leafygreen-ui/checkbox': patch
'@leafygreen-ui/select': patch
'@leafygreen-ui/modal': patch
'@leafygreen-ui/lib': patch
'@lg-tools/lint': patch
---

Avoid duplicated data-lgid and data-testid
8 changes: 4 additions & 4 deletions packages/checkbox/src/Checkbox/Checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ const Checkbox = React.forwardRef(
className={cx(labelStyle, labelHoverStyle[theme], {
[disabledLabelStyle]: disabled,
})}
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.label}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the reasons we pass the root to label and description is because <Label> and <Description> already generate their own specific lgids internally.

data-lgid={getLgIds(dataLgId).label}
data-testid={getLgIds(dataLgId).label}

So if you pass lgids.root, the root is lg-checkbox and the id generated in <Label> will be lg-checkbox-label.

However, if we pass lgIds.label, that will pass lg-checkbox-label to <Label>, which will generate lg-checkbox-label-label.

The first approach allows us to avoid duplication in the id string

Copy link
Member Author

@kraenhansen kraenhansen Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see. Wouldn't it make more sense (and in alignment with the style-guide / BEM-like) for the Label component to have its own getLgIds function? 🤔 I mean, Label is not a "child element" of some "typography block".

I've pushed a suggestion to work around this in the simplest possible way I could imagine.

const lgIds = getLgIds(dataLgId ?? getLgIds().description);
return (
<Component
data-lgid={lgIds.root}
data-testid={lgIds.root}

data-testid={lgIds.label}
>
<input
{...rest}
Expand Down Expand Up @@ -186,8 +186,8 @@ const Checkbox = React.forwardRef(
<Description
className={descriptionStyle}
disabled={disabled}
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.description}
data-testid={lgIds.description}
>
{description}
</Description>
Expand Down
2 changes: 2 additions & 0 deletions packages/checkbox/src/utils/getLgIds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ export const DEFAULT_LGID_ROOT = 'lg-checkbox';
export const getLgIds = (root: LgIdString = DEFAULT_LGID_ROOT) => {
const ids = {
root,
label: `${root}-label`,
description: `${root}-description`,
} as const;
return ids;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,32 @@ describe('packages/confirmation-modal', () => {
});
});

describe('testid attribute', () => {
it('propagates to the dom element', () => {
const { getByTestId } = renderModal({
open: true,
'data-testid': 'my-modal',
});

const modal = getByTestId('my-modal');
expect(modal).toBeInTheDocument();
});

it('propagates to the buttons', () => {
const { getByTestId } = renderModal({
open: true,
confirmButtonProps: { 'data-testid': 'my-confirm-btn' },
cancelButtonProps: { 'data-testid': 'my-cancel-btn' },
});

const confirmButton = getByTestId('my-confirm-btn');
expect(confirmButton).toBeInTheDocument();

const cancelButton = getByTestId('my-cancel-btn');
expect(cancelButton).toBeInTheDocument();
});
});

// eslint-disable-next-line jest/no-disabled-tests
test.skip('types behave as expected', () => {
<>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ export const ConfirmationModal = forwardRef<
</div>
<Footer>
<Button
{...confirmButtonProps}
data-testid={lgIds.confirm}
{...confirmButtonProps}
disabled={!confirmEnabled || isConfirmDisabled}
className={cx(buttonStyle, confirmButtonProps?.className)}
variant={variant}
Expand All @@ -145,10 +145,10 @@ export const ConfirmationModal = forwardRef<
{buttonText || confirmButtonProps?.children || 'Confirm'}
</Button>
<Button
data-testid={lgIds.cancel}
{...cancelButtonProps}
onClick={handleCancel}
className={cx(buttonStyle, cancelButtonProps?.className)}
data-testid={lgIds.cancel}
>
Cancel
</Button>
Expand Down
8 changes: 4 additions & 4 deletions packages/form-field/src/FormField/FormField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ export const FormField = forwardRef<HTMLDivElement, FormFieldProps>(
>
{label && (
<Label
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.label}
data-testid={lgIds.label}
className={fontStyles}
htmlFor={inputId}
id={labelId}
Expand All @@ -106,8 +106,8 @@ export const FormField = forwardRef<HTMLDivElement, FormFieldProps>(
)}
{description && (
<Description
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.description}
data-testid={lgIds.description}
className={fontStyles}
id={descriptionId}
disabled={disabled}
Expand Down
5 changes: 5 additions & 0 deletions packages/lib/src/LgIdProps/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@ export interface LgIdProps {
* LG test id passed to the component wrapper.
*/
['data-lgid']?: LgIdString;

/**
* An additional test id passed to the component wrapper, meant for use by consumers of the library.
*/
['data-testid']?: string;
}
12 changes: 12 additions & 0 deletions packages/modal/src/Modal/Modal.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,18 @@ describe('packages/modal', () => {
expect(modal).not.toBeVisible();
});
});

describe('testid attribute', () => {
it('propagates to the dom element', () => {
const { getByTestId } = renderModal({
open: true,
'data-testid': 'my-modal',
});

const modal = getByTestId('my-modal');
expect(modal).toBeInTheDocument();
});
});
});

async function expectElementToNotBeRemoved(element: HTMLElement) {
Expand Down
4 changes: 2 additions & 2 deletions packages/modal/src/Modal/ModalView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ const ModalView = React.forwardRef<HTMLDialogElement, ModalProps>(
}}
>
<dialog
data-testid={lgIds.root}
data-lgid={lgIds.root}
{...rest}
ref={dialogRef}
id={id}
data-testid={lgIds.root}
data-lgid={lgIds.root}
className={getDialogStyles({
backdropClassName,
className,
Expand Down
6 changes: 3 additions & 3 deletions packages/password-input/src/PasswordInput/PasswordInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export const PasswordInput = React.forwardRef<
className={className}
ref={forwardedRef}
data-lgid={lgIds.root}
data-test={lgIds.root}
data-testid={lgIds.root}
>
{label && (
<Label
Expand All @@ -124,8 +124,8 @@ export const PasswordInput = React.forwardRef<
})}
htmlFor={inputId}
disabled={disabled}
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.label}
data-testid={lgIds.label}
>
{label}
</Label>
Expand Down
1 change: 1 addition & 0 deletions packages/password-input/src/utils/getLgIds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export const getLgIds = (root: LgIdString = DEFAULT_LGID_ROOT) => {
const ids = {
root,
stateNotifications: `${root}-state_notifications`,
label: `${root}-label`,
} as const;
return ids;
};
Expand Down
4 changes: 2 additions & 2 deletions packages/progress-bar/src/ProgressBar/ProgressBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export function ProgressBar(props: ProgressBarProps) {
>
<div className={headerStyles}>
<Label
data-lgid={lgIds.root} // handled by <Label> internally
data-lgid={lgIds.label} // handled by <Label> internally
id={labelId}
htmlFor={barId}
className={truncatedTextStyles}
Expand Down Expand Up @@ -209,7 +209,7 @@ export function ProgressBar(props: ProgressBarProps) {
<Description
id={descId}
disabled={disabled}
data-lgid={lgIds.root} // handled by <Description> internally
data-lgid={lgIds.description} // handled by <Description> internally
className={cx({ [getAnimatedTextStyles()]: isNewDescription })}
// if on fade-in transition, reset state after animation ends
onAnimationEnd={() => setIsNewDescription(false)}
Expand Down
2 changes: 2 additions & 0 deletions packages/progress-bar/src/testing/getTestUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export const getLgIds = (root: `lg-${string}` = DEFAULT_LGID_ROOT) => {
track: `${root}-track`,
fill: `${root}-fill`,
valueText: `${root}-value_text`,
label: `${root}-label`,
description: `${root}-description`,
} as const;
};

Expand Down
2 changes: 1 addition & 1 deletion packages/select/src/Select.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ export const LiveExample: StoryFn<SelectProps> = ({
}: SelectProps) => (
<Select
{...args}
data-test="hello-world"
data-testid="hello-world"
className={cx(
css`
min-width: 200px;
Expand Down
8 changes: 4 additions & 4 deletions packages/select/src/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -542,8 +542,8 @@ export const Select = forwardRef<HTMLDivElement, SelectProps>(
<div className={labelDescriptionContainerStyle}>
{label && (
<Label
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.label}
data-testid={lgIds.label}
htmlFor={menuButtonId}
id={labelId}
darkMode={darkMode}
Expand Down Expand Up @@ -574,8 +574,8 @@ export const Select = forwardRef<HTMLDivElement, SelectProps>(

{description && (
<Description
data-lgid={lgIds.root}
data-testid={lgIds.root}
data-lgid={lgIds.description}
data-testid={lgIds.description}
id={descriptionId}
darkMode={darkMode}
disabled={disabled}
Expand Down
2 changes: 2 additions & 0 deletions packages/select/src/utils/getLgIds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ export const getLgIds = (root: LgIdString = DEFAULT_LGID_ROOT) => {
popover: `${root}-popover`,
trigger: `${root}-trigger`,
buttonText: `${root}-button_text`,
label: `${root}-label`,
description: `${root}-description`,
} as const;
return ids;
};
Expand Down
33 changes: 22 additions & 11 deletions packages/typography/src/Description/Description.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,10 @@
import React, { ReactNode } from 'react';
import React from 'react';
import { render } from '@testing-library/react';

import { PolymorphicAs } from '@leafygreen-ui/polymorphic';

import Description from './Description';

const renderDescription = ({
as,
children = 'Test description',
}: {
as?: PolymorphicAs;
children?: ReactNode;
}) => {
return render(<Description as={as}>{children}</Description>);
const renderDescription = (props: React.ComponentProps<typeof Description>) => {
return render(<Description {...props} />);
};

describe('Description Component', () => {
Expand All @@ -37,4 +29,23 @@ describe('Description Component', () => {
});
expect(container.querySelector('div')).toBeInTheDocument();
});

test('renders with a default data-lgid', () => {
const { container } = renderDescription({ children: 'Test description' });
expect(container.querySelector('p')).toHaveAttribute(
'data-lgid',
'lg-typography-description',
);
});

test('renders with a custom data-lgid', () => {
const { container } = renderDescription({
children: 'Test description',
'data-lgid': 'lg-custom-lgid',
});
expect(container.querySelector('p')).toHaveAttribute(
'data-lgid',
'lg-custom-lgid',
);
});
});
6 changes: 4 additions & 2 deletions packages/typography/src/Description/Description.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ export const Description = Polymorphic<DescriptionProps>(
const as = asProp ?? asDerivedFromChildren;
const { Component } = usePolymorphic(as);

const lgIds = getLgIds(dataLgId ?? getLgIds().description);

return (
<Component
data-lgid={getLgIds(dataLgId).description}
data-testid={getLgIds(dataLgId).description}
data-lgid={lgIds.root}
data-testid={lgIds.root}
className={cx(
getDescriptionStyle(theme),
descriptionTypeScaleStyles[baseFontSize],
Expand Down
6 changes: 4 additions & 2 deletions packages/typography/src/Label/Label.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ export const Label = Polymorphic<BaseLabelProps>(
const baseFontSize = useUpdatedBaseFontSize(baseFontSizeOverride);
const { Component } = usePolymorphic(as);

const lgIds = getLgIds(dataLgId ?? getLgIds().label);

return (
<Component
data-lgid={getLgIds(dataLgId).label}
data-testid={getLgIds(dataLgId).label}
data-lgid={lgIds.root}
data-testid={lgIds.root}
className={cx(
getLabelStyles(theme),
labelTypeScaleStyles[baseFontSize],
Expand Down
3 changes: 3 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tools/eslint-plugin/scripts/buildRulesIndex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export function buildRulesIndexFile() {
const indexContent = `/**
* DO NOT MODIFY THIS FILE
* ANY CHANGES WILL BE REMOVED ON THE NEXT BUILD
* USE THE "create-rule" SCRIPT TO ADD NEW RULES INSTEAD
*/
${importStatements}

Expand Down
3 changes: 3 additions & 0 deletions tools/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
/**
* DO NOT MODIFY THIS FILE
* ANY CHANGES WILL BE REMOVED ON THE NEXT BUILD
* USE THE "create-rule" SCRIPT TO ADD NEW RULES INSTEAD
*/
import { noDuplicateIdsRule } from './no-duplicate-ids';
import { noIndirectImportsRule } from './no-indirect-imports';
import { noRenderHookFromRtlRule } from './no-render-hook-from-rtl';

export const rules = {
'no-duplicate-ids': noDuplicateIdsRule,
'no-indirect-imports': noIndirectImportsRule,
'no-render-hook-from-rtl': noRenderHookFromRtlRule,
};
Loading
Loading