From db6c3607c43ba288d4df1a68e1590bbb5f5aedb6 Mon Sep 17 00:00:00 2001
From: Brandyn Phelps <brphelps@github.com>
Date: Thu, 23 Jan 2025 09:25:53 -0800
Subject: [PATCH] Banner allows critical variant to use ondismiss (#5583)

* Allow critical on dismiss, ready to test

* add changeset

* Update .changeset/blue-actors-rule.md

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

* Remove onDismiss action from Banner story

* refactor(test): update to address eslint warnings

---------

Co-authored-by: Josh Black <joshblack@github.com>
---
 .changeset/blue-actors-rule.md            |  5 +++++
 packages/react/src/Banner/Banner.test.tsx | 13 ++++++++-----
 packages/react/src/Banner/Banner.tsx      |  6 ++----
 3 files changed, 15 insertions(+), 9 deletions(-)
 create mode 100644 .changeset/blue-actors-rule.md

diff --git a/.changeset/blue-actors-rule.md b/.changeset/blue-actors-rule.md
new file mode 100644
index 00000000000..fb4743875e1
--- /dev/null
+++ b/.changeset/blue-actors-rule.md
@@ -0,0 +1,5 @@
+---
+"@primer/react": minor
+---
+
+Banner now supports onDismiss used when using variant critical.
diff --git a/packages/react/src/Banner/Banner.test.tsx b/packages/react/src/Banner/Banner.test.tsx
index 59975b0f63c..28fce588e4f 100644
--- a/packages/react/src/Banner/Banner.test.tsx
+++ b/packages/react/src/Banner/Banner.test.tsx
@@ -169,11 +169,14 @@ describe('Banner', () => {
     expect(onDismiss).toHaveBeenCalledTimes(3)
   })
 
-  it('should not support onDismiss when `variant="critical"`', () => {
-    const onDismiss = jest.fn()
-    render(<Banner title="test" description="test-description" onDismiss={onDismiss} variant="critical" />)
-    expect(screen.queryByRole('button', {name: 'Dismiss banner'})).toBe(null)
-  })
+  it.each(['critical', 'info', 'success', 'upsell', 'warning'] as const)(
+    'should support onDismiss for the %s variant',
+    variant => {
+      const onDismiss = jest.fn()
+      render(<Banner title="test" description="test-description" onDismiss={onDismiss} variant={variant} />)
+      expect(screen.queryByRole('button', {name: 'Dismiss banner'})).toBeInTheDocument()
+    },
+  )
 
   it('should pass extra props onto the container element', () => {
     const {container} = render(<Banner title="test" data-testid="test" />)
diff --git a/packages/react/src/Banner/Banner.tsx b/packages/react/src/Banner/Banner.tsx
index e1c7a1119e8..527e45177b0 100644
--- a/packages/react/src/Banner/Banner.tsx
+++ b/packages/react/src/Banner/Banner.tsx
@@ -7,7 +7,7 @@ import {useMergedRefs} from '../internal/hooks/useMergedRefs'
 import classes from './Banner.module.css'
 import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
 
-type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning'
+export type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning'
 
 export type BannerProps = React.ComponentPropsWithoutRef<'section'> & {
   /**
@@ -41,8 +41,6 @@ export type BannerProps = React.ComponentPropsWithoutRef<'section'> & {
   /**
    * Optionally provide a handler to be called when the banner is dismissed.
    * Providing this prop will show a dismiss button.
-   *
-   * Note: This is not available for critical banners.
    */
   onDismiss?: () => void
 
@@ -101,7 +99,7 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
   },
   forwardRef,
 ) {
-  const dismissible = variant !== 'critical' && onDismiss
+  const dismissible = !!onDismiss
   const hasActions = primaryAction || secondaryAction
   const bannerRef = React.useRef<HTMLElement>(null)
   const ref = useMergedRefs(forwardRef, bannerRef)