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 3 commits into
base: main
Choose a base branch
from
+17 −9
Open

Banner allows critical variant to use ondismiss #5583

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 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
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": patch
brphelps marked this conversation as resolved.
Show resolved Hide resolved
---

Banner now supports onDismiss used when using variant critical.
1 change: 1 addition & 0 deletions packages/react/src/Banner/Banner.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const Critical = () => {
to enable two-factor authentication as an additional security measure.
</>
}
onDismiss={action('onDismiss')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a docs perspective we might not want this to be visible in the default story for critical, I think the idea was that it would be rare for critical banners to be dismissible since you must address them but I'm curious what @lukasoppermann would say 👀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, @joshblack and I discussed it and we would like to leave the the critical story as is, without the dismiss. Users can still get to it in the playground.

If you could to this change @brphelps, we can merge this.

Thank you for this PR. 🙏

variant="critical"
/>
)
Expand Down
14 changes: 9 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
@@ -1,7 +1,7 @@
import {render, screen} from '@testing-library/react'
import {cleanup, render, screen} from '@testing-library/react'

Check failure on line 1 in packages/react/src/Banner/Banner.test.tsx

View workflow job for this annotation

GitHub Actions / lint 

`cleanup` is performed automatically by your test runner, you don't need manual cleanups
import userEvent from '@testing-library/user-event'
import React from 'react'
import {Banner} from '../Banner'
import {Banner, BannerVariant} from '../Banner'

Check failure on line 4 in packages/react/src/Banner/Banner.test.tsx

View workflow job for this annotation

GitHub Actions / lint 

Imports "BannerVariant" are only used as type
import {FeatureFlags} from '../FeatureFlags'

describe('Banner', () => {
Expand Down Expand Up @@ -169,10 +169,14 @@
expect(onDismiss).toHaveBeenCalledTimes(3)
})

it('should not support onDismiss when `variant="critical"`', () => {
it('should support onDismiss for all variants', () => {
const onDismiss = jest.fn()
render(<Banner title="test" description="test-description" onDismiss={onDismiss} variant="critical" />)
expect(screen.queryByRole('button', {name: 'Dismiss banner'})).toBe(null)
const variantTypes: BannerVariant[] = ['critical', 'info', 'success', 'upsell', 'warning']
variantTypes.map(variant => {
cleanup()
render(<Banner title="test" description="test-description" onDismiss={onDismiss} variant={variant} />)
expect(screen.queryByRole('button', {name: 'Dismiss banner'})).toBeTruthy()
})
})

it('should pass extra props onto the container element', () => {
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