From 8ff5f4137151137de4e78f44bf53ca7ff18dff69 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 6 Sep 2023 16:59:27 -0500 Subject: [PATCH] feat(Button): add support for leadingVisual and trailingVisual --- .changeset/nasty-jobs-itch.md | 7 ++ package-lock.json | 4 +- src/Button/Button.docs.json | 18 +++- src/Button/Button.features.stories.tsx | 4 +- src/Button/Button.stories.tsx | 8 +- src/Button/Button.tsx | 11 ++- src/Button/ButtonBase.tsx | 14 +-- src/{ => Button}/__tests__/Button.test.tsx | 88 +++++++++++++++++-- .../__tests__/Button.types.test.tsx | 10 ++- .../__snapshots__/Button.test.tsx.snap | 0 src/Button/types.ts | 23 ++++- 11 files changed, 154 insertions(+), 33 deletions(-) create mode 100644 .changeset/nasty-jobs-itch.md rename src/{ => Button}/__tests__/Button.test.tsx (50%) rename src/{ => Button}/__tests__/Button.types.test.tsx (88%) rename src/{ => Button}/__tests__/__snapshots__/Button.test.tsx.snap (100%) diff --git a/.changeset/nasty-jobs-itch.md b/.changeset/nasty-jobs-itch.md new file mode 100644 index 00000000000..8698f41de0b --- /dev/null +++ b/.changeset/nasty-jobs-itch.md @@ -0,0 +1,7 @@ +--- +'@primer/react': minor +--- + +Add support for leadingVisual and trailingVisual props to Button + + diff --git a/package-lock.json b/package-lock.json index 03c7e80ea7c..406995744c2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@primer/react", - "version": "35.28.0", + "version": "35.29.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@primer/react", - "version": "35.28.0", + "version": "35.29.0", "license": "MIT", "dependencies": { "@github/combobox-nav": "^2.1.5", diff --git a/src/Button/Button.docs.json b/src/Button/Button.docs.json index 4fc26b7b1bc..bf9c739d76b 100644 --- a/src/Button/Button.docs.json +++ b/src/Button/Button.docs.json @@ -25,12 +25,24 @@ { "name": "leadingIcon", "type": "React.ComponentType", - "description": "An icon to display before the button text." + "description": "An icon to display before the button text.", + "deprecated": true + }, + { + "name": "leadingVisual", + "type": "React.ElementType", + "description": "A visual to display before the button text." }, { "name": "trailingIcon", "type": "React.ComponentType", - "description": "An icon to display after the button text." + "description": "An icon to display after the button text.", + "deprecated": true + }, + { + "name": "trailingVisual", + "type": "React.ElementType", + "description": "A visual to display after the button text." }, { "name": "as", @@ -67,4 +79,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/src/Button/Button.features.stories.tsx b/src/Button/Button.features.stories.tsx index c92ffa956d9..b5611963f75 100644 --- a/src/Button/Button.features.stories.tsx +++ b/src/Button/Button.features.stories.tsx @@ -14,9 +14,9 @@ export const Invisible = () => export const Outline = () => -export const LeadingVisual = () => +export const LeadingVisual = () => -export const TrailingVisual = () => +export const TrailingVisual = () => export const TrailingCounter = () => { const [count, setCount] = useState(0) diff --git a/src/Button/Button.stories.tsx b/src/Button/Button.stories.tsx index daa7ec54ee3..f4d51ee3ba3 100644 --- a/src/Button/Button.stories.tsx +++ b/src/Button/Button.stories.tsx @@ -35,8 +35,8 @@ export default { type: 'boolean', }, }, - leadingIcon: OcticonArgType([EyeClosedIcon, EyeIcon, SearchIcon, XIcon, HeartIcon]), - trailingIcon: OcticonArgType([EyeClosedIcon, EyeIcon, SearchIcon, XIcon, HeartIcon]), + leadingVisual: OcticonArgType([EyeClosedIcon, EyeIcon, SearchIcon, XIcon, HeartIcon]), + trailingVisual: OcticonArgType([EyeClosedIcon, EyeIcon, SearchIcon, XIcon, HeartIcon]), trailingAction: OcticonArgType([TriangleDownIcon]), trailingVisualCount: { control: { @@ -50,8 +50,8 @@ export default { disabled: false, variant: 'default', alignContent: 'center', - trailingIcon: null, - leadingIcon: null, + trailingVisual: null, + leadingVisual: null, trailingAction: null, trailingVisualCount: undefined, }, diff --git a/src/Button/Button.tsx b/src/Button/Button.tsx index f4f02ce123b..99662a270b4 100644 --- a/src/Button/Button.tsx +++ b/src/Button/Button.tsx @@ -7,12 +7,14 @@ import {BetterSystemStyleObject} from '../sx' const ButtonComponent = forwardRef(({children, sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => { let sxStyles = sxProp + const leadingVisual = props.leadingVisual ?? props.leadingIcon + const trailingVisual = props.trailingVisual ?? props.trailingIcon // grap the button props that have associated data attributes in the styles - const {block, size, leadingIcon, trailingIcon, trailingAction} = props + const {block, size, trailingAction} = props if (sxProp !== null && Object.keys(sxProp).length > 0) { - sxStyles = generateCustomSxProp({block, size, leadingIcon, trailingIcon, trailingAction}, sxProp) + sxStyles = generateCustomSxProp({block, size, leadingVisual, trailingVisual, trailingAction}, sxProp) } return ( @@ -63,13 +65,14 @@ sx={{ // We need to make sure we append the customCSSSelector to the original class selector. i.e & - > &[data-attribute="Icon"][data-size="small"] */ export function generateCustomSxProp( - props: Partial>, + props: Partial>, providedSx: BetterSystemStyleObject, ) { // Possible data attributes: data-size, data-block, data-no-visuals const size = props.size && props.size !== 'medium' ? `[data-size="${props.size}"]` : '' // medium is a default size therefore it doesn't have a data attribute that used for styling const block = props.block ? `[data-block="block"]` : '' - const noVisuals = props.leadingIcon || props.trailingIcon || props.trailingAction ? '' : '[data-no-visuals="true"]' + const noVisuals = + props.leadingVisual || props.trailingVisual || props.trailingAction ? '' : '[data-no-visuals="true"]' // this is custom selector. We need to make sure we add the data attributes to the base css class (& -> &[data-attributename="value"]]) const cssSelector = `&${size}${block}${noVisuals}` // &[data-size="small"][data-block="block"][data-no-visuals="true"] diff --git a/src/Button/ButtonBase.tsx b/src/Button/ButtonBase.tsx index 18e38f40df8..bf11edff9e2 100644 --- a/src/Button/ButtonBase.tsx +++ b/src/Button/ButtonBase.tsx @@ -11,8 +11,6 @@ import {defaultSxProp} from '../utils/defaultSxProp' const ButtonBase = forwardRef( ({children, as: Component = 'button', sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => { const { - leadingIcon: LeadingIcon, - trailingIcon: TrailingIcon, trailingAction: TrailingAction, icon: Icon, variant = 'default', @@ -21,6 +19,8 @@ const ButtonBase = forwardRef( block = false, ...rest } = props + const LeadingVisual = props.leadingVisual ?? props.leadingIcon + const TrailingVisual = props.trailingVisual ?? props.trailingIcon const innerRef = React.useRef(null) useRefObjectAsForwardedRef(forwardedRef, innerRef) @@ -65,22 +65,22 @@ const ButtonBase = forwardRef( ref={innerRef} data-block={block ? 'block' : null} data-size={size === 'small' || size === 'large' ? size : undefined} - data-no-visuals={!LeadingIcon && !TrailingIcon && !TrailingAction ? true : undefined} + data-no-visuals={!LeadingVisual && !TrailingVisual && !TrailingAction ? true : undefined} > {Icon ? ( ) : ( <> - {LeadingIcon && ( + {LeadingVisual && ( - + )} {children && {children}} - {TrailingIcon && ( + {TrailingVisual && ( - + )} diff --git a/src/__tests__/Button.test.tsx b/src/Button/__tests__/Button.test.tsx similarity index 50% rename from src/__tests__/Button.test.tsx rename to src/Button/__tests__/Button.test.tsx index edbc3f37c60..cbf50711a8a 100644 --- a/src/__tests__/Button.test.tsx +++ b/src/Button/__tests__/Button.test.tsx @@ -1,10 +1,9 @@ -import React from 'react' -import {IconButton, Button} from '../Button' -import {behavesAsComponent} from '../utils/testing' -import {render, fireEvent} from '@testing-library/react' -import {axe, toHaveNoViolations} from 'jest-axe' import {SearchIcon} from '@primer/octicons-react' -expect.extend(toHaveNoViolations) +import {render, screen, fireEvent} from '@testing-library/react' +import {axe} from 'jest-axe' +import React from 'react' +import {IconButton, Button} from '../../Button' +import {behavesAsComponent} from '../../utils/testing' describe('Button', () => { behavesAsComponent({Component: Button, options: {skipSx: true}}) @@ -89,4 +88,81 @@ describe('Button', () => { const button = container.getByRole('button') expect(button).toMatchSnapshot() }) + + it('should render the leadingIcon prop before the button content', () => { + render( + , + ) + expect(screen.getByTestId('leadingIcon')).toBeInTheDocument() + + const position = screen.getByText('content').compareDocumentPosition(screen.getByTestId('leadingIcon')) + expect(position).toBe(Node.DOCUMENT_POSITION_PRECEDING) + }) + + it('should render the leadingVisual prop before the button content', () => { + render( + , + ) + + expect(screen.getByTestId('leadingVisual')).toBeInTheDocument() + + const position = screen.getByText('content').compareDocumentPosition(screen.getByTestId('leadingVisual')) + expect(position).toBe(Node.DOCUMENT_POSITION_PRECEDING) + }) + + it('should only render the leadingVisual prop if leadingIcon is also defined', () => { + render( + , + ) + + expect(screen.getByTestId('leadingVisual')).toBeInTheDocument() + expect(screen.queryByText('leadingIcon')).toEqual(null) + }) + + it('should render the trailingIcon prop after the button content', () => { + render( + , + ) + expect(screen.getByTestId('trailingIcon')).toBeInTheDocument() + + const position = screen.getByText('content').compareDocumentPosition(screen.getByTestId('trailingIcon')) + expect(position).toBe(Node.DOCUMENT_POSITION_FOLLOWING) + }) + + it('should render the trailingVisual prop after the button content', () => { + render( + , + ) + expect(screen.getByTestId('trailingVisual')).toBeInTheDocument() + + const position = screen.getByText('content').compareDocumentPosition(screen.getByTestId('trailingVisual')) + expect(position).toBe(Node.DOCUMENT_POSITION_FOLLOWING) + }) + + it('should only render the trailingVisual prop if trailingIcon is also defined', () => { + render( + , + ) + + expect(screen.getByTestId('trailingVisual')).toBeInTheDocument() + expect(screen.queryByText('trailingIcon')).toEqual(null) + }) }) diff --git a/src/__tests__/Button.types.test.tsx b/src/Button/__tests__/Button.types.test.tsx similarity index 88% rename from src/__tests__/Button.types.test.tsx rename to src/Button/__tests__/Button.types.test.tsx index 12f928055ea..29e813162d2 100644 --- a/src/__tests__/Button.types.test.tsx +++ b/src/Button/__tests__/Button.types.test.tsx @@ -1,6 +1,6 @@ import {StopIcon} from '@primer/octicons-react' import React, {useRef} from 'react' -import {Button, IconButton} from '../Button' +import {Button, IconButton} from '../../Button' export function shouldAcceptOnlyAChildProp() { return @@ -68,3 +68,11 @@ export function iconButtonShouldNotAcceptOutlandishProps() { // @ts-expect-error system props should not be accepted return +} + +export function supportsTrailingVisual() { + return +} diff --git a/src/__tests__/__snapshots__/Button.test.tsx.snap b/src/Button/__tests__/__snapshots__/Button.test.tsx.snap similarity index 100% rename from src/__tests__/__snapshots__/Button.test.tsx.snap rename to src/Button/__tests__/__snapshots__/Button.test.tsx.snap diff --git a/src/Button/types.ts b/src/Button/types.ts index bccc1c0761c..bd8d96f293b 100644 --- a/src/Button/types.ts +++ b/src/Button/types.ts @@ -42,20 +42,35 @@ export type ButtonProps = { /** * The icon for the IconButton */ - icon?: React.ElementType | null | undefined + icon?: React.ElementType | null + + /** + * The leading visual which comes before the button content + */ + leadingVisual?: React.ElementType | null + /** * The leading icon comes before button content */ - leadingIcon?: React.ElementType | null | undefined + leadingIcon?: React.ElementType | null + /** * The trailing icon comes after button content */ - trailingIcon?: React.ElementType | null | undefined + trailingIcon?: React.ElementType | null + + /** + * The trailing visual which comes after the button content + */ + trailingVisual?: React.ElementType | null + /** * Trailing action appears to the right of the trailing visual and is always locked to the end */ - trailingAction?: React.ElementType | null | undefined + trailingAction?: React.ElementType | null + children: React.ReactNode + /** * Content alignment for when visuals are present */