Uh oh!
There was an error while loading. Please reload this page.
chore: always render ActionMenu in viewport when inside Dialog under feature flag - #7524
Conversation
🦋 Changeset detectedLatest commit: 31e8b99 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
There was a problem hiding this comment.
Pull request overview
Adds a feature-flagged behavior change so ActionMenu.Overlay can automatically opt into displayInViewport when rendered inside a portal (e.g., inside a Dialog), aiming to improve overlay positioning/clipping in portaled contexts.
Changes:
- Adds a new default feature flag:
primer_react_action_menu_display_in_viewport_inside_portal. - Updates
ActionMenu.Overlayto consult the feature flag and detect whether it is inside a portal to adjustdisplayInViewport. - Updates the
InsideDialogActionMenu story to enable the flag and remove the explicitdisplayInViewportprop.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/Portal/Portal.tsx | Attempts to ensure a “default” PortalContext exists so descendants can detect portal rendering. |
| packages/react/src/FeatureFlags/DefaultFeatureFlags.ts | Registers the new feature flag with a default value of false. |
| packages/react/src/ActionMenu/ActionMenu.tsx | Adds feature-flag logic and portal detection to influence displayInViewport. |
| packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx | Enables the feature flag in the InsideDialog story and relies on the new default behavior. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
@francinelucca I've opened a new pull request, #7525, to work on those changes. Once the pull request is ready, I'll request review from you. |
Updated the version of '@primer/react' from patch to minor and modified the ActionMenu rendering behavior.
…ithub.com:primer/react into chore/action-menu-display-in-viewport-enhancement
…e_portal feature flag (#7525) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
siddharthkp
left a comment
There was a problem hiding this comment.
Left one clarifying question, everything else looks great! Approving in advance!
| variant={variant} | ||
| displayInViewport={displayInViewport} | ||
| displayInViewport={ | ||
| displayInViewport !== undefined ? displayInViewport : featureFlagDisplayInViewportInsidePortal && isInsideDialog |
There was a problem hiding this comment.
Interesting that we have this in props and automatically applied from context. Should we remove it from props or do we want to preserve displayInViewport=false as an override?
There was a problem hiding this comment.
I was wanting to let the gate open for someone to want to do displayInViewport=true ad-hoc. (like if your ActionMenu is not inside a Dialog, but you still want to force it to displayInViewPort. A good use-case I see for this would be the "close as" - completed, duplicate - not planned - menu for issues when inside the sidebar - it works well now due to some marie-fu, but this'd be a much cleaner solution - ).
And then I got to thinking that if we allow displayInViewport=true, then we should probably allow displayInViewport=false to override the default behavior, hence this seemingly weird conditional. Thoughts?
@francinelucca I've opened a new pull request, #7565, to work on those changes. Once the pull request is ready, I'll request review from you. |
…rtal (#7565) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14744 |
Uh oh!
There was an error while loading. Please reload this page.
…feature flag (#7524) Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Relates to https://github.com/github/primer/issues/6361
Implements @TylerDixon's suggestion to have the ActionMenu.Overlay be able to detect if its being rendered inside a Dialog (via a (new)
DialogContext) to then applydisplayInViewport=true. Adding this under a feature flag since its somewhat of a widespread change.Changelog
New
DialogContextto Dialogprimer_react_action_menu_display_in_viewport_inside_dialogfeature flag toDefaultFeatureFlags, enabling control over overlay display behavior inside dialogs.ActionMenu.Overlayto use the new feature flag and detect if it is inside a Dialog, modifying thedisplayInViewportprop logic accordingly.Changed
InsideDialogstory inFeatureFlagsand removed the explicitdisplayInViewportprop fromActionMenu.Overlay, relying on the feature flag for behavior.Removed
Rollout strategy
Testing & Reviewing
Merge checklist