Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/twenty-baboons-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Remove overflow property from popover (It has no usage)
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member

Choose a reason for hiding this comment

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

I can't see what has changed here? 😅 Do you know? (safe to ignore?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its a flaky test. I tried to make it robust. I think its safe to ignore

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member

Choose a reason for hiding this comment

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

Caret is back, but it seems like the background color has changed? Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did notice this and checked that its the same color that primitive offers. I refreshed my node modules and ran the tests on local and there's no change to the snapshots. The primitives changed the color three months back. Do you think the snapshots were just never updated since it doesn't register color change as an error?

Copy link
Member

@siddharthkp siddharthkp Sep 10, 2025

Choose a reason for hiding this comment

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

that's strange!

Do you think the snapshots were just never updated since it doesn't register color change as an error?

possible, don't think i haven't seen that before 🤔 maybe others have? worth asking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe @langermank knows?

Copy link
Contributor

Choose a reason for hiding this comment

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

This happens sometimes. When the change is really minimal it just doesn't always pick it up.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions e2e/components/IconButton.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ const stories = [
disableAnimations: true,
async setup(page: Page) {
await page.keyboard.press('Tab') // focus on icon button
await page.getByText('Bold').waitFor({
state: 'visible',
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a fix for the flakiness I experienced.

},
},
{
Expand Down
4 changes: 0 additions & 4 deletions e2e/components/Popover.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ const stories = [
title: 'SX Props',
id: 'components-popover-dev--sx-props',
},
{
title: 'Popover Overflow',
id: 'components-popover-dev--popover-overflow',
},
] as const

test.describe('Popover', () => {
Expand Down
16 changes: 0 additions & 16 deletions packages/react/src/Popover/Popover.dev.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,3 @@ export const SxProps = () => (
</Popover.Content>
</Popover>
)

export const PopoverOverflow = () => (
<Popover relative open={true}>
<Popover.Content height={'small'}>
<Heading style={{fontSize: 2}}>Popover heading</Heading>
<Text as="p">Message about popovers</Text>
<Text as="p">Message about popovers</Text>
<Text as="p">Message about popovers</Text>
<Text as="p">Message about popovers</Text>
<Text as="p">Message about popovers</Text>
<Text as="p">Message about popovers</Text>
<Text as="p">Message about popovers</Text>
<Button>Got it!</Button>
</Popover.Content>
</Popover>
)
17 changes: 0 additions & 17 deletions packages/react/src/Popover/Popover.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
background-color: var(--overlay-bgColor);
border: var(--borderWidth-thin) solid var(--borderColor-default);
border-radius: var(--borderRadius-medium);
overflow: hidden;

/* Carets */
&::before,
Expand Down Expand Up @@ -253,20 +252,4 @@
&:where([data-height='auto']) {
height: auto;
}

&:where([data-overflow='auto']) {
overflow: auto;
}

&:where([data-overflow='hidden']) {
overflow: hidden;
}

&:where([data-overflow='scroll']) {
overflow: scroll;
}

&:where([data-overflow='visible']) {
overflow: visible;
}
}
8 changes: 1 addition & 7 deletions packages/react/src/Popover/Popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const Default = () => (

export const Playground: StoryFn<PopoverProps & PopoverContentProps> = args => (
<Popover {...args}>
<Popover.Content sx={{marginTop: 2}} width={args.width || 'small'} height={args.height} overflow={args.overflow}>
<Popover.Content sx={{marginTop: 2}} width={args.width || 'small'} height={args.height}>
<Heading sx={{fontSize: 2}}>Popover heading</Heading>
<Text as="p">Message about popovers</Text>
<Button>Got it!</Button>
Expand Down Expand Up @@ -79,10 +79,4 @@ Playground.argTypes = {
},
options: ['small', 'medium', 'large', 'auto', 'xlarge', 'fit-content'],
},
overflow: {
control: {
type: 'radio',
},
options: ['auto', 'hidden', 'scroll', 'visible'],
},
}
3 changes: 0 additions & 3 deletions packages/react/src/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,19 @@ export type PopoverContentProps = {
as?: React.ElementType
width?: 'xsmall' | 'small' | 'large' | 'medium' | 'auto' | 'xlarge'
height?: 'small' | 'large' | 'medium' | 'auto' | 'xlarge' | 'fit-content'
overflow?: 'auto' | 'hidden' | 'scroll' | 'visible'
} & StyledPopoverProps &
HTMLProps<HTMLDivElement>

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