feat(Button): add support for leadingVisual and trailingVisual (#3715)

Co-authored-by: Josh Black <joshblack@users.noreply.github.com>

.changeset/nasty-jobs-itch.md

@@ -0,0 +1,7 @@
+---
+'@primer/react': minor
+---
+
+Add support for leadingVisual and trailingVisual props to Button
+
+<!-- Changed components: Button -->

src/Button/Button.docs.json

@@ -25,12 +25,24 @@
     {
       "name": "leadingIcon",
       "type": "React.ComponentType<OcticonProps>",
-      "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<OcticonProps>",
-      "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 @@
       ]
     }
   ]
-}
+}

src/Button/Button.features.stories.tsx

@@ -14,9 +14,9 @@ export const Invisible = () => <Button variant="invisible">Invisible</Button>
 
 export const Outline = () => <Button variant="outline">Outline</Button>
 
-export const LeadingVisual = () => <Button leadingIcon={HeartIcon}>Leading visual</Button>
+export const LeadingVisual = () => <Button leadingVisual={HeartIcon}>Leading visual</Button>
 
-export const TrailingVisual = () => <Button trailingIcon={EyeIcon}>Trailing visual</Button>
+export const TrailingVisual = () => <Button trailingVisual={EyeIcon}>Trailing visual</Button>
 
 export const TrailingCounter = () => {
   const [count, setCount] = useState(0)

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,
   },

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<Pick<ButtonProps, 'size' | 'block' | 'leadingIcon' | 'trailingIcon' | 'trailingAction'>>,
+  props: Partial<Pick<ButtonProps, 'size' | 'block' | 'leadingVisual' | 'trailingVisual' | 'trailingAction'>>,
   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"]

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<HTMLButtonElement>(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 ? (
           <Icon />
         ) : (
           <>
             <Box as="span" data-component="buttonContent" sx={getAlignContentSize(alignContent)}>
-              {LeadingIcon && (
+              {LeadingVisual && (
                 <Box as="span" data-component="leadingVisual" sx={{...iconWrapStyles}}>
-                  <LeadingIcon />
+                  <LeadingVisual />
                 </Box>
               )}
               {children && <span data-component="text">{children}</span>}
-              {TrailingIcon && (
+              {TrailingVisual && (
                 <Box as="span" data-component="trailingVisual" sx={{...iconWrapStyles}}>
-                  <TrailingIcon />
+                  <TrailingVisual />
                 </Box>
               )}
             </Box>

src/__tests__/Button.test.tsx → 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(
+      <Button leadingIcon={() => <span data-testid="leadingIcon" />}>
+        <span>content</span>
+      </Button>,
+    )
+    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(
+      <Button leadingVisual={() => <span data-testid="leadingVisual" />}>
+        <span>content</span>
+      </Button>,
+    )
+
+    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(
+      <Button
+        leadingVisual={() => <span data-testid="leadingVisual" />}
+        leadingIcon={() => <span data-testid="leadingIcon" />}
+      >
+        <span>content</span>
+      </Button>,
+    )
+
+    expect(screen.getByTestId('leadingVisual')).toBeInTheDocument()
+    expect(screen.queryByText('leadingIcon')).toEqual(null)
+  })
+
+  it('should render the trailingIcon prop after the button content', () => {
+    render(
+      <Button trailingIcon={() => <span data-testid="trailingIcon" />}>
+        <span>content</span>
+      </Button>,
+    )
+    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(
+      <Button trailingVisual={() => <span data-testid="trailingVisual" />}>
+        <span>content</span>
+      </Button>,
+    )
+    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(
+      <Button
+        trailingVisual={() => <span data-testid="trailingVisual" />}
+        trailingIcon={() => <span data-testid="trailingIcon" />}
+      >
+        <span>content</span>
+      </Button>,
+    )
+
+    expect(screen.getByTestId('trailingVisual')).toBeInTheDocument()
+    expect(screen.queryByText('trailingIcon')).toEqual(null)
+  })
 })

src/__tests__/Button.types.test.tsx → 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 <Button>child</Button>
@@ -68,3 +68,11 @@ export function iconButtonShouldNotAcceptOutlandishProps() {
   // @ts-expect-error system props should not be accepted
   return <Button icon={StopIcon} aria-label="Stop icon" anOutlandshPropThatShouldNotBeAllowedOnA={'Button'} />
 }
+
+export function supportsLeadingVisual() {
+  return <Button leadingVisual={() => <span />}>child</Button>
+}
+
+export function supportsTrailingVisual() {
+  return <Button trailingVisual={() => <span />}>child</Button>
+}

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
    */