Support overflow scrolling in ActionMenu #3926

strackoverflow · 2023-11-09T17:10:33Z

When looking into #3486, it occurred to me that while we support passing a maxHeight prop, we don't enable overflow scrolling when that prop is provided.

This PR adds an optional overflow prop to Overlay which allows developers to specify the overflow behavior of the menu.

I think ideally we should automatically apply overflow: auto whenever a max height is set (or even for all ActionMenu instances) but at least for now this can unblock this use case without impacting existing usage of Overlay.

Demo

Changelog

New

Changed

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
Integration tests pass at github/github (Learn more about how to run integration tests)

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2023-11-09T17:10:36Z

🦋 Changeset detected

Latest commit: b831070

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

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

github-actions · 2023-11-09T17:13:01Z

size-limit report 📦

Path	Size
dist/browser.esm.js	105.56 KB (+0.02% 🔺)
dist/browser.umd.js	106.14 KB (+0.02% 🔺)

strackoverflow · 2023-11-15T23:50:02Z

Closing in favor of primer/view_components#2383

strackoverflow · 2023-11-15T23:50:26Z

Re-opening, I meant to close a different PR 😄

siddharthkp · 2023-12-01T14:47:47Z

I think ideally we should automatically apply overflow: auto whenever a max height is set (or even for all ActionMenu instances)

Same! Are there any risks in doing that? (for example: Overlays that suddenly have an unexpected scroll?)

If we feel more confident, we can enable it just for primer components that internally use Overlay like ActionMenu, Autocomplete, etc.

siddharthkp

Looks good!

Left a non blocking comment that we can look at in follow up PR

strackoverflow · 2023-12-01T16:35:32Z

Are there any risks in doing that? (for example: Overlays that suddenly have an unexpected scroll?)

@siddharthkp this is the exact thing I want to be cautious about. I agree, we can merge this basic step for now to unblock anyone who needs to enable scrolling but we can revisit to make the DX a little better. Thanks!

strackoverflow · 2023-12-12T19:11:51Z

src/Overlay/Overlay.tsx

@@ -64,7 +65,7 @@ const StyledOverlay = styled.div<StyledOverlayProps>`
  max-height: ${props => props.maxHeight && heightMap[props.maxHeight]};
  width: ${props => widthMap[props.width || 'auto']};
  border-radius: 12px;
-  overflow: hidden;
+  ${props => (props.overflow ? `overflow: ${props.overflow};` : '')}


I just realized we should probably retain the existing overflow behavior if the prop isn't provided so that we don't cause any unexpected side effects. I'll push up a fix for that.

strackoverflow · 2023-12-12T19:42:17Z

@siddharthkp 👋 would you mind taking another look at this PR? I was about to merge it when I realized that this changed the default overflow behavior of Overlay, which was not intentional.

Let me know if the current diff looks good to merge now!

broccolinisoup

Looks great! ✨

support overflow scrolling when maxHeight is set

eb499dc

strackoverflow self-assigned this Nov 9, 2023

strackoverflow temporarily deployed to github-pages November 9, 2023 17:14 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3926 November 9, 2023 17:15 Inactive

strackoverflow mentioned this pull request Nov 15, 2023

Fix overflow issue in ActionMenu primer/view_components#2383

Merged

10 tasks

strackoverflow closed this Nov 15, 2023

strackoverflow reopened this Nov 15, 2023

strackoverflow temporarily deployed to github-pages November 15, 2023 23:55 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3926 November 15, 2023 23:55 Inactive

pass overflow behavior in as a prop

46d88c7

strackoverflow temporarily deployed to github-pages November 16, 2023 21:21 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3926 November 16, 2023 21:22 Inactive

only set overflow if prop is provided

c73d0f2

strackoverflow temporarily deployed to github-pages November 17, 2023 19:49 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3926 November 17, 2023 19:50 Inactive

changeset

0ba90fe

strackoverflow temporarily deployed to github-pages November 27, 2023 17:00 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3926 November 27, 2023 17:01 Inactive

strackoverflow marked this pull request as ready for review November 28, 2023 15:40

strackoverflow requested review from a team and pksjce November 28, 2023 15:40

strackoverflow changed the title ~~Support overflow scrolling when maxHeight is set in ActionMenu~~ Support overflow scrolling in ActionMenu Nov 28, 2023

siddharthkp approved these changes Dec 1, 2023

View reviewed changes

strackoverflow enabled auto-merge December 1, 2023 16:35

strackoverflow disabled auto-merge December 1, 2023 19:05

update snapshot

39c8409

strackoverflow temporarily deployed to github-pages December 12, 2023 18:30 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3926 December 12, 2023 18:30 Inactive

strackoverflow added this pull request to the merge queue Dec 12, 2023

strackoverflow removed this pull request from the merge queue due to a manual request Dec 12, 2023

strackoverflow commented Dec 12, 2023

View reviewed changes

keep overflow hidden as default style

7fabe57

strackoverflow deployed to github-pages December 12, 2023 19:19 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3926 December 12, 2023 19:20 View deployment

Merge branch 'main' into action-menu-max-height

b831070

strackoverflow had a problem deploying to github-pages December 14, 2023 17:34 — with GitHub Actions Failure

strackoverflow had a problem deploying to github-pages December 14, 2023 21:10 — with GitHub Actions Failure

lesliecdubs requested a review from a team December 18, 2023 22:02

broccolinisoup approved these changes Dec 19, 2023

View reviewed changes

strackoverflow added this pull request to the merge queue Dec 19, 2023

Merged via the queue into main with commit 4e4c5ec Dec 19, 2023
28 of 29 checks passed

strackoverflow deleted the action-menu-max-height branch December 19, 2023 03:10

primer bot mentioned this pull request Dec 19, 2023

Release tracking #4085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support overflow scrolling in ActionMenu #3926

Support overflow scrolling in ActionMenu #3926

strackoverflow commented Nov 9, 2023 •

edited

Loading

changeset-bot bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

strackoverflow commented Nov 15, 2023

strackoverflow commented Nov 15, 2023

siddharthkp commented Dec 1, 2023 •

edited

Loading

siddharthkp left a comment •

edited

Loading

strackoverflow commented Dec 1, 2023

strackoverflow Dec 12, 2023

strackoverflow commented Dec 12, 2023

broccolinisoup left a comment

Support overflow scrolling in ActionMenu #3926

Support overflow scrolling in ActionMenu #3926

Conversation

strackoverflow commented Nov 9, 2023 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Nov 9, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 9, 2023 • edited Loading

size-limit report 📦

strackoverflow commented Nov 15, 2023

strackoverflow commented Nov 15, 2023

siddharthkp commented Dec 1, 2023 • edited Loading

siddharthkp left a comment • edited Loading

Choose a reason for hiding this comment

strackoverflow commented Dec 1, 2023

strackoverflow Dec 12, 2023

Choose a reason for hiding this comment

strackoverflow commented Dec 12, 2023

broccolinisoup left a comment

Choose a reason for hiding this comment

strackoverflow commented Nov 9, 2023 •

edited

Loading

changeset-bot bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

siddharthkp commented Dec 1, 2023 •

edited

Loading

siddharthkp left a comment •

edited

Loading