From 11a735e02774f56848cadc0fb08f5c62b9644061 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 9 Nov 2026 23:27:12 +0100 Subject: [PATCH 01/16] Fix medium shortcut button padding Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/button-shortcut-padding.md | 5 +++++ packages/react/src/Button/ButtonBase.module.css | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 .changeset/button-shortcut-padding.md diff --git a/.changeset/button-shortcut-padding.md b/.changeset/button-shortcut-padding.md new file mode 100644 index 00000000000..aa0cb53dffe --- /dev/null +++ b/.changeset/button-shortcut-padding.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Button: Adjust horizontal padding for medium buttons with keyboard shortcuts. diff --git a/packages/react/src/Button/ButtonBase.module.css b/packages/react/src/Button/ButtonBase.module.css index 278b4979314..450375cf770 100644 --- a/packages/react/src/Button/ButtonBase.module.css +++ b/packages/react/src/Button/ButtonBase.module.css @@ -27,6 +27,11 @@ /* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */ &:has([data-kbd-chord]) { padding-inline-end: var(--base-size-6); + + &:where([data-size='medium']) { + padding-inline-start: calc(var(--base-size-12) - var(--base-size-2)); + padding-inline-end: var(--base-size-8); + } } &:hover { From dc056b9ff577249ecedc774f992ec647f3f1045c Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Mon, 9 Nov 2026 23:44:25 +0100 Subject: [PATCH 02/16] Match Button spacing across sizes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/small-button-spacing.md | 5 + .../react/src/Button/ButtonBase.module.css | 47 ++++++---- packages/react/src/Button/ButtonBase.tsx | 2 +- .../src/Button/__tests__/Button.test.tsx | 91 +++++++++++++++++++ 4 files changed, 128 insertions(+), 17 deletions(-) create mode 100644 .changeset/small-button-spacing.md diff --git a/.changeset/small-button-spacing.md b/.changeset/small-button-spacing.md new file mode 100644 index 00000000000..d88f293563d --- /dev/null +++ b/.changeset/small-button-spacing.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Button: Align visual spacing, keyboard shortcut padding, and label line height across button sizes. diff --git a/packages/react/src/Button/ButtonBase.module.css b/packages/react/src/Button/ButtonBase.module.css index 450375cf770..ab8df9c23ff 100644 --- a/packages/react/src/Button/ButtonBase.module.css +++ b/packages/react/src/Button/ButtonBase.module.css @@ -24,10 +24,27 @@ justify-content: space-between; gap: var(--base-size-8); + &.HasLeadingVisual:where(:not([data-icon-only-counter])) { + padding-inline: var(--base-size-8) var(--base-size-12); + + &:where([data-size='small']) { + padding-inline: var(--base-size-6) var(--base-size-8); + } + + &:where([data-size='large']) { + padding-inline: var(--base-size-12) var(--base-size-16); + } + } + /* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */ &:has([data-kbd-chord]) { padding-inline-end: var(--base-size-6); + &:where([data-size='small']) { + /* stylelint-disable-next-line primer/spacing */ + padding-inline-start: var(--control-small-paddingInline-condensed); + } + &:where([data-size='medium']) { padding-inline-start: calc(var(--base-size-12) - var(--base-size-2)); padding-inline-end: var(--base-size-8); @@ -156,7 +173,7 @@ align-content: center; & > :not(:last-child) { - margin-right: var(--base-size-8); + margin-right: var(--base-size-6); } /* Content alignment */ @@ -175,7 +192,8 @@ } & :where(.Label) { - line-height: var(--text-body-lineHeight-medium); + /* stylelint-disable-next-line primer/typography -- match the 20px control label line height */ + line-height: var(--base-size-20); white-space: nowrap; grid-area: text; } @@ -194,16 +212,12 @@ height: var(--control-small-size); /* stylelint-disable-next-line primer/spacing */ padding: 0 var(--control-small-paddingInline-condensed); - gap: var(--control-small-gap); + gap: var(--base-size-6); font-size: var(--text-body-size-small); - - & .ButtonContent > :not(:last-child) { - /* stylelint-disable-next-line primer/spacing */ - margin-right: var(--control-small-gap); - } + line-height: var(--base-text-lineHeight-normal); & .Label { - line-height: var(--text-body-lineHeight-small); + line-height: var(--base-text-lineHeight-normal); } } @@ -211,16 +225,12 @@ height: var(--control-large-size); /* stylelint-disable-next-line primer/spacing */ padding: 0 var(--control-large-paddingInline-spacious); - gap: var(--control-large-gap); - - & .ButtonContent > :not(:last-child) { - /* stylelint-disable-next-line primer/spacing */ - margin-right: var(--control-large-gap); - } + gap: var(--base-size-6); /* stylelint-disable-next-line selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */ &:has([data-kbd-chord]) { - padding-inline-end: var(--base-size-8); + padding-inline-start: var(--base-size-12); + padding-inline-end: calc(var(--base-size-12) - var(--base-size-2)); } } @@ -622,9 +632,14 @@ } & .Label { + line-height: var(--text-body-lineHeight-medium); white-space: unset; } + &:where([data-size='small']) .Label { + line-height: var(--text-body-lineHeight-small); + } + &:not([data-no-visuals]) .Label { text-underline-offset: 0.125rem; } diff --git a/packages/react/src/Button/ButtonBase.tsx b/packages/react/src/Button/ButtonBase.tsx index d4747b94d4d..23b48d09157 100644 --- a/packages/react/src/Button/ButtonBase.tsx +++ b/packages/react/src/Button/ButtonBase.tsx @@ -111,7 +111,7 @@ const ButtonBase = forwardRef(({children, as: Component = 'button', ...props}, f {...rest} // @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent ref={mergedRef} - className={clsx(classes.ButtonBase, className)} + className={clsx(classes.ButtonBase, LeadingVisual && classes.HasLeadingVisual, className)} data-block={block ? 'block' : null} data-inactive={inactive ? true : undefined} data-loading={Boolean(loading)} diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index e95eb5d614d..61e6233e416 100644 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ b/packages/react/src/Button/__tests__/Button.test.tsx @@ -7,6 +7,7 @@ import {IconButton, Button, LinkButton} from '../../Button' import classes from '../ButtonBase.module.css' import {implementsClassName} from '../../utils/testing' import {FeatureFlags} from '../../FeatureFlags' +import {KeybindingHint} from '../../KeybindingHint' type StatefulLoadingButtonProps = { children?: React.ReactNode @@ -166,6 +167,96 @@ describe('Button', () => { expect(button.element()).toMatchSnapshot() }) + describe.each([ + {size: 'small', padding: '8px', leadingPadding: '6px', height: '28px', lineHeight: '18px'}, + {size: 'medium', padding: '12px', leadingPadding: '8px', height: '32px', lineHeight: '20px'}, + {size: 'large', padding: '16px', leadingPadding: '12px', height: '40px', lineHeight: '20px'}, + ] as const)('$size visual spacing', ({size, padding, leadingPadding, height, lineHeight}) => { + it.each([ + {leading: false, trailing: false}, + {leading: true, trailing: false}, + {leading: false, trailing: true}, + {leading: true, trailing: true}, + ])('spaces buttons with leading=$leading and trailing=$trailing', async ({leading, trailing}) => { + render( + , + ) + + const button = page.getByRole('button', {name: 'Search', exact: true}) + await expect + .poll(() => getComputedStyle(button.element()).paddingInlineStart) + .toBe(leading ? leadingPadding : padding) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) + expect(getComputedStyle(button.element()).height).toBe(height) + expect(getComputedStyle(page.getByText('Search', {exact: true}).element()).lineHeight).toBe(lineHeight) + + const label = page.getByText('Search', {exact: true}).element().getBoundingClientRect() + const icons = button.element().querySelectorAll('svg') + if (leading) { + expect(label.left - icons[0].getBoundingClientRect().right).toBe(6) + } + if (trailing) { + expect(icons[icons.length - 1].getBoundingClientRect().left - label.right).toBe(6) + } + }) + + it('preserves inline Link height while applying leading visual padding', async () => { + render( +
+ +
, + ) + const button = page.getByRole('button', {name: 'Search', exact: true}) + await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(leadingPadding) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) + expect(button.element().getBoundingClientRect().height).toBeLessThan(Number.parseFloat(height)) + const labelStyle = getComputedStyle(page.getByText('Search', {exact: true}).element()) + expect(labelStyle.fontSize).toBe('16px') + expect(labelStyle.lineHeight).toBe(size === 'small' ? '26px' : '24px') + }) + }) + + it.each([ + {size: 'small', padding: '4px'}, + {size: 'medium', padding: '8px'}, + {size: 'large', padding: '12px'}, + ] as const)('preserves $size icon-only counter padding', async ({size, padding}) => { + render(, + ) + const button = page.getByRole('button', {name: /^Save/}) + await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) + rerender( + , + ) + await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) + }) + it('styles primary button appropriately', () => { render( , - ) - - const button = page.getByRole('button', {name: 'Search', exact: true}) - await expect - .poll(() => getComputedStyle(button.element()).paddingInlineStart) - .toBe(leading ? leadingPadding : padding) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) - expect(getComputedStyle(button.element()).height).toBe(height) - expect(getComputedStyle(page.getByText('Search', {exact: true}).element()).lineHeight).toBe(lineHeight) - - const label = page.getByText('Search', {exact: true}).element().getBoundingClientRect() - const icons = button.element().querySelectorAll('svg') - if (leading) { - expect(label.left - icons[0].getBoundingClientRect().right).toBe(6) - } - if (trailing) { - expect(icons[icons.length - 1].getBoundingClientRect().left - label.right).toBe(6) - } - }) - - it('preserves inline Link height while applying leading visual padding', async () => { - render( -
- -
, - ) - const button = page.getByRole('button', {name: 'Search', exact: true}) - await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(leadingPadding) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) - expect(button.element().getBoundingClientRect().height).toBeLessThan(Number.parseFloat(height)) - const labelStyle = getComputedStyle(page.getByText('Search', {exact: true}).element()) - expect(labelStyle.fontSize).toBe('16px') - expect(labelStyle.lineHeight).toBe(size === 'small' ? '26px' : '24px') - }) - }) - - it.each([ - {size: 'small', padding: '4px'}, - {size: 'medium', padding: '8px'}, - {size: 'large', padding: '12px'}, - ] as const)('preserves $size icon-only counter padding', async ({size, padding}) => { - render(, - ) - const button = page.getByRole('button', {name: /^Save/}) - await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) - rerender( - , - ) - await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) - }) - it('styles primary button appropriately', () => { render( , + ) + + const button = page.getByRole('button', {name: 'Search', exact: true}) + await expect + .poll(() => getComputedStyle(button.element()).paddingInlineStart) + .toBe(leading ? leadingPadding : padding) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) + expect(getComputedStyle(button.element()).height).toBe(height) + expect(getComputedStyle(page.getByText('Search', {exact: true}).element()).lineHeight).toBe(lineHeight) + + const label = page.getByText('Search', {exact: true}).element().getBoundingClientRect() + const icons = button.element().querySelectorAll('svg') + if (leading) { + expect(label.left - icons[0].getBoundingClientRect().right).toBe(6) + } + if (trailing) { + expect(icons[icons.length - 1].getBoundingClientRect().left - label.right).toBe(6) + } + }) + + it('preserves inline Link height while applying leading visual padding', async () => { + render( +
+ +
, + ) + const button = page.getByRole('button', {name: 'Search', exact: true}) + await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(leadingPadding) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) + expect(button.element().getBoundingClientRect().height).toBeLessThan(Number.parseFloat(height)) + const labelStyle = getComputedStyle(page.getByText('Search', {exact: true}).element()) + expect(labelStyle.fontSize).toBe('16px') + expect(labelStyle.lineHeight).toBe(size === 'small' ? '26px' : '24px') + }) + }) + + it.each([ + {size: 'small', padding: '4px'}, + {size: 'medium', padding: '8px'}, + {size: 'large', padding: '12px'}, + ] as const)('preserves $size icon-only counter padding', async ({size, padding}) => { + render(, + ) + const button = page.getByRole('button', {name: /^Save/}) + await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) + rerender( + , + ) + await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) + expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) + }) + it('styles primary button appropriately', () => { render( export const Link = () => +export const AllSizes = () => ( +
+ + + +
+) + +const kitchenSinkVariants = ['default', 'primary', 'danger', 'invisible', 'link'] as const +const kitchenSinkSizes = ['small', 'medium', 'large'] as const + +export const AllVariants = () => ( + + {kitchenSinkVariants.map(variant => ( + + ))} + +) + +export const AllVariantsWithLeadingVisual = () => ( + + {kitchenSinkVariants.map(variant => ( + + ))} + +) + +export const KitchenSink = { + parameters: { + layout: 'fullscreen', + }, + render: () => ( +
+ + {kitchenSinkSizes.map(size => ( + + + {size} + + + + + Variants + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + With leading visual + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + With trailing visual + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + With both visuals + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + Disabled + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + Inactive + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + With key shortcut + + + {kitchenSinkVariants.map(variant => ( + + ))} + + + + + Block + + + + + + + + + ))} + +
+ ), +} + export const LeadingVisual = () => export const TrailingVisual = () => From 2ec907b584870e02c946f36c1110dcae4b71282a Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 10 Nov 2026 15:14:14 +0100 Subject: [PATCH 07/16] Button: match radius and font weight Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/button-modern-spacing.md | 2 +- .../Button/ButtonBase.modernize.module.css | 6 +++ .../src/Button/__tests__/Button.test.tsx | 37 +++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/.changeset/button-modern-spacing.md b/.changeset/button-modern-spacing.md index d88f293563d..3ab9e0fc1eb 100644 --- a/.changeset/button-modern-spacing.md +++ b/.changeset/button-modern-spacing.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -Button: Align visual spacing, keyboard shortcut padding, and label line height across button sizes. +Button: Align visual spacing, keyboard shortcut padding, label line height, weight, and border radius across button sizes. diff --git a/packages/react/src/Button/ButtonBase.modernize.module.css b/packages/react/src/Button/ButtonBase.modernize.module.css index 28989a5a2bc..79979d7d5e9 100644 --- a/packages/react/src/Button/ButtonBase.modernize.module.css +++ b/packages/react/src/Button/ButtonBase.modernize.module.css @@ -3,6 +3,12 @@ margin-right: var(--base-size-6); } + &:where(:not([data-component='IconButton'])) { + font-weight: var(--base-text-weight-semibold); + /* stylelint-disable-next-line primer/borders -- GitHub App's 8px radius has no matching Primer token */ + border-radius: 8px; + } + &:where(:not([data-variant='link'])) [data-component='text'] { /* stylelint-disable-next-line primer/typography -- match the 20px control label line height */ line-height: var(--base-size-20); diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index 61e6233e416..6bb5fb5b9f3 100644 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ b/packages/react/src/Button/__tests__/Button.test.tsx @@ -172,6 +172,35 @@ describe('Button', () => { {size: 'medium', padding: '12px', leadingPadding: '8px', height: '32px', lineHeight: '20px'}, {size: 'large', padding: '16px', leadingPadding: '12px', height: '40px', lineHeight: '20px'}, ] as const)('$size visual spacing', ({size, padding, leadingPadding, height, lineHeight}) => { + it.each(['default', 'primary', 'danger', 'invisible', 'link'] as const)( + 'uses semibold text and an 8px radius for the %s variant', + async variant => { + render( + , + ) + const button = page.getByRole('button', {name: 'Label', exact: true}) + const label = page.getByText('Label', {exact: true}) + await expect.poll(() => getComputedStyle(button.element()).fontWeight).toBe('600') + await expect.poll(() => getComputedStyle(label.element()).fontWeight).toBe('600') + expect(getComputedStyle(button.element()).borderRadius).toBe('8px') + }, + ) + + it('does not apply the label weight to shortcut keycaps', async () => { + render( + , + ) + const button = page.getByRole('button', {name: /^Save/}) + await expect + .poll(() => getComputedStyle(button.element().querySelector('[data-component="text"]')!).fontWeight) + .toBe('600') + expect(getComputedStyle(button.element().querySelector('[data-kbd-chord]')!).fontWeight).toBe('400') + }) + it.each([ {leading: false, trailing: false}, {leading: true, trailing: false}, @@ -224,6 +253,14 @@ describe('Button', () => { }) }) + it.each(['small', 'medium', 'large'] as const)('does not change %s IconButton styles', async size => { + render() + + const button = page.getByRole('button', {name: 'Search', exact: true}) + await expect.poll(() => getComputedStyle(button.element()).fontWeight).toBe('500') + expect(getComputedStyle(button.element()).borderRadius).toBe('6px') + }) + it.each([ {size: 'small', padding: '4px'}, {size: 'medium', padding: '8px'}, From aa781565fc28fab9118432a66e8a6264176607a0 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 10 Nov 2026 15:14:23 +0100 Subject: [PATCH 08/16] Ignore Playwright MCP artifacts Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 78cc2319772..b96272f69a7 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ storybook-static coverage/ # Playwright +.playwright-mcp/ stats.html blob-report results.json From 4d7f9960ab39834442d2807b84a1ad08b1e7e8b4 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 10 Nov 2026 15:26:53 +0100 Subject: [PATCH 09/16] Button: apply antialiased font smoothing Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/button-modern-spacing.md | 2 +- .../src/Button/ButtonBase.modernize.module.css | 1 + .../react/src/Button/__tests__/Button.test.tsx | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/.changeset/button-modern-spacing.md b/.changeset/button-modern-spacing.md index 3ab9e0fc1eb..ac3b109e0c5 100644 --- a/.changeset/button-modern-spacing.md +++ b/.changeset/button-modern-spacing.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -Button: Align visual spacing, keyboard shortcut padding, label line height, weight, and border radius across button sizes. +Button: Align visual spacing, keyboard shortcut padding, label typography, and border radius across button sizes. diff --git a/packages/react/src/Button/ButtonBase.modernize.module.css b/packages/react/src/Button/ButtonBase.modernize.module.css index 79979d7d5e9..bbe95773817 100644 --- a/packages/react/src/Button/ButtonBase.modernize.module.css +++ b/packages/react/src/Button/ButtonBase.modernize.module.css @@ -5,6 +5,7 @@ &:where(:not([data-component='IconButton'])) { font-weight: var(--base-text-weight-semibold); + -webkit-font-smoothing: antialiased; /* stylelint-disable-next-line primer/borders -- GitHub App's 8px radius has no matching Primer token */ border-radius: 8px; } diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index 6bb5fb5b9f3..71ea908a6bb 100644 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ b/packages/react/src/Button/__tests__/Button.test.tsx @@ -195,10 +195,23 @@ describe('Button', () => { , ) const button = page.getByRole('button', {name: /^Save/}) + await expect + .poll(() => getComputedStyle(button.element()).getPropertyValue('-webkit-font-smoothing')) + .toBe('antialiased') await expect .poll(() => getComputedStyle(button.element().querySelector('[data-component="text"]')!).fontWeight) .toBe('600') + expect( + getComputedStyle(button.element().querySelector('[data-component="text"]')!).getPropertyValue( + '-webkit-font-smoothing', + ), + ).toBe('antialiased') expect(getComputedStyle(button.element().querySelector('[data-kbd-chord]')!).fontWeight).toBe('400') + expect( + getComputedStyle(button.element().querySelector('[data-kbd-chord]')!).getPropertyValue( + '-webkit-font-smoothing', + ), + ).toBe('antialiased') }) it.each([ @@ -259,6 +272,7 @@ describe('Button', () => { const button = page.getByRole('button', {name: 'Search', exact: true}) await expect.poll(() => getComputedStyle(button.element()).fontWeight).toBe('500') expect(getComputedStyle(button.element()).borderRadius).toBe('6px') + expect(getComputedStyle(button.element()).getPropertyValue('-webkit-font-smoothing')).not.toBe('antialiased') }) it.each([ From df9a086fc0834a9267d4962c14b96727db16e047 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 10 Nov 2026 15:47:58 +0100 Subject: [PATCH 10/16] Delete .changeset/button-modern-spacing.md --- .changeset/button-modern-spacing.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/button-modern-spacing.md diff --git a/.changeset/button-modern-spacing.md b/.changeset/button-modern-spacing.md deleted file mode 100644 index ac3b109e0c5..00000000000 --- a/.changeset/button-modern-spacing.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@primer/react': patch ---- - -Button: Align visual spacing, keyboard shortcut padding, label typography, and border radius across button sizes. From 46fe83eee883aa90515048baa6bace0109713bf8 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Tue, 10 Nov 2026 15:49:07 +0100 Subject: [PATCH 11/16] Remove Button component test file Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Button/__tests__/Button.test.tsx | 853 ------------------ 1 file changed, 853 deletions(-) delete mode 100644 packages/react/src/Button/__tests__/Button.test.tsx diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx deleted file mode 100644 index 71ea908a6bb..00000000000 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ /dev/null @@ -1,853 +0,0 @@ -import {page, userEvent} from 'vitest/browser' -import {SearchIcon, HeartIcon} from '@primer/octicons-react' -import {render, fireEvent} from '@testing-library/react' -import {describe, it, expect, vi} from 'vitest' -import React from 'react' -import {IconButton, Button, LinkButton} from '../../Button' -import classes from '../ButtonBase.module.css' -import {implementsClassName} from '../../utils/testing' -import {FeatureFlags} from '../../FeatureFlags' -import {KeybindingHint} from '../../KeybindingHint' - -type StatefulLoadingButtonProps = { - children?: React.ReactNode - id?: string - ['aria-describedby']?: string - loadingAnnouncement?: string -} - -const StatefulLoadingButton = (props: StatefulLoadingButtonProps) => { - const [isLoading, setIsLoading] = React.useState(false) - const handleClick = () => { - setIsLoading(true) - } - - return ) - const button = page.getByRole('button') - expect(button.element().textContent).toEqual('Default') - }) - - it('preserves "onClick" prop', async () => { - const onClick = vi.fn() - render() - const button = page.getByRole('button') - await userEvent.click(button) - expect(onClick).toHaveBeenCalledTimes(1) - }) - - it('respects block prop', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - it('displays a notification indicator on the button boundary', async () => { - render() - await expect.element(page.getByRole('button')).toHaveAttribute('data-notification-indicator', 'button') - }) - - it('displays a notification indicator on the leading visual', async () => { - render( - , - ) - - await expect.element(page.getByRole('button')).toHaveAttribute('data-notification-indicator', 'leadingVisual') - }) - - it('displays a notification indicator on an icon button icon', async () => { - render() - - await expect.element(page.getByRole('button')).toHaveAttribute('data-notification-indicator', 'icon') - }) - - it('warns when a leading visual notification indicator has no leading visual', () => { - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - - render() - - expect(consoleSpy).toHaveBeenCalledWith( - 'Button: `notificationIndicator="leadingVisual"` requires a `leadingVisual` prop.', - ) - consoleSpy.mockRestore() - }) - - it('respects the "disabled" prop', () => { - const onClick = vi.fn() - - render( - , - ) - - const button = page.getByRole('button') - expect(button.element().hasAttribute('disabled')).toEqual(true) - fireEvent.click(button.element()) - expect(onClick).toHaveBeenCalledTimes(0) - }) - - it('respects the small size prop', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - it('respects the large size prop', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - describe.each([ - {size: 'small', padding: '8px', leadingPadding: '6px', height: '28px', lineHeight: '18px'}, - {size: 'medium', padding: '12px', leadingPadding: '8px', height: '32px', lineHeight: '20px'}, - {size: 'large', padding: '16px', leadingPadding: '12px', height: '40px', lineHeight: '20px'}, - ] as const)('$size visual spacing', ({size, padding, leadingPadding, height, lineHeight}) => { - it.each(['default', 'primary', 'danger', 'invisible', 'link'] as const)( - 'uses semibold text and an 8px radius for the %s variant', - async variant => { - render( - , - ) - const button = page.getByRole('button', {name: 'Label', exact: true}) - const label = page.getByText('Label', {exact: true}) - await expect.poll(() => getComputedStyle(button.element()).fontWeight).toBe('600') - await expect.poll(() => getComputedStyle(label.element()).fontWeight).toBe('600') - expect(getComputedStyle(button.element()).borderRadius).toBe('8px') - }, - ) - - it('does not apply the label weight to shortcut keycaps', async () => { - render( - , - ) - const button = page.getByRole('button', {name: /^Save/}) - await expect - .poll(() => getComputedStyle(button.element()).getPropertyValue('-webkit-font-smoothing')) - .toBe('antialiased') - await expect - .poll(() => getComputedStyle(button.element().querySelector('[data-component="text"]')!).fontWeight) - .toBe('600') - expect( - getComputedStyle(button.element().querySelector('[data-component="text"]')!).getPropertyValue( - '-webkit-font-smoothing', - ), - ).toBe('antialiased') - expect(getComputedStyle(button.element().querySelector('[data-kbd-chord]')!).fontWeight).toBe('400') - expect( - getComputedStyle(button.element().querySelector('[data-kbd-chord]')!).getPropertyValue( - '-webkit-font-smoothing', - ), - ).toBe('antialiased') - }) - - it.each([ - {leading: false, trailing: false}, - {leading: true, trailing: false}, - {leading: false, trailing: true}, - {leading: true, trailing: true}, - ])('spaces buttons with leading=$leading and trailing=$trailing', async ({leading, trailing}) => { - render( - , - ) - - const button = page.getByRole('button', {name: 'Search', exact: true}) - await expect - .poll(() => getComputedStyle(button.element()).paddingInlineStart) - .toBe(leading ? leadingPadding : padding) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) - expect(getComputedStyle(button.element()).height).toBe(height) - expect(getComputedStyle(page.getByText('Search', {exact: true}).element()).lineHeight).toBe(lineHeight) - - const label = page.getByText('Search', {exact: true}).element().getBoundingClientRect() - const icons = button.element().querySelectorAll('svg') - if (leading) { - expect(label.left - icons[0].getBoundingClientRect().right).toBe(6) - } - if (trailing) { - expect(icons[icons.length - 1].getBoundingClientRect().left - label.right).toBe(6) - } - }) - - it('preserves inline Link height while applying leading visual padding', async () => { - render( -
- -
, - ) - const button = page.getByRole('button', {name: 'Search', exact: true}) - await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(leadingPadding) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(padding) - expect(button.element().getBoundingClientRect().height).toBeLessThan(Number.parseFloat(height)) - const labelStyle = getComputedStyle(page.getByText('Search', {exact: true}).element()) - expect(labelStyle.fontSize).toBe('16px') - expect(labelStyle.lineHeight).toBe(size === 'small' ? '26px' : '24px') - }) - }) - - it.each(['small', 'medium', 'large'] as const)('does not change %s IconButton styles', async size => { - render() - - const button = page.getByRole('button', {name: 'Search', exact: true}) - await expect.poll(() => getComputedStyle(button.element()).fontWeight).toBe('500') - expect(getComputedStyle(button.element()).borderRadius).toBe('6px') - expect(getComputedStyle(button.element()).getPropertyValue('-webkit-font-smoothing')).not.toBe('antialiased') - }) - - it.each([ - {size: 'small', padding: '4px'}, - {size: 'medium', padding: '8px'}, - {size: 'large', padding: '12px'}, - ] as const)('preserves $size icon-only counter padding', async ({size, padding}) => { - render(, - ) - const button = page.getByRole('button', {name: /^Save/}) - await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) - rerender( - , - ) - await expect.poll(() => getComputedStyle(button.element()).paddingInlineStart).toBe(start) - expect(getComputedStyle(button.element()).paddingInlineEnd).toBe(end) - }) - - it('styles primary button appropriately', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - it('styles invisible button appropriately', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - it('styles danger button appropriately', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - it('makes sure icon button has an aria-label', async () => { - render() - const IconOnlyButton = page.getByLabelText('Search button', { - exact: true, - }) - await expect.element(IconOnlyButton).toBeInTheDocument() - }) - - it('respects the alignContent prop', () => { - render( - , - ) - - const button = page.getByRole('button') - expect(button.element()).toMatchSnapshot() - }) - - it('should render the leadingVisual prop before the button content', async () => { - render( - , - ) - - await expect.element(page.getByTestId('leadingVisual')).toBeInTheDocument() - - const position = page - .getByText('content', { - exact: true, - }) - .element() - .compareDocumentPosition(page.getByTestId('leadingVisual').element()) - expect(position).toBe(Node.DOCUMENT_POSITION_PRECEDING) - }) - - it('should render the trailingVisual prop after the button content', async () => { - render( - , - ) - await expect.element(page.getByTestId('trailingVisual')).toBeInTheDocument() - - const position = page - .getByText('content', { - exact: true, - }) - .element() - .compareDocumentPosition(page.getByTestId('trailingVisual').element()) - expect(position).toBe(Node.DOCUMENT_POSITION_FOLLOWING) - }) - - it('should describe the button with a default loading announcement, and only when the button is in a loading state', async () => { - const buttonId = 'loading-button' - - render( - - content - , - ) - - const buttonNode = page.getByRole('button') - - await userEvent.click(buttonNode) - - await expect.element(buttonNode).toHaveAttribute('aria-describedby', `${buttonId}-loading-announcement`) - - fireEvent.click(buttonNode.element()) - - await expect.poll(() => document.getElementById(`${buttonId}-loading-announcement`)?.textContent).toBe('Loading') - }) - - it('should render a custom loading announcement, and only when the button is in a loading state', async () => { - const buttonId = 'loading-button' - - render( - - content - , - ) - - const buttonNode = page.getByRole('button') - - await userEvent.click(buttonNode) - - await expect.element(buttonNode).toHaveAttribute('aria-describedby', `${buttonId}-loading-announcement`) - - fireEvent.click(buttonNode.element()) - - await expect - .poll(() => document.getElementById(`${buttonId}-loading-announcement`)?.textContent) - .toBe('Action loading') - }) - - it('should be described by loading announcement AND whatever is passed to aria-describedby', async () => { - const buttonDescriptionId = 'button-description' - const buttonId = 'loading-button' - - render( - - content - , - ) - - const buttonNode = page.getByRole('button') - - await userEvent.click(buttonNode) - - const buttonDescribedBy = page.getByRole('button').element().getAttribute('aria-describedby') - const loadingAnnouncementId = `${buttonId}-loading-announcement` - - expect(buttonDescribedBy).toContain(loadingAnnouncementId) - - expect(buttonDescribedBy).toContain(buttonDescriptionId) - }) - - it('should only set aria-disabled to "true" when the button is in a loading state', async () => { - render( - - content - , - ) - - const buttonNode = page.getByRole('button') - - expect(buttonNode.element().getAttribute('aria-disabled')).not.toBe('true') - - await userEvent.click(buttonNode) - - await expect.element(buttonNode).toHaveAttribute('aria-disabled', 'true') - }) - - it('allows the consumer to override `aria-disabled`', async () => { - render() - - await expect.element(page.getByRole('button')).toHaveAttribute('aria-disabled', 'true') - }) - - describe('accessible loading disabled styles', () => { - const visualStyles = (element: Element) => { - const {backgroundColor, borderColor, color, cursor} = getComputedStyle(element) - return {backgroundColor, borderColor, color, cursor} - } - - const renderWithLoadingDisabledStyles = (children: React.ReactNode) => - render({children}) - - it('applies disabled styles to a loading primary button without using native disabled', async () => { - renderWithLoadingDisabledStyles( - <> - - - - , - ) - - const loadingButton = page.getByRole('button', { - name: 'Loading', - exact: true, - }) - const disabledButton = page.getByRole('button', { - name: 'Disabled', - exact: true, - }) - const enabledButton = page.getByRole('button', { - name: 'Enabled', - exact: true, - }) - - await expect.element(loadingButton).toHaveAttribute('aria-disabled', 'true') - await expect.element(loadingButton).toHaveAttribute('data-loading', 'true') - await expect.element(loadingButton).toHaveAttribute('data-loading-disabled-styles', 'true') - await expect.element(loadingButton).not.toBeDisabled() - expect(visualStyles(loadingButton.element())).toEqual(visualStyles(disabledButton.element())) - expect(visualStyles(loadingButton.element()).backgroundColor).not.toBe( - visualStyles(enabledButton.element()).backgroundColor, - ) - expect(visualStyles(loadingButton.element()).cursor).toBe('not-allowed') - }) - - it('applies disabled styles when loading is combined with aria-disabled', async () => { - renderWithLoadingDisabledStyles( - <> - - - , - ) - - const loadingButton = page.getByRole('button', { - name: 'Loading', - exact: true, - }) - const disabledButton = page.getByRole('button', { - name: 'Disabled', - exact: true, - }) - - await expect.element(loadingButton).toHaveAttribute('aria-disabled', 'true') - await expect.element(loadingButton).not.toBeDisabled() - expect(visualStyles(loadingButton.element())).toEqual(visualStyles(disabledButton.element())) - }) - - it('preserves existing loading styles when the feature flag is disabled', async () => { - render( - <> - - - , - ) - - const loadingButton = page.getByRole('button', { - name: 'Loading', - exact: true, - }) - const disabledButton = page.getByRole('button', { - name: 'Disabled', - exact: true, - }) - - await expect.element(loadingButton).not.toHaveAttribute('data-loading-disabled-styles') - expect(visualStyles(loadingButton.element())).not.toEqual(visualStyles(disabledButton.element())) - }) - - it('does not call onClick while loading', () => { - const onClick = vi.fn() - render( - , - ) - - fireEvent.click( - page - .getByRole('button', { - name: 'Submit', - exact: true, - }) - .element(), - ) - expect(onClick).not.toHaveBeenCalled() - }) - - it('remains focusable while loading', async () => { - render( - , - ) - - const button = page.getByRole('button', { - name: 'Submit', - exact: true, - }) - button.element().focus() - expect(button.element()).toHaveFocus() - }) - - it('still applies disabled styles for aria-disabled without loading', () => { - render( - <> - - - , - ) - - expect( - visualStyles( - page - .getByRole('button', { - name: 'Aria disabled', - exact: true, - }) - .element(), - ), - ).toEqual( - visualStyles( - page - .getByRole('button', { - name: 'Disabled', - exact: true, - }) - .element(), - ), - ) - }) - - it('does not let inactive override primary the same way disabled styles do', () => { - render( - <> - - - , - ) - - expect( - visualStyles( - page - .getByRole('button', { - name: 'Inactive', - exact: true, - }) - .element(), - ), - ).not.toEqual( - visualStyles( - page - .getByRole('button', { - name: 'Disabled', - exact: true, - }) - .element(), - ), - ) - }) - }) - - it('should preserve the accessible button name when the button is in a loading state', async () => { - render() - - await expect.element(page.getByRole('button')).toHaveAccessibleName('content') - }) - - it('should apply inline-flex display to loading wrapper when variant is link', async () => { - const {container} = render( - , - ) - - const wrapper = container.querySelector('[data-loading-wrapper]') - await expect.element(wrapper).toBeInTheDocument() - await expect.element(wrapper).toHaveClass(classes.ConditionalWrapperLink) - }) - - it('should render tooltip on an icon button when unsafeDisableTooltip prop is passed as false', async () => { - render() - const triggerEL = page.getByRole('button') - const tooltipEl = page.getByText('Heart', { - exact: true, - }) - await expect.element(triggerEL).toHaveAttribute('aria-labelledby', tooltipEl.element().id) - }) - it('should render description type tooltip on an icon button when unsafeDisableTooltip prop is passed as false', async () => { - render() - - const triggerEL = page.getByRole('button') - await expect.element(triggerEL).toHaveAttribute('aria-label', 'Heart') - const tooltipEl = page.getByText('Love is all around', { - exact: true, - }) - expect(triggerEL.element().getAttribute('aria-describedby')).toContain(tooltipEl.element().id) - }) - it('should render tooltip on an icon button by default', async () => { - render() - const triggerEl = page.getByRole('button') - const tooltipEl = page.getByText('Heart', { - exact: true, - }) - await expect.element(triggerEl).toHaveAttribute('aria-labelledby', tooltipEl.element().id) - await expect.element(triggerEl).not.toHaveAttribute('aria-label') - }) - it('should render aria-keyshorts on an icon button when keyshortcuts prop is passed', async () => { - render() - const triggerEl = page.getByRole('button') - await expect.element(triggerEl).toHaveAttribute('aria-keyshortcuts', 'Command+H') - }) - it('should append the keyshortcuts to the tooltip text that labels the icon button when keyshortcuts prop is passed', async () => { - render() - const triggerEl = page.getByRole('button', { - name: 'Heart (command h)', - exact: true, - }) - await expect.element(triggerEl).toBeInTheDocument() - }) - it('should render aria-keyshortcuts on an icon button when keyshortcuts prop is passed (Description Type)', async () => { - render() - - const triggerEl = page.getByRole('button') - await expect.element(triggerEl).toHaveAttribute('aria-keyshortcuts', 'Command+H') - }) - it('should append the keyshortcuts to the tooltip text that describes the icon button when keyshortcuts prop is passed (Description Type)', async () => { - render() - - const triggerEl = page.getByRole('button', { - name: 'Heart', - exact: true, - }) - await expect.element(triggerEl).toHaveAccessibleDescription('Love is all around (command h)') - }) -}) - -describe('data-component attributes', () => { - describe('Button', () => { - it('should have data-component="Button" on the button element', async () => { - const {container} = render() - await expect.element(container.querySelector('[data-component="Button"]')).toBeInTheDocument() - }) - - it('should have data-component="buttonContent" on the content wrapper', async () => { - const {container} = render() - await expect.element(container.querySelector('[data-component="buttonContent"]')).toBeInTheDocument() - }) - - it('should have data-component="text" on the text element', async () => { - const {container} = render() - await expect.element(container.querySelector('[data-component="text"]')).toBeInTheDocument() - }) - - it('should have data-component="leadingVisual" on leading visual', async () => { - const {container} = render() - await expect.element(container.querySelector('[data-component="leadingVisual"]')).toBeInTheDocument() - }) - - it('should have data-component="trailingVisual" on trailing visual', async () => { - const {container} = render() - await expect - .element(container.querySelector('[data-component="trailingVisual"]')) - .toBeInTheDocument() - }) - - it('should have data-component="ButtonCounter" on count element', async () => { - const {container} = render() - await expect.element(container.querySelector('[data-component="ButtonCounter"]')).toBeInTheDocument() - }) - - it('should have data-component="loadingSpinner" when loading without visuals', async () => { - const {container} = render() - await expect - .element(container.querySelector('[data-component="loadingSpinner"]')) - .toBeInTheDocument() - }) - }) - - describe('data-icon-only-counter', () => { - it('is set when count, leadingVisual, and no children are present', async () => { - const {container} = render(, - ) - await expect - .element(container.querySelector('[data-component="Button"]')) - .not.toHaveAttribute('data-icon-only-counter') - }) - - it('is not set when leadingVisual is missing', async () => { - const {container} = render() + const button = page.getByRole('button') + expect(button.element().textContent).toEqual('Default') + }) + + it('preserves "onClick" prop', async () => { + const onClick = vi.fn() + render() + const button = page.getByRole('button') + await userEvent.click(button) + expect(onClick).toHaveBeenCalledTimes(1) + }) + + it('respects block prop', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('displays a notification indicator on the button boundary', async () => { + render() + await expect.element(page.getByRole('button')).toHaveAttribute('data-notification-indicator', 'button') + }) + + it('displays a notification indicator on the leading visual', async () => { + render( + , + ) + + await expect.element(page.getByRole('button')).toHaveAttribute('data-notification-indicator', 'leadingVisual') + }) + + it('displays a notification indicator on an icon button icon', async () => { + render() + + await expect.element(page.getByRole('button')).toHaveAttribute('data-notification-indicator', 'icon') + }) + + it('warns when a leading visual notification indicator has no leading visual', () => { + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + render() + + expect(consoleSpy).toHaveBeenCalledWith( + 'Button: `notificationIndicator="leadingVisual"` requires a `leadingVisual` prop.', + ) + consoleSpy.mockRestore() + }) + + it('respects the "disabled" prop', () => { + const onClick = vi.fn() + + render( + , + ) + + const button = page.getByRole('button') + expect(button.element().hasAttribute('disabled')).toEqual(true) + fireEvent.click(button.element()) + expect(onClick).toHaveBeenCalledTimes(0) + }) + + it('respects the small size prop', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('respects the large size prop', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('styles primary button appropriately', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('styles invisible button appropriately', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('styles danger button appropriately', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('makes sure icon button has an aria-label', async () => { + render() + const IconOnlyButton = page.getByLabelText('Search button', { + exact: true, + }) + await expect.element(IconOnlyButton).toBeInTheDocument() + }) + + it('respects the alignContent prop', () => { + render( + , + ) + + const button = page.getByRole('button') + expect(button.element()).toMatchSnapshot() + }) + + it('should render the leadingVisual prop before the button content', async () => { + render( + , + ) + + await expect.element(page.getByTestId('leadingVisual')).toBeInTheDocument() + + const position = page + .getByText('content', { + exact: true, + }) + .element() + .compareDocumentPosition(page.getByTestId('leadingVisual').element()) + expect(position).toBe(Node.DOCUMENT_POSITION_PRECEDING) + }) + + it('should render the trailingVisual prop after the button content', async () => { + render( + , + ) + await expect.element(page.getByTestId('trailingVisual')).toBeInTheDocument() + + const position = page + .getByText('content', { + exact: true, + }) + .element() + .compareDocumentPosition(page.getByTestId('trailingVisual').element()) + expect(position).toBe(Node.DOCUMENT_POSITION_FOLLOWING) + }) + + it('should describe the button with a default loading announcement, and only when the button is in a loading state', async () => { + const buttonId = 'loading-button' + + render( + + content + , + ) + + const buttonNode = page.getByRole('button') + + await userEvent.click(buttonNode) + + await expect.element(buttonNode).toHaveAttribute('aria-describedby', `${buttonId}-loading-announcement`) + + fireEvent.click(buttonNode.element()) + + await expect.poll(() => document.getElementById(`${buttonId}-loading-announcement`)?.textContent).toBe('Loading') + }) + + it('should render a custom loading announcement, and only when the button is in a loading state', async () => { + const buttonId = 'loading-button' + + render( + + content + , + ) + + const buttonNode = page.getByRole('button') + + await userEvent.click(buttonNode) + + await expect.element(buttonNode).toHaveAttribute('aria-describedby', `${buttonId}-loading-announcement`) + + fireEvent.click(buttonNode.element()) + + await expect + .poll(() => document.getElementById(`${buttonId}-loading-announcement`)?.textContent) + .toBe('Action loading') + }) + + it('should be described by loading announcement AND whatever is passed to aria-describedby', async () => { + const buttonDescriptionId = 'button-description' + const buttonId = 'loading-button' + + render( + + content + , + ) + + const buttonNode = page.getByRole('button') + + await userEvent.click(buttonNode) + + const buttonDescribedBy = page.getByRole('button').element().getAttribute('aria-describedby') + const loadingAnnouncementId = `${buttonId}-loading-announcement` + + expect(buttonDescribedBy).toContain(loadingAnnouncementId) + + expect(buttonDescribedBy).toContain(buttonDescriptionId) + }) + + it('should only set aria-disabled to "true" when the button is in a loading state', async () => { + render( + + content + , + ) + + const buttonNode = page.getByRole('button') + + expect(buttonNode.element().getAttribute('aria-disabled')).not.toBe('true') + + await userEvent.click(buttonNode) + + await expect.element(buttonNode).toHaveAttribute('aria-disabled', 'true') + }) + + it('allows the consumer to override `aria-disabled`', async () => { + render() + + await expect.element(page.getByRole('button')).toHaveAttribute('aria-disabled', 'true') + }) + + describe('accessible loading disabled styles', () => { + const visualStyles = (element: Element) => { + const {backgroundColor, borderColor, color, cursor} = getComputedStyle(element) + return {backgroundColor, borderColor, color, cursor} + } + + const renderWithLoadingDisabledStyles = (children: React.ReactNode) => + render({children}) + + it('applies disabled styles to a loading primary button without using native disabled', async () => { + renderWithLoadingDisabledStyles( + <> + + + + , + ) + + const loadingButton = page.getByRole('button', { + name: 'Loading', + exact: true, + }) + const disabledButton = page.getByRole('button', { + name: 'Disabled', + exact: true, + }) + const enabledButton = page.getByRole('button', { + name: 'Enabled', + exact: true, + }) + + await expect.element(loadingButton).toHaveAttribute('aria-disabled', 'true') + await expect.element(loadingButton).toHaveAttribute('data-loading', 'true') + await expect.element(loadingButton).toHaveAttribute('data-loading-disabled-styles', 'true') + await expect.element(loadingButton).not.toBeDisabled() + expect(visualStyles(loadingButton.element())).toEqual(visualStyles(disabledButton.element())) + expect(visualStyles(loadingButton.element()).backgroundColor).not.toBe( + visualStyles(enabledButton.element()).backgroundColor, + ) + expect(visualStyles(loadingButton.element()).cursor).toBe('not-allowed') + }) + + it('applies disabled styles when loading is combined with aria-disabled', async () => { + renderWithLoadingDisabledStyles( + <> + + + , + ) + + const loadingButton = page.getByRole('button', { + name: 'Loading', + exact: true, + }) + const disabledButton = page.getByRole('button', { + name: 'Disabled', + exact: true, + }) + + await expect.element(loadingButton).toHaveAttribute('aria-disabled', 'true') + await expect.element(loadingButton).not.toBeDisabled() + expect(visualStyles(loadingButton.element())).toEqual(visualStyles(disabledButton.element())) + }) + + it('preserves existing loading styles when the feature flag is disabled', async () => { + render( + <> + + + , + ) + + const loadingButton = page.getByRole('button', { + name: 'Loading', + exact: true, + }) + const disabledButton = page.getByRole('button', { + name: 'Disabled', + exact: true, + }) + + await expect.element(loadingButton).not.toHaveAttribute('data-loading-disabled-styles') + expect(visualStyles(loadingButton.element())).not.toEqual(visualStyles(disabledButton.element())) + }) + + it('does not call onClick while loading', () => { + const onClick = vi.fn() + render( + , + ) + + fireEvent.click( + page + .getByRole('button', { + name: 'Submit', + exact: true, + }) + .element(), + ) + expect(onClick).not.toHaveBeenCalled() + }) + + it('remains focusable while loading', async () => { + render( + , + ) + + const button = page.getByRole('button', { + name: 'Submit', + exact: true, + }) + button.element().focus() + expect(button.element()).toHaveFocus() + }) + + it('still applies disabled styles for aria-disabled without loading', () => { + render( + <> + + + , + ) + + expect( + visualStyles( + page + .getByRole('button', { + name: 'Aria disabled', + exact: true, + }) + .element(), + ), + ).toEqual( + visualStyles( + page + .getByRole('button', { + name: 'Disabled', + exact: true, + }) + .element(), + ), + ) + }) + + it('does not let inactive override primary the same way disabled styles do', () => { + render( + <> + + + , + ) + + expect( + visualStyles( + page + .getByRole('button', { + name: 'Inactive', + exact: true, + }) + .element(), + ), + ).not.toEqual( + visualStyles( + page + .getByRole('button', { + name: 'Disabled', + exact: true, + }) + .element(), + ), + ) + }) + }) + + it('should preserve the accessible button name when the button is in a loading state', async () => { + render() + + await expect.element(page.getByRole('button')).toHaveAccessibleName('content') + }) + + it('should apply inline-flex display to loading wrapper when variant is link', async () => { + const {container} = render( + , + ) + + const wrapper = container.querySelector('[data-loading-wrapper]') + await expect.element(wrapper).toBeInTheDocument() + await expect.element(wrapper).toHaveClass(classes.ConditionalWrapperLink) + }) + + it('should render tooltip on an icon button when unsafeDisableTooltip prop is passed as false', async () => { + render() + const triggerEL = page.getByRole('button') + const tooltipEl = page.getByText('Heart', { + exact: true, + }) + await expect.element(triggerEL).toHaveAttribute('aria-labelledby', tooltipEl.element().id) + }) + it('should render description type tooltip on an icon button when unsafeDisableTooltip prop is passed as false', async () => { + render() + + const triggerEL = page.getByRole('button') + await expect.element(triggerEL).toHaveAttribute('aria-label', 'Heart') + const tooltipEl = page.getByText('Love is all around', { + exact: true, + }) + expect(triggerEL.element().getAttribute('aria-describedby')).toContain(tooltipEl.element().id) + }) + it('should render tooltip on an icon button by default', async () => { + render() + const triggerEl = page.getByRole('button') + const tooltipEl = page.getByText('Heart', { + exact: true, + }) + await expect.element(triggerEl).toHaveAttribute('aria-labelledby', tooltipEl.element().id) + await expect.element(triggerEl).not.toHaveAttribute('aria-label') + }) + it('should render aria-keyshorts on an icon button when keyshortcuts prop is passed', async () => { + render() + const triggerEl = page.getByRole('button') + await expect.element(triggerEl).toHaveAttribute('aria-keyshortcuts', 'Command+H') + }) + it('should append the keyshortcuts to the tooltip text that labels the icon button when keyshortcuts prop is passed', async () => { + render() + const triggerEl = page.getByRole('button', { + name: 'Heart (command h)', + exact: true, + }) + await expect.element(triggerEl).toBeInTheDocument() + }) + it('should render aria-keyshortcuts on an icon button when keyshortcuts prop is passed (Description Type)', async () => { + render() + + const triggerEl = page.getByRole('button') + await expect.element(triggerEl).toHaveAttribute('aria-keyshortcuts', 'Command+H') + }) + it('should append the keyshortcuts to the tooltip text that describes the icon button when keyshortcuts prop is passed (Description Type)', async () => { + render() + + const triggerEl = page.getByRole('button', { + name: 'Heart', + exact: true, + }) + await expect.element(triggerEl).toHaveAccessibleDescription('Love is all around (command h)') + }) +}) + +describe('data-component attributes', () => { + describe('Button', () => { + it('should have data-component="Button" on the button element', async () => { + const {container} = render() + await expect.element(container.querySelector('[data-component="Button"]')).toBeInTheDocument() + }) + + it('should have data-component="buttonContent" on the content wrapper', async () => { + const {container} = render() + await expect.element(container.querySelector('[data-component="buttonContent"]')).toBeInTheDocument() + }) + + it('should have data-component="text" on the text element', async () => { + const {container} = render() + await expect.element(container.querySelector('[data-component="text"]')).toBeInTheDocument() + }) + + it('should have data-component="leadingVisual" on leading visual', async () => { + const {container} = render() + await expect.element(container.querySelector('[data-component="leadingVisual"]')).toBeInTheDocument() + }) + + it('should have data-component="trailingVisual" on trailing visual', async () => { + const {container} = render() + await expect + .element(container.querySelector('[data-component="trailingVisual"]')) + .toBeInTheDocument() + }) + + it('should have data-component="ButtonCounter" on count element', async () => { + const {container} = render() + await expect.element(container.querySelector('[data-component="ButtonCounter"]')).toBeInTheDocument() + }) + + it('should have data-component="loadingSpinner" when loading without visuals', async () => { + const {container} = render() + await expect + .element(container.querySelector('[data-component="loadingSpinner"]')) + .toBeInTheDocument() + }) + }) + + describe('data-icon-only-counter', () => { + it('is set when count, leadingVisual, and no children are present', async () => { + const {container} = render(, + ) + await expect + .element(container.querySelector('[data-component="Button"]')) + .not.toHaveAttribute('data-icon-only-counter') + }) + + it('is not set when leadingVisual is missing', async () => { + const {container} = render( + + Standalone action + + + Actions + + + + Simple action + + + + + + + + + Search + Find a file + + + + + , + ) + + const trigger = page.getByRole('button', {name: 'Actions', exact: true}) + expect(getComputedStyle(trigger.element()).fontSize).toBe('13px') + const standaloneButton = page.getByRole('button', {name: 'Standalone button', exact: true}) + expect(getComputedStyle(standaloneButton.element()).fontSize).toBe('14px') + await userEvent.click(trigger) + + const simpleItem = page.getByRole('menuitem', {name: /^Simple action/}) + const descriptionItem = page.getByRole('menuitem', {name: 'Search', exact: true}) + await expect.element(descriptionItem).toBeVisible() + + expect(simpleItem.element().getBoundingClientRect().height).toBeCloseTo(28, 3) + expect(descriptionItem.element().getBoundingClientRect().height).toBeCloseTo(48, 3) + expect(descriptionItem.element().querySelector('svg')?.getBoundingClientRect().width).toBeCloseTo(14, 3) + const label = descriptionItem.element().querySelector('[data-component="ActionList.Item.Label"]')! + expect(getComputedStyle(label).fontSize).toBe('13px') + expect(getComputedStyle(label).fontWeight).toBe('500') + expect(getComputedStyle(label).lineHeight).toBe('20px') + + const standaloneItem = page.getByRole('button', {name: 'Standalone action', exact: true}) + expect(standaloneItem.element().getBoundingClientRect().height).toBeCloseTo(32, 3) + }) + it('renders data-component attributes for ActionMenu parts', async () => { const component = HTMLRender() diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx index c33997690f5..6e7a6449031 100644 --- a/packages/react/src/ActionMenu/ActionMenu.tsx +++ b/packages/react/src/ActionMenu/ActionMenu.tsx @@ -15,6 +15,8 @@ import type {MandateProps} from '../utils/types' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {Tooltip} from '../TooltipV2/Tooltip' import styles from './ActionMenu.module.css' +import menuModernizeClasses from './ActionMenu.modernize.module.css' +import modernizeClasses from '../ActionList/ActionList.modernize.module.css' import {useResponsiveValue, type ResponsiveValue} from '../hooks/useResponsiveValue' import {isSlot} from '../utils/is-slot' import type {FCWithSlotMarker, WithSlotMarker} from '../utils/types/Slots' @@ -254,10 +256,16 @@ const Anchor: WithSlotMarker< /** this component is syntactical sugar 🍭 */ export type ActionMenuButtonProps = ButtonProps -const MenuButton = React.forwardRef(({...props}, anchorRef) => { +const MenuButton = React.forwardRef(({className, ...props}, anchorRef) => { return ( -