Skip to content

Commit

Permalink
Use the Text component to apply Button text styles (Shopify#11735)
Browse files Browse the repository at this point in the history
### WHY are these changes introduced?

Cleans up the button CSS by re-using the Text component styles to apply
the proper `Button` typography.

This PR also address some test failures:

- `Button.test.tsx`: The `matchMedia` mock needed to be cleared
- `TrapFocus.test.tsx`: The `Button` external element was being rendered
outside of the test, avoiding the global `beforeEach`/`afterEach` test
setup which mocks `matchMedia` needed by `Button`.

> [!NOTE]  
> Depends on Shopify#11724

Related to https://github.com/Shopify/polaris-internal/issues/1483
  • Loading branch information
sam-b-rose authored Mar 15, 2024
1 parent 409c127 commit a30cc57
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 36 deletions.
5 changes: 5 additions & 0 deletions .changeset/ten-squids-nail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': patch
---

Used `Text` component to apply text styles for `Button`
17 changes: 1 addition & 16 deletions polaris-react/src/components/Button/Button.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,9 @@
border-radius: var(--p-border-radius-200);
box-shadow: var(--pc-button-box-shadow);
color: var(--pc-button-color);

font-size: var(--p-font-size-325);
font-weight: var(--p-font-weight-medium);
line-height: var(--p-font-line-height-500);

cursor: pointer;
user-select: none;
-webkit-tap-highlight-color: transparent;

@media (--p-breakpoints-md-up) {
font-size: var(--p-font-size-300);
line-height: var(--p-font-line-height-400);
}
}

