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

chore(Banner): Remove the CSS modules feature flag from Banner #5282

Merged
merged 8 commits into from
Dec 6, 2024
Merged

chore(Banner): Remove the CSS modules feature flag from Banner #5282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3c3662d
Remove feature flag from Banner
jonrohan Nov 13, 2024
a6c9d97
Create metal-steaks-unite.md
jonrohan Nov 13, 2024
10f6f2d
Fix test
jonrohan Nov 14, 2024
a2a9b53
Merge branch 'main' into css_modules_banner_remove_flag
jonrohan Nov 14, 2024
fc2735c
Merge branch 'main' into css_modules_banner_remove_flag
jonrohan Nov 14, 2024
e96fb88
Merge branch 'main' into css_modules_banner_remove_flag
jonrohan Dec 2, 2024
2888d0e
Merge branch 'main' into css_modules_banner_remove_flag
jonrohan Dec 4, 2024
ed08230
Update Banner tests to use queryAllByRole
jonrohan Dec 5, 2024
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/metal-steaks-unite.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Remove the CSS modules feature flag from Banner
12 changes: 6 additions & 6 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 @@ -115,9 +115,9 @@ describe('Banner', () => {
/>,
)

expect(screen.getByRole('button', {name: 'test primary action'})).toBeInTheDocument()
expect(screen.queryAllByRole('button', {name: 'test primary action', hidden: true}).length).toBe(2)

await user.click(screen.getByRole('button', {name: 'test primary action'}))
await user.click(screen.queryAllByText('test primary action')[0])
expect(onClick).toHaveBeenCalledTimes(1)
})

Expand All @@ -132,9 +132,9 @@ describe('Banner', () => {
/>,
)

expect(screen.getByRole('button', {name: 'test secondary action'})).toBeInTheDocument()
expect(screen.queryAllByRole('button', {name: 'test secondary action', hidden: true}).length).toBe(2)

await user.click(screen.getByRole('button', {name: 'test secondary action'}))
await user.click(screen.queryAllByText('test secondary action')[0])
expect(onClick).toHaveBeenCalledTimes(1)
})

Expand All @@ -148,8 +148,8 @@ describe('Banner', () => {
/>,
)

expect(screen.getByRole('button', {name: 'test primary action'})).toBeInTheDocument()
expect(screen.getByRole('button', {name: 'test secondary action'})).toBeInTheDocument()
expect(screen.queryAllByRole('button', {name: 'test primary action', hidden: true}).length).toBe(2)
expect(screen.queryAllByRole('button', {name: 'test secondary action', hidden: true}).length).toBe(2)
Comment on lines +151 to +152
Copy link
Member

Choose a reason for hiding this comment

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

can you walk me through this? we have to buttons now and they're both hidden?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was always 2 buttons, but I think jest doesn't have access to the CSS modules styles like it did before, so it thinks they're both visible. I only really added the hidden for backwards compatibility but I think it's unnecessary here.

Copy link
Member

Choose a reason for hiding this comment

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

ahh I see , thanks for explaining that! Didn't know jest didn't have access to the css

})

