Uh oh!
There was an error while loading. Please reload this page.
Remove overflow property from popover - #6709
Conversation
🦋 Changeset detectedLatest commit: 0394474 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the overflow property from the Popover component to fix an issue where the default overflow:hidden was causing the popover's caret to disappear.
- Removes the
overflowprop fromPopoverContentPropsinterface and its default value - Removes overflow-related CSS styles and data attributes from the component implementation
- Updates Storybook stories to remove overflow controls and usage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react/src/Popover/Popover.tsx | Removes overflow prop from PopoverContentProps interface and removes overflow handling in PopoverContent component |
| packages/react/src/Popover/Popover.stories.tsx | Updates Playground story to remove overflow prop usage and removes overflow controls from argTypes |
| packages/react/src/Popover/Popover.module.css | Removes default overflow:hidden style and all overflow-related CSS rules |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
| await page.keyboard.press('Tab') // focus on icon button | ||
| await page.getByText('Bold').waitFor({ | ||
| state: 'visible', | ||
| }) |
There was a problem hiding this comment.
This is just a fix for the flakiness I experienced.
langermank
left a comment
There was a problem hiding this comment.
@pksjce not sure if you've accounted for this but since the caret was broken, we had to suggest teams use the overflow prop to get around it: https://primer-query.githubapp.com/?query=popover+attribute%3Aoverflow
I wonder if we should just change the default prop to visible, and then remove the prop in a separate PR as a major breaking change?
pksjce
commented
Sep 8, 2025
I see that this affects two uages. I can have a github-ui PR ready to go when this change is released. Does that work? |
Fixes for release(path-to-green)- Will update in release tracking PR after merge https://github.com/github/github-ui/pull/2049/commits/9194c3449bfb80a3977e7814edcedeeedc40d05b |
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
siddharthkp
left a comment
There was a problem hiding this comment.
Left a couple comments to confirm if the visual changes are intentional
Approving in advance
There was a problem hiding this comment.
Caret is back, but it seems like the background color has changed? Is that intentional?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This happens sometimes. When the change is really minimal it just doesn't always pick it up.
There was a problem hiding this comment.
I can't see what has changed here? 😅 Do you know? (safe to ignore?)
There was a problem hiding this comment.
I think its a flaky test. I tried to make it robust. I think its safe to ignore
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: pksjce <417268+pksjce@users.noreply.github.com>
Closes #https://github.com/github/primer/issues/5729
The original issue was a complaint that popover does not have customisable width and that they were replacing it with an
AnchoredOverlay. In #6303, I fixed this and also premptively added support for height and overflow. However,overflow:hiddendefault property makes the caret disappear. Fixing this here by removing overflow property altogether. If a need for this property arises in the future, we can look into how to safely add it. Probably by changing the implementation of the caret visual.Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist