Skip to content

Fix: ActionMenu with overflow doesn’t contain scrollbars within its rounded border #6978

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

Closed
pksjce wants to merge 10 commits into from
Closed

Fix: ActionMenu with overflow doesn’t contain scrollbars within its rounded border #6978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0aab81d
It kinda fixes it
pksjce Oct 10, 2025
5855566
Create serious-schools-hope.md
pksjce Oct 10, 2025
34cc3b9
Merge branch 'main' into pk/actionmenu-scrollbars
pksjce Oct 11, 2025
f28fad1
style(ActionMenu): add spacing between max-height variants
pksjce Oct 13, 2025
c81f4b4
Merge branch 'main' into pk/actionmenu-scrollbars
pksjce Oct 14, 2025
4452cb7
Fix ActionMenu overflow scrollbars appearance
pksjce Oct 14, 2025
38caacc
Merge branch 'main' into pk/actionmenu-scrollbars
pksjce Oct 14, 2025
1c845f8
Merge branch 'main' into pk/actionmenu-scrollbars
pksjce Oct 15, 2025
1edbe60
Merge branch 'main' into pk/actionmenu-scrollbars
pksjce Oct 16, 2025
5769bb8
Merge branch 'main' into pk/actionmenu-scrollbars
pksjce Oct 17, 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/serious-schools-hope.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Fix: ActionMenu with overflow contains scrollbars within its rounded border
42 changes: 42 additions & 0 deletions packages/react/src/ActionMenu/ActionMenu.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,46 @@
&:where([data-variant='fullscreen']) {
padding-top: var(--base-size-36);
}

/* Overflow variants */
&:where([data-overflow-auto]) {
overflow: auto;
}

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

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

&:where([data-overflow-visible]) {
overflow: visible;
}

/* Max-height size tokens (mirror Overlay sizes) */
&:where([data-max-height-xsmall]) {
max-height: 192px;
}

&:where([data-max-height-small]) {
max-height: 256px;
}

&:where([data-max-height-medium]) {
max-height: 320px;
}

&:where([data-max-height-large]) {
max-height: 432px;
}

&:where([data-max-height-xlarge]) {
max-height: 600px;
Comment on lines +25 to +41
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

The hardcoded pixel values for max-height create potential inconsistency with the design system. Consider using CSS custom properties or design tokens to ensure these values stay synchronized with the Overlay component's sizing system that this is mirroring.

Suggested change
max-height: 192px;
}
&:where([data-max-height-small]) {
max-height: 256px;
}
&:where([data-max-height-medium]) {
max-height: 320px;
}
&:where([data-max-height-large]) {
max-height: 432px;
}
&:where([data-max-height-xlarge]) {
max-height: 600px;
max-height: var(--overlay-size-xsmall, 192px);
}
&:where([data-max-height-small]) {
max-height: var(--overlay-size-small, 256px);
}
&:where([data-max-height-medium]) {
max-height: var(--overlay-size-medium, 320px);
}
&:where([data-max-height-large]) {
max-height: var(--overlay-size-large, 432px);
}
&:where([data-max-height-xlarge]) {
max-height: var(--overlay-size-xlarge, 600px);

Copilot uses AI. Check for mistakes.
Copy link
Member

@siddharthkp siddharthkp Oct 14, 2025

Choose a reason for hiding this comment

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

Good suggestion!

}

&:where([data-max-height-fit-content]) {
max-height: fit-content;
}
}
8 changes: 7 additions & 1 deletion packages/react/src/ActionMenu/ActionMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,13 @@ const Overlay: FCWithSlotMarker<React.PropsWithChildren<MenuOverlayProps>> = ({
onPositionChange={onPositionChange}
variant={variant}
>
<div ref={containerRef} className={styles.ActionMenuContainer} data-variant={responsiveVariant}>
<div
ref={containerRef}
className={styles.ActionMenuContainer}
data-variant={responsiveVariant}
{...(overlayProps.overflow ? {[`data-overflow-${overlayProps.overflow}`]: ''} : {})}
Copy link
Member

@siddharthkp siddharthkp Oct 14, 2025

Choose a reason for hiding this comment

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

Do we want a default value or not?

Copy link
Contributor Author

@pksjce pksjce Oct 14, 2025

Choose a reason for hiding this comment

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

not sure :( Don't want to add unnecessary hidden. I just want it to reflect its parent.

Copy link
Member

@siddharthkp siddharthkp Oct 14, 2025

Choose a reason for hiding this comment

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

Valid, what's the default right now? Maybe we should keep that (is it auto?)

Copy link
Contributor Author

@pksjce pksjce Oct 14, 2025

Choose a reason for hiding this comment

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

It came with no value(maybe default is visible from docs). And I add it only if parent has some value.

{...(overlayProps.maxHeight ? {[`data-max-height-${overlayProps.maxHeight}`]: ''} : {})}
Comment on lines +323 to +324
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

The dynamic data attribute generation using template literals could be error-prone if overlayProps contains unexpected values. Consider validating the overflow and maxHeight values against known valid options or using a mapping function to ensure only supported values are used.

Suggested change
{...(overlayProps.overflow ? {[`data-overflow-${overlayProps.overflow}`]: ''} : {})}
{...(overlayProps.maxHeight ? {[`data-max-height-${overlayProps.maxHeight}`]: ''} : {})}
{
(() => {
const allowedOverflows = ['auto', 'visible', 'hidden', 'scroll'];
const allowedMaxHeights = ['small', 'medium', 'large'];
const overflow = overlayProps.overflow;
const maxHeight = overlayProps.maxHeight;
const attrs: {[key: string]: string} = {};
if (overflow && allowedOverflows.includes(overflow)) {
attrs[`data-overflow-${overflow}`] = '';
}
if (maxHeight && allowedMaxHeights.includes(maxHeight)) {
attrs[`data-max-height-${maxHeight}`] = '';
}
return attrs;
})()
}

Copilot uses AI. Check for mistakes.
>
<ActionListContainerContext.Provider
value={{
container: 'ActionMenu',
Expand Down
Loading