Banner: Deprecate icon prop in favor of leadingVisual, fix SecondaryAction styling and playground story

.changeset/open-mails-prove.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Deprecate `icon` prop in favor of `leadingVisual` and fix SecondaryAction styling in Banner.

packages/react/src/Banner/Banner.docs.json

@@ -75,7 +75,13 @@
     {
       "name": "icon",
       "type": "React.ReactNode",
-      "description": "Provide a custom icon for the Banner. This is only available when `variant` is `info` or `upsell`"
+      "description": "Provide a custom icon for the Banner. This is only available when `variant` is `info` or `upsell`",
+      "deprecated": true
+    },
+    {
+      "name": "leadingVisual",
+      "type": "React.ReactNode",
+      "description": "Provide a custom leading visual for the Banner. This is only available when `variant` is `info` or `upsell`"
     },
     {
       "name": "onDismiss",
@@ -151,4 +157,4 @@
       }
     }
   ]
-}
+}

packages/react/src/Banner/Banner.features.stories.tsx

@@ -255,7 +255,7 @@ export const CustomIcon = () => {
     <Banner
       title="Upsell"
       description="An example banner with a custom icon"
-      icon={<CopilotIcon />}
+      leadingVisual={<CopilotIcon />}
       onDismiss={action('onDismiss')}
       variant="upsell"
     />

packages/react/src/Banner/Banner.stories.tsx

@@ -3,6 +3,7 @@ import Link from '../Link'
 import {Banner} from '../Banner'
 import {PageLayout} from '../PageLayout'
 import {action} from 'storybook/actions'
+import {CopilotIcon, GitPullRequestIcon} from '@primer/octicons-react'
 
 const meta = {
   title: 'Components/Banner',
@@ -31,8 +32,16 @@ export const Default = () => {
   )
 }
 
+const iconMap = {
+  GitPullRequestIcon: <GitPullRequestIcon />,
+  CopilotIcon: <CopilotIcon />,
+}
+
 export const Playground: StoryObj<typeof Banner> = {
-  render: ({onDismiss, primaryAction, secondaryAction, ...rest}) => {
+  render: ({onDismiss, primaryAction, secondaryAction, leadingVisual, ...rest}) => {
+    // Map the string selection to the actual icon component
+    const leadingVisualElement = leadingVisual && iconMap[leadingVisual as keyof typeof iconMap]
+
     return (
       <PageLayout>
         <PageLayout.Pane divider="line" position="start">
@@ -43,6 +52,7 @@ export const Playground: StoryObj<typeof Banner> = {
             secondaryAction={
               secondaryAction ? <Banner.SecondaryAction>{secondaryAction}</Banner.SecondaryAction> : null
             }
+            leadingVisual={leadingVisualElement}
             {...rest}
           />
         </PageLayout.Pane>
@@ -55,6 +65,7 @@ export const Playground: StoryObj<typeof Banner> = {
             secondaryAction={
               secondaryAction ? <Banner.SecondaryAction>{secondaryAction}</Banner.SecondaryAction> : null
             }
+            leadingVisual={leadingVisualElement}
             {...rest}
           />
         </PageLayout.Content>
@@ -78,6 +89,12 @@ export const Playground: StoryObj<typeof Banner> = {
     description: {
       control: 'text',
     },
+    leadingVisual: {
+      control: {
+        type: 'select',
+      },
+      options: [undefined, 'GitPullRequestIcon', 'CopilotIcon'],
+    },
     onDismiss: {
       control: 'boolean',
       defaultValue: false,

packages/react/src/Banner/Banner.test.tsx

@@ -188,6 +188,42 @@ describe('Banner', () => {
     expect(screen.queryByTestId('icon')).toBe(null)
   })
 
+  it('should support a custom leadingVisual for info and upsell variants', () => {
+    const CustomIcon = vi.fn(() => <svg data-testid="leading-visual" aria-hidden="true" />)
+    const {rerender} = render(
+      <Banner title="test" description="test-description" variant="info" leadingVisual={<CustomIcon />} />,
+    )
+    expect(screen.getByTestId('leading-visual')).toBeInTheDocument()
+
+    rerender(<Banner title="test" description="test-description" variant="upsell" leadingVisual={<CustomIcon />} />)
+    expect(screen.getByTestId('leading-visual')).toBeInTheDocument()
+
+    rerender(<Banner title="test" description="test-description" variant="critical" leadingVisual={<CustomIcon />} />)
+    expect(screen.queryByTestId('leading-visual')).toBe(null)
+
+    rerender(<Banner title="test" description="test-description" variant="success" leadingVisual={<CustomIcon />} />)
+    expect(screen.queryByTestId('leading-visual')).toBe(null)
+
+    rerender(<Banner title="test" description="test-description" variant="warning" leadingVisual={<CustomIcon />} />)
+    expect(screen.queryByTestId('leading-visual')).toBe(null)
+  })
+
+  it('should prefer leadingVisual over icon when both are provided', () => {
+    const LeadingVisualIcon = () => <svg data-testid="leading-visual" aria-hidden="true" />
+    const DeprecatedIcon = () => <svg data-testid="deprecated-icon" aria-hidden="true" />
+    render(
+      <Banner
+        title="test"
+        description="test-description"
+        variant="info"
+        leadingVisual={<LeadingVisualIcon />}
+        icon={<DeprecatedIcon />}
+      />,
+    )
+    expect(screen.getByTestId('leading-visual')).toBeInTheDocument()
+    expect(screen.queryByTestId('deprecated-icon')).toBe(null)
+  })
+
   it('should render data-actions-layout attribute with inline value', () => {
     const {container} = render(<Banner title="test" actionsLayout="inline" />)
     expect(container.firstChild).toHaveAttribute('data-actions-layout', 'inline')

packages/react/src/Banner/Banner.tsx

@@ -35,9 +35,15 @@ export type BannerProps = React.ComponentPropsWithoutRef<'section'> & {
 
   /**
    * Provide a custom icon for the Banner. This is only available when `variant` is `info` or `upsell`
+   * @deprecated Use `leadingVisual` instead
    */
   icon?: React.ReactNode
 
+  /**
+   * Provide a custom leading visual for the Banner. This is only available when `variant` is `info` or `upsell`
+   */
+  leadingVisual?: React.ReactNode
+
   /**
    * Optionally provide a handler to be called when the banner is dismissed.
    * Providing this prop will show a dismiss button.
@@ -101,6 +107,7 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
     description,
     hideTitle,
     icon,
+    leadingVisual,
     onDismiss,
     primaryAction,
     secondaryAction,
@@ -117,6 +124,8 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
   const ref = useMergedRefs(forwardRef, bannerRef)
   const supportsCustomIcon = variant === 'info' || variant === 'upsell'
 
+  const visual = leadingVisual ?? icon
+
   if (__DEV__) {
     // This hook is called consistently depending on the environment
     // eslint-disable-next-line react-hooks/rules-of-hooks
@@ -153,7 +162,7 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
       ref={ref}
       data-layout={rest.layout || 'default'}
     >
-      <div className={classes.BannerIcon}>{icon && supportsCustomIcon ? icon : iconForVariant[variant]}</div>
+      <div className={classes.BannerIcon}>{visual && supportsCustomIcon ? visual : iconForVariant[variant]}</div>
       <div className={classes.BannerContainer}>
         <div className={classes.BannerContent}>
           {title ? (
@@ -245,7 +254,7 @@ export type BannerSecondaryActionProps = Omit<ButtonProps, 'variant'>
 
 const BannerSecondaryAction = forwardRef(({children, className, ...rest}, forwardedRef) => {
   return (
-    <Button ref={forwardedRef} className={clsx('BannerPrimaryAction', className)} variant="link" {...rest}>
+    <Button ref={forwardedRef} className={clsx('BannerPrimaryAction', className)} variant="invisible" {...rest}>
       {children}
     </Button>
   )