Skip to content

Popover: Remove sx from component #6754

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

Merged
TylerJDev merged 13 commits into from
Sep 12, 2025
Merged

Popover: Remove sx from component #6754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
21c2e83
Remove `sx` from `Popover`
TylerJDev Sep 8, 2025
2fdafd3
Fix stories
TylerJDev Sep 8, 2025
5149cf5
Add changeset
TylerJDev Sep 8, 2025
9420b56
Remove deleted story from test.ts
TylerJDev Sep 8, 2025
8787373
test(vrt): update snapshots
TylerJDev Sep 8, 2025
5c63096
test(vrt): update snapshots
TylerJDev Sep 8, 2025
672104a
Fix snapshots
TylerJDev Sep 8, 2025
af0be59
Merge branch 'main' into tylerjdev/remove-sx-form-popover
TylerJDev Sep 8, 2025
ed01b1a
Merge branch 'main' into tylerjdev/remove-sx-form-popover
TylerJDev Sep 12, 2025
06e9833
Remove `Popover` from `styled-react`
TylerJDev Sep 12, 2025
1931e55
test(vrt): update snapshots
TylerJDev Sep 12, 2025
b62cff4
Update snapshots
TylerJDev Sep 12, 2025
9d888a6
Add back snapshot
TylerJDev Sep 12, 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/busy-bananas-smash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': major
---

Update Popover to no longer support sx
4 changes: 0 additions & 4 deletions e2e/components/Popover.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ const stories = [
title: 'Playground',
id: 'components-popover--playground',
},
{
title: 'SX Props',
id: 'components-popover-dev--sx-props',
},
] as const

test.describe('Popover', () => {
Expand Down
37 changes: 0 additions & 37 deletions packages/react/src/Popover/Popover.dev.stories.tsx
View file
Open in desktop

This file was deleted.

13 changes: 9 additions & 4 deletions packages/react/src/Popover/Popover.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export default {

export const Default = () => (
<Popover relative open={true} caret="top">
<Popover.Content sx={{marginTop: 2}}>
<Heading sx={{fontSize: 2}}>Popover heading</Heading>
<Popover.Content style={{marginTop: 'var(--base-size-8)'}}>
<Heading style={{fontSize: 'var(--text-title-size-small)'}}>Popover heading</Heading>
Comment on lines +14 to +15
Copy link

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

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

The CSS custom property --text-title-size-small appears to be incorrect. Based on Primer's design system conventions, this should likely be --text-title-size-small or a similar valid token. Please verify this is a valid CSS custom property in the design system.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Copy Markdown
Contributor

@llastflowers llastflowers Sep 8, 2025

Choose a reason for hiding this comment

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

🙃 what

<Text as="p">Message about popovers</Text>
<Button>Got it!</Button>
</Popover.Content>
Expand All @@ -21,8 +21,13 @@ export const Default = () => (

export const Playground: StoryFn<PopoverProps & PopoverContentProps> = args => (
<Popover {...args}>
<Popover.Content sx={{marginTop: 2}} width={args.width || 'small'} height={args.height}>
<Heading sx={{fontSize: 2}}>Popover heading</Heading>
<Popover.Content
style={{marginTop: 'var(--base-size-8)'}}
width={args.width || 'small'}
height={args.height}
overflow={args.overflow}
>
<Heading style={{fontSize: 'var(--text-title-size-small)'}}>Popover heading</Heading>
<Text as="p">Message about popovers</Text>
<Button>Got it!</Button>
</Popover.Content>
Expand Down
21 changes: 5 additions & 16 deletions packages/react/src/Popover/Popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import {clsx} from 'clsx'
import type {SxProp} from '../sx'
import classes from './Popover.module.css'
import type {HTMLProps} from 'react'
import React from 'react'
import {BoxWithFallback} from '../internal/components/BoxWithFallback'

type CaretPosition =
| 'top'
Expand All @@ -26,12 +24,11 @@ type StyledPopoverProps = {
caret?: CaretPosition
relative?: boolean
open?: boolean
} & SxProp
}

export type PopoverProps = {
/** Class name for custom styling */
className?: string
as?: React.ElementType
} & StyledPopoverProps &
HTMLProps<HTMLDivElement>

Expand All @@ -40,7 +37,7 @@ const Popover = React.forwardRef<HTMLDivElement, PopoverProps>(function Popover(
forwardRef,
) {
return (
<BoxWithFallback
<div
{...props}
ref={forwardRef}
data-open={open ? '' : undefined}
Expand All @@ -54,26 +51,18 @@ Popover.displayName = 'Popover'

export type PopoverContentProps = {
className?: string
as?: React.ElementType
width?: 'xsmall' | 'small' | 'large' | 'medium' | 'auto' | 'xlarge'
height?: 'small' | 'large' | 'medium' | 'auto' | 'xlarge' | 'fit-content'
} & StyledPopoverProps &
HTMLProps<HTMLDivElement>
overflow?: 'auto' | 'hidden' | 'scroll' | 'visible'
} & HTMLProps<HTMLDivElement>

const PopoverContent: React.FC<React.PropsWithChildren<PopoverContentProps>> = ({
className,
width = 'small',
height = 'fit-content',
...props
}) => {
return (
<BoxWithFallback
data-width={width}
data-height={height}
className={clsx(className, classes.PopoverContent)}
{...props}
/>
)
return <div data-width={width} data-height={height} className={clsx(className, classes.PopoverContent)} {...props} />
}

PopoverContent.displayName = 'Popover.Content'
Expand Down
1 change: 0 additions & 1 deletion packages/styled-react/src/__tests__/__snapshots__/exports.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ exports[`@primer/styled-react exports 1`] = `
"Overlay",
"PageHeader",
"PageLayout",
"Popover",
"ProgressBar",
"RadioGroup",
"RelativeTime",
Expand Down
11 changes: 0 additions & 11 deletions packages/styled-react/src/__tests__/primer-react.browser.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
Overlay,
PageHeader,
PageLayout,
Popover,
ProgressBar,
RadioGroup,
RelativeTime,
Expand Down Expand Up @@ -287,16 +286,6 @@ describe('@primer/react', () => {
expect(window.getComputedStyle(container.firstElementChild!).backgroundColor).toBe('rgb(255, 0, 0)')
})

test('Popover supports `sx` prop', () => {
const {container} = render(<Popover data-testid="component" sx={{background: 'red'}} />)
expect(window.getComputedStyle(container.firstElementChild!).backgroundColor).toBe('rgb(255, 0, 0)')
})

test('Popover.Content supports `sx` prop', () => {
const {container} = render(<Popover.Content data-testid="component" sx={{background: 'red'}} />)
expect(window.getComputedStyle(container.firstElementChild!).backgroundColor).toBe('rgb(255, 0, 0)')
})

test('ProgressBar supports `sx` prop', () => {
const {container} = render(<ProgressBar data-testid="component" sx={{background: 'red'}} />)
expect(window.getComputedStyle(container.firstElementChild!).backgroundColor).toBe('rgb(255, 0, 0)')
Expand Down
1 change: 0 additions & 1 deletion packages/styled-react/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ export {
Overlay,
PageHeader,
PageLayout,
Popover,
ProgressBar,
RadioGroup,
RelativeTime,
Expand Down
Loading