it('should call `onDismiss` when the dismiss button is activated', async () => {
Expand Down
282 changes: 11 additions & 271 deletions packages/react/src/Banner/Banner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
import {clsx} from 'clsx'
import React, {forwardRef, useEffect} from 'react'
import styled from 'styled-components'
import {AlertIcon, InfoIcon, StopIcon, CheckCircleIcon, XIcon} from '@primer/octicons-react'
import {Button, IconButton, type ButtonProps} from '../Button'
import {get} from '../constants'
import {VisuallyHidden} from '../VisuallyHidden'
import {useMergedRefs} from '../internal/hooks/useMergedRefs'
import {useFeatureFlag} from '../FeatureFlags'
import classes from './Banner.module.css'
import {toggleStyledComponent} from '../internal/utils/toggleStyledComponent'
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'

type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning'
Expand Down Expand Up @@ -88,8 +84,6 @@ const labels: Record<BannerVariant, string> = {
warning: 'Warning',
}

const CSS_MODULES_FEATURE_FLAG = 'primer_react_css_modules_ga'

export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner(
{
'aria-label': label,
Expand All @@ -111,7 +105,6 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
const hasActions = primaryAction || secondaryAction
const bannerRef = React.useRef<HTMLElement>(null)
const ref = useMergedRefs(forwardRef, bannerRef)
const enabled = useFeatureFlag(CSS_MODULES_FEATURE_FLAG)
const supportsCustomIcon = variant === 'info' || variant === 'upsell'

if (__DEV__) {
Expand All @@ -137,40 +130,19 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
}

return (
<StyledBanner
<section
{...rest}
aria-label={label ?? labels[variant]}
as="section"
className={clsx(className, {
[classes.Banner]: enabled,
})}
className={clsx(className, classes.Banner)}
data-dismissible={onDismiss ? '' : undefined}
data-title-hidden={hideTitle ? '' : undefined}
data-variant={variant}
tabIndex={-1}
ref={ref}
>
{!enabled ? <style>{BannerContainerQuery}</style> : null}
<div
className={clsx({
BannerIcon: !enabled,
[classes.BannerIcon]: enabled,
})}
>
{icon && supportsCustomIcon ? icon : iconForVariant[variant]}
</div>
<div
className={clsx({
BannerContainer: !enabled,
[classes.BannerContainer]: enabled,
})}
>
<div
className={clsx({
BannerContent: !enabled,
[classes.BannerContent]: enabled,
})}
>
<div className={classes.BannerIcon}>{icon && supportsCustomIcon ? icon : iconForVariant[variant]}</div>
<div className={classes.BannerContainer}>
<div className={classes.BannerContent}>
{title ? (
hideTitle ? (
<VisuallyHidden>
Expand All @@ -189,221 +161,15 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
<IconButton
aria-label="Dismiss banner"
onClick={onDismiss}
className={clsx({
BannerDismiss: !enabled,
[classes.BannerDismiss]: enabled,
})}
className={classes.BannerDismiss}
icon={XIcon}
variant="invisible"
/>
) : null}
</StyledBanner>
</section>
)
})

const StyledBanner = toggleStyledComponent(
CSS_MODULES_FEATURE_FLAG,
/**
* For styling, it's important that the icons and the text have the same height
* for alignment to occur in multi-line scenarios. Currently, we use a
* line-height of `20px` so that means that the height of icons should match
* that value.
*/
'div',
styled.div`
display: grid;
grid-template-columns: auto minmax(0, 1fr) auto;
align-items: start;
background-color: var(--banner-bgColor);
border: var(--borderWidth-thin, 1px) solid var(--banner-borderColor);
padding: var(--base-size-8, 0.5rem);
border-radius: var(--borderRadius-medium, ${get('radii.2')});

@supports (container-type: inline-size) {
container: banner / inline-size;
}

&[data-variant='critical'] {
--banner-bgColor: ${get('colors.danger.subtle')};
--banner-borderColor: ${get('colors.danger.muted')};
--banner-icon-fgColor: ${get('colors.danger.fg')};
}

&[data-variant='info'] {
--banner-bgColor: ${get('colors.accent.subtle')};
--banner-borderColor: ${get('colors.accent.muted')};
--banner-icon-fgColor: ${get('colors.accent.fg')};
}

&[data-variant='success'] {
--banner-bgColor: ${get('colors.success.subtle')};
--banner-borderColor: ${get('colors.success.muted')};
--banner-icon-fgColor: ${get('colors.success.fg')};
}

&[data-variant='upsell'] {
--banner-bgColor: var(--bgColor-upsell-muted, ${get('colors.done.subtle')});
--banner-borderColor: var(--borderColor-upsell-muted, ${get('colors.done.muted')});
--banner-icon-fgColor: var(--fgColor-upsell-muted, ${get('colors.done.fg')});
}

&[data-variant='warning'] {
--banner-bgColor: ${get('colors.attention.subtle')};
--banner-borderColor: ${get('colors.attention.muted')};
--banner-icon-fgColor: ${get('colors.attention.fg')};
}

/* BannerIcon ------------------------------------------------------------- */

.BannerIcon {
display: grid;
place-items: center;
padding: var(--base-size-8, 0.5rem);
}

.BannerIcon svg {
color: var(--banner-icon-fgColor);
fill: var(--banner-icon-fgColor);
/* 20px is the line box height of the trailing action buttons */
height: var(--base-size-20, 1.25rem);
}

&[data-title-hidden=''] .BannerIcon svg {
height: var(--base-size-16, 1rem);
}

/* BannerContainer -------------------------------------------------------- */

.BannerContainer {
font-size: var(--text-body-size-medium, 0.875rem);
align-items: start;
line-height: var(--text-body-lineHeight-medium, calc(20 / 14));
row-gap: var(--base-size-4, 0.25rem);
column-gap: var(--base-size-4, 0.25rem);
}

& :where(.BannerContainer) {
display: flex;
flex-wrap: wrap;
justify-content: space-between;
}

&[data-dismissible]:not([data-title-hidden='']) .BannerContainer {
display: grid;
grid-template-columns: auto;
grid-template-rows: auto;
}

/* BannerContent ---------------------------------------------------------- */

.BannerContent {
display: grid;
row-gap: var(--base-size-4, 0.25rem);
grid-column-start: 1;
margin-block: var(--base-size-8, 0.5rem);
}

&[data-title-hidden=''] .BannerContent {
margin-block: var(--base-size-6, 0.375rem);
}

@media screen and (min-width: 544px) {
.BannerContent {
flex: 1 1 0%;
}
}

.BannerTitle {
margin: 0;
font-size: inherit;
font-weight: var(--base-text-weight-semibold, 600);
}

/* BannerActions ---------------------------------------------------------- */
.BannerActionsContainer {
display: flex;
column-gap: var(--base-size-12, 0.5rem);
align-items: center;
}

.BannerActions :where([data-primary-action='trailing']) {
display: none;
}

@media screen and (min-width: 544px) {
.BannerActions :where([data-primary-action='trailing']) {
display: flex;
}

.BannerActions :where([data-primary-action='leading']) {
display: none;
}
}

&[data-dismissible]:not([data-title-hidden]) .BannerActions {
margin-block-end: var(--base-size-6, 0.375rem);
}

&[data-dismissible]:not([data-title-hidden]) .BannerActionsContainer[data-primary-action='trailing'] {
display: none;
}

&[data-dismissible]:not([data-title-hidden]) .BannerActionsContainer[data-primary-action='leading'] {
display: flex;
}

/* BannerDismiss ---------------------------------------------------------- */

.BannerDismiss {
display: grid;
place-items: center;
padding: var(--base-size-8, 0.5rem);
margin-inline-start: var(--base-size-4, 0.25rem);
}

.BannerDismiss svg {
color: var(--banner-icon-fgColor);
}
`,
)

const BannerContainerQuery = `
@container banner (max-width: 500px) {
.BannerContainer {
display: grid;
grid-template-rows: auto auto;
}

.BannerActions {
margin-block-end: var(--size-small, 0.375rem);
}

.BannerActions [data-primary-action="trailing"] {
display: none;
}

.BannerActions [data-primary-action="leading"] {
display: flex;
}
}

@container banner (min-width: 500px) {
.BannerContainer {
display: grid;
grid-template-columns: auto auto;
}

.BannerActions [data-primary-action="trailing"] {
display: flex;
min-height: var(--base-size-32, 2rem);
}

.BannerActions [data-primary-action="leading"] {
display: none;
}
}
`

type HeadingElement = 'h2' | 'h3' | 'h4' | 'h5' | 'h6'

export type BannerTitleProps<As extends HeadingElement> = {
Expand All @@ -413,16 +179,8 @@ export type BannerTitleProps<As extends HeadingElement> = {

export function BannerTitle<As extends HeadingElement>(props: BannerTitleProps<As>) {
const {as: Heading = 'h2', className, children, ...rest} = props
const enabled = useFeatureFlag(CSS_MODULES_FEATURE_FLAG)
return (
<Heading
{...rest}
className={clsx(className, {
[classes.BannerTitle]: enabled,
BannerTitle: !enabled,
})}
data-banner-title=""
>
<Heading {...rest} className={clsx(className, classes.BannerTitle)} data-banner-title="">
{children}
</Heading>
)
Expand All @@ -444,31 +202,13 @@ export type BannerActionsProps = {
}

export function BannerActions({primaryAction, secondaryAction}: BannerActionsProps) {
const enabled = useFeatureFlag(CSS_MODULES_FEATURE_FLAG)
return (
<div
className={clsx({
[classes.BannerActions]: enabled,
BannerActions: !enabled,
})}
>
<div
className={clsx({
[classes.BannerActionsContainer]: enabled,
BannerActionsContainer: !enabled,
})}
data-primary-action="trailing"
>
<div className={classes.BannerActions}>
<div className={classes.BannerActionsContainer} data-primary-action="trailing">
{secondaryAction ?? null}
{primaryAction ?? null}
</div>
<div
className={clsx({
[classes.BannerActionsContainer]: enabled,
BannerActionsContainer: !enabled,
})}
data-primary-action="leading"
>
<div className={classes.BannerActionsContainer} data-primary-action="leading">
{primaryAction ?? null}
{secondaryAction ?? null}
</div>
Expand Down
Loading