Skip to content

Commit

Permalink
Banner allows critical variant to use ondismiss (#5583)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
brphelps and joshblack authored Jan 23, 2025
1 parent ccc3c99 commit db6c360
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 9 deletions.
5 changes: 5 additions & 0 deletions .changeset/blue-actors-rule.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Banner now supports onDismiss used when using variant critical.
13 changes: 8 additions & 5 deletions packages/react/src/Banner/Banner.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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" />)
Expand Down
6 changes: 2 additions & 4 deletions packages/react/src/Banner/Banner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'> & {
/**
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit db6c360

Please sign in to comment.