/* https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity#increasing_specificity_by_duplicating_selector
Expand Down Expand Up @@ -166,7 +156,7 @@
--pc-button-color_active: var(--p-color-text-link-active);
}

.variantPlain:is(:hover, :active, :focus-visible) {
.variantPlain:is(:hover, :active, :focus-visible):not(.removeUnderline) {
text-decoration: underline;
}

Expand All @@ -179,9 +169,6 @@
--pc-button-bg_disabled: transparent;
margin: calc(-1 * var(--pc-button-padding-block))
calc(-1 * var(--pc-button-padding-inline));
font-size: var(--p-font-size-325);
font-weight: var(--p-font-weight-regular);
line-height: var(--p-font-line-height-400);
}

.variantPlain:focus-visible,
Expand Down Expand Up @@ -252,8 +239,6 @@
.sizeLarge {
--pc-button-padding-block: var(--p-space-150);
--pc-button-padding-inline: var(--p-space-300);
font-size: var(--p-font-size-325);
line-height: var(--p-font-line-height-500);
min-height: var(--p-height-900);
min-width: var(--p-height-900);

Expand Down
20 changes: 17 additions & 3 deletions polaris-react/src/components/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ import {
ChevronUpIcon,
} from '@shopify/polaris-icons';

import {useBreakpoints} from '../../utilities/breakpoints';
import type {BaseButton, IconSource} from '../../types';
import {classNames, variationName} from '../../utilities/css';
import {handleMouseUpByBlurring} from '../../utilities/focus';
import type {MouseUpBlurHandler} from '../../utilities/focus';
import {useI18n} from '../../utilities/i18n';
import {Icon} from '../Icon';
import {Spinner} from '../Spinner';
import {Text} from '../Text';
import type {TextProps} from '../Text';
import {UnstyledButton} from '../UnstyledButton';
import type {UnstyledButtonProps} from '../UnstyledButton';

Expand Down Expand Up @@ -121,6 +124,7 @@ export function Button({
}: ButtonProps) {
const i18n = useI18n();
const isDisabled = disabled || loading;
const {mdUp} = useBreakpoints();

const className = classNames(
styles.Button,
Expand Down Expand Up @@ -164,14 +168,24 @@ export function Button({
<span className={loading ? styles.hidden : styles.Icon}>{iconSource}</span>
) : null;

const hasPlainText = ['plain', 'monochromePlain'].includes(variant);
let textFontWeight: TextProps['fontWeight'] = 'medium';
if (hasPlainText) {
textFontWeight = 'regular';
} else if (variant === 'primary') {
textFontWeight = mdUp ? 'medium' : 'semibold';
}

const childMarkup = children ? (
<span
className={removeUnderline ? styles.removeUnderline : ''}
<Text
as="span"
variant={size === 'large' || hasPlainText ? 'bodyMd' : 'bodySm'}
fontWeight={textFontWeight}
// Fixes Safari bug that doesn't re-render button text to correct color
key={disabled ? 'text-disabled' : 'text'}
>
{children}
</span>
</Text>
) : null;

const spinnerSVGMarkup = loading ? (
Expand Down
41 changes: 31 additions & 10 deletions polaris-react/src/components/Button/tests/Button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,27 @@ import {mountWithApp} from 'tests/utilities';

import {Icon} from '../../Icon';
import {Spinner} from '../../Spinner';
import {Text} from '../../Text';
import {UnstyledButton} from '../../UnstyledButton';
import {Button} from '../Button';

jest.mock('../../../utilities/breakpoints', () => ({
...(jest.requireActual('../../../utilities/breakpoints') as any),
useBreakpoints: jest.fn(),
}));

describe('<Button />', () => {
let warnSpy: jest.SpyInstance | null = null;

beforeEach(() => {
warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
mockUseBreakpoints(false);
});

afterEach(() => warnSpy?.mockRestore());
afterEach(() => {
warnSpy?.mockRestore();
jest.clearAllMocks();
});

describe('children', () => {
it('passes prop', () => {
Expand Down Expand Up @@ -356,15 +366,6 @@ describe('<Button />', () => {
className: expect.stringContaining('removeUnderline'),
});
});

it('passes prop to <span/> className', () => {
const children = 'Sample children';
const button = mountWithApp(<Button removeUnderline>{children}</Button>);
const childrenSpan = button.find('span', {children})!;
expect(childrenSpan).toHaveReactProps({
className: expect.stringContaining('removeUnderline'),
});
});
});

describe('dataPrimaryLink', () => {
Expand All @@ -390,4 +391,24 @@ describe('<Button />', () => {
expect(link).toContainReactComponent('button', selector);
});
});

it('renders with medium fontWeight when on a small screen', () => {
mockUseBreakpoints(true);
const wrapper = mountWithApp(<Button variant="primary">Test</Button>);
expect(wrapper).toContainReactComponent(Text, {
variant: 'bodySm',
fontWeight: 'medium',
children: 'Test',
});
});
});

function mockUseBreakpoints(mdUp: boolean) {
const useBreakpoints: jest.Mock = jest.requireMock(
'../../../utilities/breakpoints',
).useBreakpoints;

useBreakpoints.mockReturnValue({
mdUp,
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,8 @@ describe('<TrapFocus />', () => {
});

describe('handleBlur', () => {
const externalDomNode = mountWithApp(<Button />).find('button')!.domNode;

const event: FocusEvent = new FocusEvent('focusout', {
relatedTarget: externalDomNode,
});
Object.assign(event, {preventDefault: jest.fn()});

it('allows default when trapping is false', () => {
const externalDomNode = mountWithApp(<Button />).find('button')!.domNode;
const trapFocus = mountWithApp(
<TrapFocus trapping={false}>
<TextField
Expand Down Expand Up @@ -212,6 +206,7 @@ describe('<TrapFocus />', () => {
});

it('focuses focusTrapWrapper when focusTrapWrapper does not contain a focusable element and the event target is not the firstFocusableNode', () => {
const externalDomNode = mountWithApp(<Button />).find('button')!.domNode;
const trapFocus = mountWithApp(
<TrapFocus>
<div id="other" />
Expand Down

0 comments on commit a30cc57

Please sign in to comment.