Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Banner allows critical variant to use ondismiss #5583

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+15 −9
Open

Banner allows critical variant to use ondismiss #5583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
18a7d8b
Allow critical on dismiss, ready to test
brphelps Jan 21, 2025
b6020c3
add changeset
brphelps Jan 21, 2025
4e34a4b
Update .changeset/blue-actors-rule.md
brphelps Jan 21, 2025
98df8a6
Remove onDismiss action from Banner story
brphelps Jan 22, 2025
9a3da42
refactor(test): update to address eslint warnings
joshblack Jan 23, 2025
2206468
Merge branch 'main' into banner-allows-critical-ondismiss
joshblack Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/blue-actors-rule.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading